OProfile

On Wed, Jan 27, 2010 at 08:25:52AM -0800, Myke Smith wrote:

> On hand held devices we are -typically- very memory constrained and as  
> such the standard vmlinux image is not available. The attached patch  
> re-uses and re-implements some of the ideas in Brian Eaton's patch  
> from the summer of 2006.

Why both? kallsyms seems sensible if not preferred, but why System.map
support?

regards
john

John,

I don't have a strong reason for supporting both. It was easy to do  
both at the same time.  I was initially following Brian Eaton's idea  
using System.map from 0.9.3 and then it dawned on me that I could do  
kallsyms just as easily so I did it.

If you have a strong feeling on this I can easily prune out  
System.map  I am not wedded to the need and in some sense it makes it  
simpler.

-myke

On Jan 27, 2010, at 11:52 AM, John Levon wrote:

> On Wed, Jan 27, 2010 at 08:25:52AM -0800, Myke Smith wrote:
>
>> On hand held devices we are -typically- very memory constrained and  
>> as
>> such the standard vmlinux image is not available. The attached patch
>> re-uses and re-implements some of the ideas in Brian Eaton's patch
>> from the summer of 2006.
>
> Why both? kallsyms seems sensible if not preferred, but why System.map
> support?
>
> regards
> john

Myke Smith wrote:
> John,
> 
> I don't have a strong reason for supporting both. It was easy to do  
> both at the same time.  I was initially following Brian Eaton's idea  
> using System.map from 0.9.3 and then it dawned on me that I could do  
> kallsyms just as easily so I did it.
> 
> If you have a strong feeling on this I can easily prune out  
> System.map  I am not wedded to the need and in some sense it makes it  
> simpler.
I think that would be best.  The /proc/kallsyms should always be present.  On the other hand, I'm not sure that distros provide /boot/System.map in all cases -- they probably do, but just not sure.

So let's keep it simple and just support kallsyms.  When you resubmit you patch, please build the patch with 'diff -paur' as it's much easier to read.  Also, please include a ChangeLog entry.

Thanks.
-Maynard
> 
> -myke
> 
> On Jan 27, 2010, at 11:52 AM, John Levon wrote:
> 
>> On Wed, Jan 27, 2010 at 08:25:52AM -0800, Myke Smith wrote:
>>
>>> On hand held devices we are -typically- very memory constrained and  
>>> as
>>> such the standard vmlinux image is not available. The attached patch
>>> re-uses and re-implements some of the ideas in Brian Eaton's patch
>>> from the summer of 2006.
>> Why both? kallsyms seems sensible if not preferred, but why System.map
>> support?
>>
>> regards
>> john
> 
> 
> ------------------------------------------------------------------------------
> The Planet: dedicated and managed hosting, cloud storage, colocation
> Stay online with enterprise data centers and the best network in the business
> Choose flexible plans and management services without long-term contracts
> Personal 24x7 support from experience hosting pros just a phone call away.
> http://p.sf.net/sfu/theplanet-com
> _______________________________________________
> oprofile-list mailing list
> oprofile-list@...
> https://lists.sourceforge.net/lists/listinfo/oprofile-list

On hand held devices we are -typically- very memory constrained and   
as such the standard vmlinux image is not available. The attached  
patch re-uses and re-implements some of the ideas in Brian Eaton's  
patch from the summer of 2006.

Specifically it adds a flag to opcontrol --use-kallsyms which in  
combination with --vm-nolinux  passes /proc/kallsyms to oprofiled.

There are modifications to libutil++/op_bfd.* to detect the use of / 
proc/kallsyms and complementary routines to the standard BFD based  
routines to handle the text versus binary file.




Signed-off-by: Myke Smith <myke.smith@...>

Found and fixed a bug in opcontrol.

This occurred if you chose not to identify any kernel symbols, did not  
set --use-kallsyms. This caused a hang in opcontrol get_image().

The attached patch has everything in the previous patch plus this fix.  
Not sure if this is the appropriate way or if you want a patch to just  
opcontrol. If you just want a patch to opcontrol let me know and I  
will make it.

Myke Smith wrote:
> Found and fixed a bug in opcontrol.
> 
> This occurred if you chose not to identify any kernel symbols, did not 
> set --use-kallsyms. This caused a hang in opcontrol get_image().
> 
> The attached patch has everything in the previous patch plus this fix. 
> Not sure if this is the appropriate way or if you want a patch to just 
> opcontrol. If you just want a patch to opcontrol let me know and I will 
> make it.
> 
> 
> 
> 
> Signed-off-by: Myke Smith <myke.smith@...>
> 
> 
> On Jan 28, 2010, at 10:44 AM, Myke Smith wrote:

I apologize for the delay in reviewing this patch.  Other priorities got in the 
way.  Thanks for the submission.  Comments embedded below.

-Maynard

> 
>>
>> On hand held devices we are -typically- very memory constrained and  
>> as such the standard vmlinux image is not available. The attached 
>> patch re-uses and re-implements some of the ideas in Brian Eaton's 
>> patch from the summer of 2006.

Adding the "Signed-off-by" line below, implies you have read and are complying 
with the "Developer's Certificate of Origin 1.1" (DCO) from 
http://lwn.net/Articles/139918/.  Since by your own admission, this patch is 
based on an original patch from Brian Eaton, you need to get Brian to sign off 
on this patch.

>>
>> Specifically it adds a flag to opcontrol --use-kallsyms which in 
>> combination with --vm-nolinux  passes /proc/kallsyms to oprofiled.
I presume you mean "--no-vmlinux".
>>
>> There are modifications to libutil++/op_bfd.* to detect the use of 
>> /proc/kallsyms and complementary routines to the standard BFD based 
>> routines to handle the text versus binary file.
>>
>>
>>
>>
>> Signed-off-by: Myke Smith <myke.smith@...>
>>
> Index: oprofile/ChangeLog
> ===================================================================

Your patch does not compile for me.  I get the following error:

++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
make[3]: Entering directory `/home/mpj/oprof-work/op-tmp/libutil++'
if g++ -DHAVE_CONFIG_H -I. -I. -I..  -I ../libutil -I ../libop -I ../libpp  -W 
-Wall -fno-common -ftemplate-depth-50 -Werror -g -O2 -MT op_bfd.o -MD -MP -MF 
".deps/op_bfd.Tpo" -c -o op_bfd.o op_bfd.cpp; \
then mv -f ".deps/op_bfd.Tpo" ".deps/op_bfd.Po"; else rm -f ".deps/op_bfd.Tpo"; 
exit 1; fi
cc1plus: warnings being treated as errors
op_bfd.h: In constructor ?op_bfd::op_bfd(const std::string&, const 
string_filter&, const extra_images&, bool&)?:
op_bfd.h:332: warning: ?op_bfd::section_vma? will be initialized after
op_bfd.h:329: warning:   ?long unsigned int op_bfd::section_filepos?
op_bfd.cpp:114: warning:   when initialized here
make[3]: *** [op_bfd.o] Error 1
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

Member initializers are executed in the order in which they are found in the 
class defintiion.  The warning message is telling you that the order in which 
you coded the member initializers in the class constructor differs from that 
found in the class definition.  If you patched and built this with an oprofile 
cvs checkout, the warning should have caused a compile error for you.  Maybe 
your C++ compiler just isn't catching this.

After fixing the above compile error, I ran into another one.  This second error 
was only failing on a more recent g++ version (4.3):

++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
make[3]: Entering directory `/home/mpj/temp/oprofile/libutil++'
g++ -DHAVE_CONFIG_H -I. -I..  -I ../libutil -I ../libop -I ../libpp  -W -Wall 
-fno-common -ftemplate-depth-50 -Werror -g -O2 -MT op_bfd.o -MD -MP -MF 
.deps/op_bfd.Tpo -c -o op_bfd.o op_bfd.cpp
cc1plus: warnings being treated as errors
op_bfd.cpp: In member function 'bool op_bfd::parse_line_sysmap(const 
std::string&, bfd_vma&, char&, std::string&)':
op_bfd.cpp:369: error: format '%Lx' expects type 'long long unsigned int*', but 
argument 3 has type 'bfd_vma*'
make[3]: *** [op_bfd.o] Error 1
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

After fixing this compile error, I was able to build oprofile OK.

Some general comments:

1. The new opcontrol option for using kallsyms must be documented in both the 
opcontrol man page and in the oprofile user manual (doc/oprofile.xml).
2. Once set, there's no way to change your new option.  So instead of 
'--use-kallsyms', perhaps it should be '--use-kallsys=[y | n]'.
3. Should it be an illegal operation to set '--vmlinux' and '--use-kallsyms' at 
the same time?  Currently, it's not.
4. I think there should be a new op_bfd constructor for the /proc/kallsyms. 
Inserting all the special case code into the existing ctor is messy.  Besides, I 
don't think there's a need for extra_images to be passed for a "/proc/kallsyms" 
op_bfd ctor.  Or am I wrong about that?
5. Please look at all of your comments and documentation and clean them up for 
grammar and clarity.


> RCS file: /cvsroot/oprofile/oprofile/ChangeLog,v
> retrieving revision 1.1918
> diff -p -a -u -r1.1918 ChangeLog
> --- oprofile/ChangeLog	26 Jan 2010 14:42:00 -0000	1.1918
> +++ oprofile/ChangeLog	29 Jan 2010 00:21:59 -0000
> @@ -1,3 +1,19 @@
> +2010-01-28  Myke Smith <myke.smith@...>
> +	* utils/opcontrol
> +	 Changes made to support using /proc/kallsyms as a source of kernel symbols.
> +	 New flag added --use-kallsyms and it requires --no-vmlinux to be active.
> +
> +	* libpp/poulate.cpp
> +	  If the file is /proc/kallsyms ignore mtime check since this file can be
> +	  	updated at any time with modules being loaded/unloaded
> +
> +	* libutils++/op_bfd.cpp
> +	  Added a constructor to handle /proc/kallsyms.
> +	  Added support routines for parsing and retrieving symbols from
> +	  	/proc/kallsyms. 
> +	  Added a boolean "sysmap" which when set short-circuits somei tests that are
> +		BFD oriented.
> +
>  2010-01-20  Maynard Johnson  <maynardj@...>
> 
>  	* m4/qt.m4: Fix qt lib check so it works on base 64-bit system
> Index: oprofile/libpp/populate.cpp
> ===================================================================
> RCS file: /cvsroot/oprofile/oprofile/libpp/populate.cpp,v
> retrieving revision 1.14
> diff -p -a -u -r1.14 populate.cpp
> --- oprofile/libpp/populate.cpp	21 Jan 2008 21:35:17 -0000	1.14
> +++ oprofile/libpp/populate.cpp	29 Jan 2010 00:21:59 -0000
> @@ -102,6 +102,9 @@ populate_for_image(profile_container & s
>  		string filename =
>  			samples.extra_found_images.find_image_path(
>  				ip.image, error, true);
> +		
> +		// don't check mtime for proc/kallsyms cause always changing with modules in & out
> +		if (filename.compare("/proc/kallsyms") != 0)		
>  		check_mtime(filename, header);
                 ^--- indentation needed
>  	}
> 
> Index: oprofile/libutil++/bfd_support.h
> ===================================================================
> RCS file: /cvsroot/oprofile/oprofile/libutil++/bfd_support.h,v
> retrieving revision 1.9
> diff -p -a -u -r1.9 bfd_support.h
> --- oprofile/libutil++/bfd_support.h	26 Oct 2009 13:01:09 -0000	1.9
> +++ oprofile/libutil++/bfd_support.h	29 Jan 2010 00:21:59 -0000
> @@ -117,6 +117,10 @@ bfd * open_bfd(std::string const & file)
>  /// open the given BFD from the fd
>  bfd * fdopen_bfd(std::string const & file, int fd);
> 
> +/// open the given BFD from -in this SPECIFIC case is a map file and not binary- the fd
> +/// different from above in that we do not do a format_check()
> +bfd * fdopen_map(std::string const & file, int fd);
> +
>  /// Return a BFD for an SPU ELF embedded in PPE binary file
>  bfd * spu_open_bfd(std::string const name, int fd, uint64_t offset_to_spu_elf);
> 
> Index: oprofile/libutil++/op_bfd.cpp
> ===================================================================
> RCS file: /cvsroot/oprofile/oprofile/libutil++/op_bfd.cpp,v
> retrieving revision 1.87
> diff -p -a -u -r1.87 op_bfd.cpp
> --- oprofile/libutil++/op_bfd.cpp	20 Jul 2009 13:24:47 -0000	1.87
> +++ oprofile/libutil++/op_bfd.cpp	29 Jan 2010 00:21:59 -0000
> @@ -24,6 +24,9 @@
>  #include <iostream>
>  #include <iomanip>
>  #include <sstream>
> +// for sysmap
> +#include <errno.h>  
> +#include <fstream> 
> 
>  #include "op_bfd.h"
>  #include "locate_images.h"
> @@ -85,6 +88,16 @@ op_bfd_symbol::op_bfd_symbol(bfd_vma vma
>  {
>  }
> 
> +// for sysmap
> +op_bfd_symbol::op_bfd_symbol(bfd_vma vma, size_t size, string const & name,
> +                             bfd_vma sectionvma, unsigned long sectionfilepos)
> +	: bfd_symbol(0), symb_value(vma),
> +	  section_filepos(sectionfilepos), section_vma(sectionvma),
> +	  symb_size(size), symb_name(name),
> +	  symb_hidden(false), symb_weak(false), symb_artificial(false)
> +{
> +}
> +
> 
>  bool op_bfd_symbol::operator<(op_bfd_symbol const & rhs) const
>  {
> @@ -104,7 +117,11 @@ op_bfd::op_bfd(string const & fname, str
>  	archive_path(extra_images.get_archive_path()),
>  	extra_found_images(extra_images),
>  	file_size(-1),
> -	anon_obj(false)
> +	anon_obj(false),
> +	sysmap(false),
> +	section_vma(0),

Compilation error --^

> +	section_filepos(0),
> +	arch_len(0)
>  {
>  	int fd;
>  	struct stat st;
> @@ -116,10 +133,16 @@ op_bfd::op_bfd(string const & fname, str
>  	string suf = ".jo";
> 
>  	image_error img_ok;
> +
> +	if ((filename.compare("/proc/kallsyms") == 0)) {
> +		sysmap = true;
> +		cverb << vbfd << "sysmap enabled by  " << filename << endl;	
> +	}
> +
>  	string const image_path =
>  		extra_images.find_image_path(filename, img_ok, true);
> 
> -	cverb << vbfd << "op_bfd ctor for " << image_path << endl;
> +	cverb << vbfd << "op_bfd ctor for " << image_path << " filename was " << filename << endl;
> 
>  	// if there's a problem already, don't try to open it
>  	if (!ok || img_ok != image_ok) {
> @@ -127,6 +150,12 @@ op_bfd::op_bfd(string const & fname, str
>  		goto out_fail;
>  	}
> 
> +	/* Since the sysmap is NOT a binary file we can't use some of the functions *
> +	 * to get and add symbols. So we will call sysmap specific versions to do   *
> +	 * work                                                                     */
> +	if (sysmap) goto sysmap_image;
> +	
> +
>  	fd = open(image_path.c_str(), O_RDONLY);
>  	if (fd == -1) {
>  		cverb << vbfd << "open failed for " << image_path << endl;
> @@ -184,6 +213,126 @@ out_fail:
>  	// make the fake symbol fit within the fake file
>  	file_size = -1;
>  	goto out;
> +	
> +sysmap_image:
> +	// First find the start and end of the text segment.
> +	bfd_vma stext = 0;
> +	bfd_vma etext = 0;
> +	
> +	ifstream in(image_path.c_str());
> +	if (!in) {
> +		cverb << vbfd << "Unable to open '" << image_path <<
> +		      "': " << strerror(errno) << endl;
> +		ok = false;
> +		goto out_fail;
> +	}
> +	string sysmap_line;
> +	while (getline(in, sysmap_line)) {
> +		bfd_vma sysmap_addr;
> +		string sysmap_name;
> +		char sysmap_type;
> +		if (!parse_line_sysmap(sysmap_line, sysmap_addr, sysmap_type, sysmap_name)) {
> +			cverb << vbfd << "Unexpected input on symbol line '" << 
> +				sysmap_line << "' in /proc/kallsyms " << 
> +				image_path << endl;
> +			ok = false;
> +			goto out_fail;
> +		}
> +		
> +		if (   (  (sysmap_name == "_text") 
> +		       || (sysmap_name == "_stext"))
> +		    && (stext == 0)) {
> +			stext = sysmap_addr;
> +			
> +		} else if (sysmap_name == "_etext")
> +			etext = sysmap_addr;
> +			
> +		// We keep parsing even if we have both _text/_stext and
> +		// _etext, just to make sure the file looks right.
> +		
> +		
> +
> +	}
> +	if (in.bad()) {
> +		cverb << vbfd << "Error on symbol line '" << sysmap_line <<
> +			"' in /proc/kallsyms " << image_path << ": " << 
> +			strerror(errno) << endl;
> +		ok = false;
> +		goto out_fail;
> +	}
> +
> +	if (stext == 0 || etext == 0) {
> +		cverb << vbfd << "Unable to find _text and _etext in " << image_path << endl;
> +		ok = false;
> +		goto out_fail;
> +	}
> +
> +	section_vma = stext;
> +
> +	// Read all of the code symbols in the text segment
> +	int number_symbols = 0;
> +	in.clear();
> +	in.seekg(0, ios::beg);
> +	string prevname;
> +	bfd_vma prevaddr = 0;
> +	while (getline(in, sysmap_line)) {
> +		bfd_vma sysmap_addr;
> +		string sysmap_name;
> +		char sysmap_type;
> +		if (!parse_line_sysmap(sysmap_line, sysmap_addr, sysmap_type, sysmap_name)) {
> +			cverb << vbfd << "Unexpected input on symbol line '" << 
> +				sysmap_line << "' in " << image_path << endl;
> +			ok = false;
> +			goto out_fail;
> +		}
> +		if (sysmap_addr < stext)
> +			continue;
> +		if (sysmap_addr > etext)
> +			break;
> +
> +		// Can have multiple symbols of different types at same VMA,
> +		// we only want to complete a code symbol if we've moved
> +		// on to the next VMA.  If there are multiple code symbols
> +		// at the same VMA, we use the last symbol.
> +		
> +		// This covers the boring_symbols routine for BFD images
> +		if (prevaddr != 0 && prevaddr != sysmap_addr) {
> +		
> +			add_sym_sysmap(symbol_filter, prevname, prevaddr, sysmap_addr);
> +			prevname.clear();
> +			prevaddr = 0;
> +			number_symbols++;
> +		}
> +
> +		if (sysmap_type == 'T' || sysmap_type == 't') {
> +			// it's code
> +			// but we want to remove uninteresting symbols
> +			// ARM assembler internal mapping symbols
> +			// C++ exception stuff
> +			
> +			// taken from the routine interesting_symbols for BFD images
> +			if  (   (sysmap_name != "$a") 
> +			     && (sysmap_name != "$t") 
> +			     && (sysmap_name != "$d")
> +			     && (sysmap_name != ".L")) {
> +			       
> +				prevname = sysmap_name;
> +				prevaddr = sysmap_addr;
> +			}
> +		} 
> +	}
> +	
> +	cverb << vbfd << "stext:" << hex << stext << " etext:" << hex << etext << " added " 
> +	      << dec << number_symbols << " symbols from " << image_path << endl;
> +
> +
> +	arch_len = 64;
> +	if (stext < 0xffffffff)
> +		arch_len = 32;
> +	
> +	ok = true;
> +
> +	return;
>  }
> 
> 
> @@ -194,16 +343,53 @@ op_bfd::~op_bfd()
> 
>  unsigned long op_bfd::get_start_offset(bfd_vma vma) const
>  {
> +	if (sysmap) {
> +		if (!vma || vma == section_vma)
> +			return section_filepos;
> +		return 0;	
> +	} else {
>  	if (!vma || !ibfd.valid()) {
>  		filepos_map_t::const_iterator it = filepos_map.find(".text");
>  		if (it != filepos_map.end())
>  			return it->second;
>  		return 0;
>  	}

Please indent this under the new "else" clause.

> +	}
> 
>  	return 0;
>  }
> 
> +//  sysmap specific function
> +bool op_bfd::parse_line_sysmap(string const & line, bfd_vma & addr, char & type, string & name)
> +{
> +	char * sym = NULL;
> +	char * end = NULL;
> +	int c = -1;
> +#ifdef BFD64
> +	c = sscanf(line.c_str(), "%Lx %c %as %as", &addr, &type, &sym, &end);

Compilation error . . . . . . . --^

> +#else
> +	c = sscanf(line.c_str(), "%lx %c %as %as", &addr, &type, &sym, &end);
> +#endif
> +	if (c < 3) {
> +		if (sym)
> +			free(sym);
> +		return false;
> +	}
> +	name = sym;
> +	free(sym);
> +	return true;
> +}
> +
> +//  sysmap specific function
> +//  the ctor sets all the sizes and several BFD type of symbols
> +void op_bfd::add_sym_sysmap(string_filter const & symbol_filter,
> +			string const & name, bfd_vma start, bfd_vma end)

                                        ^-- maybe change to 'previous_name'?

> +{
> +	if (symbol_filter.match(name)) {
> +		syms.push_back(op_bfd_symbol(start-section_vma, end-start, name,
> +                               section_vma, section_filepos));
> +	}
> +}
> 
>  void op_bfd::get_symbols(op_bfd::symbols_found_t & symbols)
>  {
[snip]

Thank you for your comments.  The biggest issue might be contacting  
Brian. I emailed him a few months ago at the email in his original  
patches and got no replies.
If you have newer contact information I would appreciate it being  
passed on to me.

I will start working on the issues you listed.

> 1. The new opcontrol option for using kallsyms must be documented in  
> both the
> opcontrol man page and in the oprofile user manual (doc/oprofile.xml).

Is this normally done through a patch file as well? Do you want  
everything in one patch file?

thanks,

-myke

On Feb 9, 2010, at 1:57 PM, Maynard Johnson wrote:

> Myke Smith wrote:
>> Found and fixed a bug in opcontrol.
>>
>> This occurred if you chose not to identify any kernel symbols, did  
>> not
>> set --use-kallsyms. This caused a hang in opcontrol get_image().
>>
>> The attached patch has everything in the previous patch plus this  
>> fix.
>> Not sure if this is the appropriate way or if you want a patch to  
>> just
>> opcontrol. If you just want a patch to opcontrol let me know and I  
>> will
>> make it.
>>
>>
>>
>>
>> Signed-off-by: Myke Smith <myke.smith@...>
>>
>>
>> On Jan 28, 2010, at 10:44 AM, Myke Smith wrote:
>
> I apologize for the delay in reviewing this patch.  Other priorities  
> got in the
> way.  Thanks for the submission.  Comments embedded below.
>
> -Maynard
>
>>
>>>
>>> On hand held devices we are -typically- very memory constrained and
>>> as such the standard vmlinux image is not available. The attached
>>> patch re-uses and re-implements some of the ideas in Brian Eaton's
>>> patch from the summer of 2006.
>
> Adding the "Signed-off-by" line below, implies you have read and are  
> complying
> with the "Developer's Certificate of Origin 1.1" (DCO) from
> http://lwn.net/Articles/139918/.  Since by your own admission, this  
> patch is
> based on an original patch from Brian Eaton, you need to get Brian  
> to sign off
> on this patch.
>
>>>
>>> Specifically it adds a flag to opcontrol --use-kallsyms which in
>>> combination with --vm-nolinux  passes /proc/kallsyms to oprofiled.
> I presume you mean "--no-vmlinux".
>>>
>>> There are modifications to libutil++/op_bfd.* to detect the use of
>>> /proc/kallsyms and complementary routines to the standard BFD based
>>> routines to handle the text versus binary file.
>>>
>>>
>>>
>>>
>>> Signed-off-by: Myke Smith <myke.smith@...>
>>>
>> Index: oprofile/ChangeLog
>> ===================================================================
>
> Your patch does not compile for me.  I get the following error:
>
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> +++++
> make[3]: Entering directory `/home/mpj/oprof-work/op-tmp/libutil++'
> if g++ -DHAVE_CONFIG_H -I. -I. -I..  -I ../libutil -I ../libop -I ../ 
> libpp  -W
> -Wall -fno-common -ftemplate-depth-50 -Werror -g -O2 -MT op_bfd.o - 
> MD -MP -MF
> ".deps/op_bfd.Tpo" -c -o op_bfd.o op_bfd.cpp; \
> then mv -f ".deps/op_bfd.Tpo" ".deps/op_bfd.Po"; else rm -f ".deps/ 
> op_bfd.Tpo";
> exit 1; fi
> cc1plus: warnings being treated as errors
> op_bfd.h: In constructor ?op_bfd::op_bfd(const std::string&, const
> string_filter&, const extra_images&, bool&)?:
> op_bfd.h:332: warning: ?op_bfd::section_vma? will be initialized after
> op_bfd.h:329: warning:   ?long unsigned int op_bfd::section_filepos?
> op_bfd.cpp:114: warning:   when initialized here
> make[3]: *** [op_bfd.o] Error 1
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> +++++
>
> Member initializers are executed in the order in which they are  
> found in the
> class defintiion.  The warning message is telling you that the order  
> in which
> you coded the member initializers in the class constructor differs  
> from that
> found in the class definition.  If you patched and built this with  
> an oprofile
> cvs checkout, the warning should have caused a compile error for  
> you.  Maybe
> your C++ compiler just isn't catching this.
>
> After fixing the above compile error, I ran into another one.  This  
> second error
> was only failing on a more recent g++ version (4.3):
>
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> +++++
> make[3]: Entering directory `/home/mpj/temp/oprofile/libutil++'
> g++ -DHAVE_CONFIG_H -I. -I..  -I ../libutil -I ../libop -I ../libpp   
> -W -Wall
> -fno-common -ftemplate-depth-50 -Werror -g -O2 -MT op_bfd.o -MD -MP - 
> MF
> .deps/op_bfd.Tpo -c -o op_bfd.o op_bfd.cpp
> cc1plus: warnings being treated as errors
> op_bfd.cpp: In member function 'bool op_bfd::parse_line_sysmap(const
> std::string&, bfd_vma&, char&, std::string&)':
> op_bfd.cpp:369: error: format '%Lx' expects type 'long long unsigned  
> int*', but
> argument 3 has type 'bfd_vma*'
> make[3]: *** [op_bfd.o] Error 1
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> +++++
>
> After fixing this compile error, I was able to build oprofile OK.
>
> Some general comments:
>
> 1. The new opcontrol option for using kallsyms must be documented in  
> both the
> opcontrol man page and in the oprofile user manual (doc/oprofile.xml).
> 2. Once set, there's no way to change your new option.  So instead of
> '--use-kallsyms', perhaps it should be '--use-kallsys=[y | n]'.
> 3. Should it be an illegal operation to set '--vmlinux' and '--use- 
> kallsyms' at
> the same time?  Currently, it's not.
> 4. I think there should be a new op_bfd constructor for the /proc/ 
> kallsyms.
> Inserting all the special case code into the existing ctor is  
> messy.  Besides, I
> don't think there's a need for extra_images to be passed for a "/ 
> proc/kallsyms"
> op_bfd ctor.  Or am I wrong about that?
> 5. Please look at all of your comments and documentation and clean  
> them up for
> grammar and clarity.
>
>
>> RCS file: /cvsroot/oprofile/oprofile/ChangeLog,v
>> retrieving revision 1.1918
>> diff -p -a -u -r1.1918 ChangeLog
>> --- oprofile/ChangeLog        26 Jan 2010 14:42:00 -0000      1.1918
>> +++ oprofile/ChangeLog        29 Jan 2010 00:21:59 -0000
>> @@ -1,3 +1,19 @@
>> +2010-01-28  Myke Smith <myke.smith@...>
>> +     * utils/opcontrol
>> +      Changes made to support using /proc/kallsyms as a source of  
>> kernel symbols.
>> +      New flag added --use-kallsyms and it requires --no-vmlinux  
>> to be active.
>> +
>> +     * libpp/poulate.cpp
>> +       If the file is /proc/kallsyms ignore mtime check since this  
>> file can be
>> +             updated at any time with modules being loaded/unloaded
>> +
>> +     * libutils++/op_bfd.cpp
>> +       Added a constructor to handle /proc/kallsyms.
>> +       Added support routines for parsing and retrieving symbols  
>> from
>> +             /proc/kallsyms.
>> +       Added a boolean "sysmap" which when set short-circuits  
>> somei tests that are
>> +             BFD oriented.
>> +
>> 2010-01-20  Maynard Johnson  <maynardj@...>
>>
>>      * m4/qt.m4: Fix qt lib check so it works on base 64-bit system
>> Index: oprofile/libpp/populate.cpp
>> ===================================================================
>> RCS file: /cvsroot/oprofile/oprofile/libpp/populate.cpp,v
>> retrieving revision 1.14
>> diff -p -a -u -r1.14 populate.cpp
>> --- oprofile/libpp/populate.cpp       21 Jan 2008 21:35:17  
>> -0000      1.14
>> +++ oprofile/libpp/populate.cpp       29 Jan 2010 00:21:59 -0000
>> @@ -102,6 +102,9 @@ populate_for_image(profile_container & s
>>              string filename =
>>                      samples.extra_found_images.find_image_path(
>>                              ip.image, error, true);
>> +
>> +             // don't check mtime for proc/kallsyms cause always  
>> changing with modules in & out
>> +             if (filename.compare("/proc/kallsyms") != 0)
>>              check_mtime(filename, header);
>                 ^--- indentation needed
>>      }
>>
>> Index: oprofile/libutil++/bfd_support.h
>> ===================================================================
>> RCS file: /cvsroot/oprofile/oprofile/libutil++/bfd_support.h,v
>> retrieving revision 1.9
>> diff -p -a -u -r1.9 bfd_support.h
>> --- oprofile/libutil++/bfd_support.h  26 Oct 2009 13:01:09  
>> -0000      1.9
>> +++ oprofile/libutil++/bfd_support.h  29 Jan 2010 00:21:59 -0000
>> @@ -117,6 +117,10 @@ bfd * open_bfd(std::string const & file)
>> /// open the given BFD from the fd
>> bfd * fdopen_bfd(std::string const & file, int fd);
>>
>> +/// open the given BFD from -in this SPECIFIC case is a map file  
>> and not binary- the fd
>> +/// different from above in that we do not do a format_check()
>> +bfd * fdopen_map(std::string const & file, int fd);
>> +
>> /// Return a BFD for an SPU ELF embedded in PPE binary file
>> bfd * spu_open_bfd(std::string const name, int fd, uint64_t  
>> offset_to_spu_elf);
>>
>> Index: oprofile/libutil++/op_bfd.cpp
>> ===================================================================
>> RCS file: /cvsroot/oprofile/oprofile/libutil++/op_bfd.cpp,v
>> retrieving revision 1.87
>> diff -p -a -u -r1.87 op_bfd.cpp
>> --- oprofile/libutil++/op_bfd.cpp     20 Jul 2009 13:24:47  
>> -0000      1.87
>> +++ oprofile/libutil++/op_bfd.cpp     29 Jan 2010 00:21:59 -0000
>> @@ -24,6 +24,9 @@
>> #include <iostream>
>> #include <iomanip>
>> #include <sstream>
>> +// for sysmap
>> +#include <errno.h>
>> +#include <fstream>
>>
>> #include "op_bfd.h"
>> #include "locate_images.h"
>> @@ -85,6 +88,16 @@ op_bfd_symbol::op_bfd_symbol(bfd_vma vma
>> {
>> }
>>
>> +// for sysmap
>> +op_bfd_symbol::op_bfd_symbol(bfd_vma vma, size_t size, string  
>> const & name,
>> +                             bfd_vma sectionvma, unsigned long  
>> sectionfilepos)
>> +     : bfd_symbol(0), symb_value(vma),
>> +       section_filepos(sectionfilepos), section_vma(sectionvma),
>> +       symb_size(size), symb_name(name),
>> +       symb_hidden(false), symb_weak(false), symb_artificial(false)
>> +{
>> +}
>> +
>>
>> bool op_bfd_symbol::operator<(op_bfd_symbol const & rhs) const
>> {
>> @@ -104,7 +117,11 @@ op_bfd::op_bfd(string const & fname, str
>>      archive_path(extra_images.get_archive_path()),
>>      extra_found_images(extra_images),
>>      file_size(-1),
>> -     anon_obj(false)
>> +     anon_obj(false),
>> +     sysmap(false),
>> +     section_vma(0),
>
> Compilation error --^
>
>> +     section_filepos(0),
>> +     arch_len(0)
>> {
>>      int fd;
>>      struct stat st;
>> @@ -116,10 +133,16 @@ op_bfd::op_bfd(string const & fname, str
>>      string suf = ".jo";
>>
>>      image_error img_ok;
>> +
>> +     if ((filename.compare("/proc/kallsyms") == 0)) {
>> +             sysmap = true;
>> +             cverb << vbfd << "sysmap enabled by  " << filename <<  
>> endl;
>> +     }
>> +
>>      string const image_path =
>>              extra_images.find_image_path(filename, img_ok, true);
>>
>> -     cverb << vbfd << "op_bfd ctor for " << image_path << endl;
>> +     cverb << vbfd << "op_bfd ctor for " << image_path << "  
>> filename was " << filename << endl;
>>
>>      // if there's a problem already, don't try to open it
>>      if (!ok || img_ok != image_ok) {
>> @@ -127,6 +150,12 @@ op_bfd::op_bfd(string const & fname, str
>>              goto out_fail;
>>      }
>>
>> +     /* Since the sysmap is NOT a binary file we can't use some of  
>> the functions *
>> +      * to get and add symbols. So we will call sysmap specific  
>> versions to do   *
>> +      *  
>> work 
>>                                                                      */
>> +     if (sysmap) goto sysmap_image;
>> +
>> +
>>      fd = open(image_path.c_str(), O_RDONLY);
>>      if (fd == -1) {
>>              cverb << vbfd << "open failed for " << image_path <<  
>> endl;
>> @@ -184,6 +213,126 @@ out_fail:
>>      // make the fake symbol fit within the fake file
>>      file_size = -1;
>>      goto out;
>> +
>> +sysmap_image:
>> +     // First find the start and end of the text segment.
>> +     bfd_vma stext = 0;
>> +     bfd_vma etext = 0;
>> +
>> +     ifstream in(image_path.c_str());
>> +     if (!in) {
>> +             cverb << vbfd << "Unable to open '" << image_path <<
>> +                   "': " << strerror(errno) << endl;
>> +             ok = false;
>> +             goto out_fail;
>> +     }
>> +     string sysmap_line;
>> +     while (getline(in, sysmap_line)) {
>> +             bfd_vma sysmap_addr;
>> +             string sysmap_name;
>> +             char sysmap_type;
>> +             if (!parse_line_sysmap(sysmap_line, sysmap_addr,  
>> sysmap_type, sysmap_name)) {
>> +                     cverb << vbfd << "Unexpected input on symbol  
>> line '" <<
>> +                             sysmap_line << "' in /proc/kallsyms "  
>> <<
>> +                             image_path << endl;
>> +                     ok = false;
>> +                     goto out_fail;
>> +             }
>> +
>> +             if (   (  (sysmap_name == "_text")
>> +                    || (sysmap_name == "_stext"))
>> +                 && (stext == 0)) {
>> +                     stext = sysmap_addr;
>> +
>> +             } else if (sysmap_name == "_etext")
>> +                     etext = sysmap_addr;
>> +
>> +             // We keep parsing even if we have both _text/_stext  
>> and
>> +             // _etext, just to make sure the file looks right.
>> +
>> +
>> +
>> +     }
>> +     if (in.bad()) {
>> +             cverb << vbfd << "Error on symbol line '" <<  
>> sysmap_line <<
>> +                     "' in /proc/kallsyms " << image_path << ": " <<
>> +                     strerror(errno) << endl;
>> +             ok = false;
>> +             goto out_fail;
>> +     }
>> +
>> +     if (stext == 0 || etext == 0) {
>> +             cverb << vbfd << "Unable to find _text and _etext in  
>> " << image_path << endl;
>> +             ok = false;
>> +             goto out_fail;
>> +     }
>> +
>> +     section_vma = stext;
>> +
>> +     // Read all of the code symbols in the text segment
>> +     int number_symbols = 0;
>> +     in.clear();
>> +     in.seekg(0, ios::beg);
>> +     string prevname;
>> +     bfd_vma prevaddr = 0;
>> +     while (getline(in, sysmap_line)) {
>> +             bfd_vma sysmap_addr;
>> +             string sysmap_name;
>> +             char sysmap_type;
>> +             if (!parse_line_sysmap(sysmap_line, sysmap_addr,  
>> sysmap_type, sysmap_name)) {
>> +                     cverb << vbfd << "Unexpected input on symbol  
>> line '" <<
>> +                             sysmap_line << "' in " << image_path  
>> << endl;
>> +                     ok = false;
>> +                     goto out_fail;
>> +             }
>> +             if (sysmap_addr < stext)
>> +                     continue;
>> +             if (sysmap_addr > etext)
>> +                     break;
>> +
>> +             // Can have multiple symbols of different types at  
>> same VMA,
>> +             // we only want to complete a code symbol if we've  
>> moved
>> +             // on to the next VMA.  If there are multiple code  
>> symbols
>> +             // at the same VMA, we use the last symbol.
>> +
>> +             // This covers the boring_symbols routine for BFD  
>> images
>> +             if (prevaddr != 0 && prevaddr != sysmap_addr) {
>> +
>> +                     add_sym_sysmap(symbol_filter, prevname,  
>> prevaddr, sysmap_addr);
>> +                     prevname.clear();
>> +                     prevaddr = 0;
>> +                     number_symbols++;
>> +             }
>> +
>> +             if (sysmap_type == 'T' || sysmap_type == 't') {
>> +                     // it's code
>> +                     // but we want to remove uninteresting symbols
>> +                     // ARM assembler internal mapping symbols
>> +                     // C++ exception stuff
>> +
>> +                     // taken from the routine interesting_symbols  
>> for BFD images
>> +                     if  (   (sysmap_name != "$a")
>> +                          && (sysmap_name != "$t")
>> +                          && (sysmap_name != "$d")
>> +                          && (sysmap_name != ".L")) {
>> +
>> +                             prevname = sysmap_name;
>> +                             prevaddr = sysmap_addr;
>> +                     }
>> +             }
>> +     }
>> +
>> +     cverb << vbfd << "stext:" << hex << stext << " etext:" << hex  
>> << etext << " added "
>> +           << dec << number_symbols << " symbols from " <<  
>> image_path << endl;
>> +
>> +
>> +     arch_len = 64;
>> +     if (stext < 0xffffffff)
>> +             arch_len = 32;
>> +
>> +     ok = true;
>> +
>> +     return;
>> }
>>
>>
>> @@ -194,16 +343,53 @@ op_bfd::~op_bfd()
>>
>> unsigned long op_bfd::get_start_offset(bfd_vma vma) const
>> {
>> +     if (sysmap) {
>> +             if (!vma || vma == section_vma)
>> +                     return section_filepos;
>> +             return 0;
>> +     } else {
>>      if (!vma || !ibfd.valid()) {
>>              filepos_map_t::const_iterator it =  
>> filepos_map.find(".text");
>>              if (it != filepos_map.end())
>>                      return it->second;
>>              return 0;
>>      }
>
> Please indent this under the new "else" clause.
>
>> +     }
>>
>>      return 0;
>> }
>>
>> +//  sysmap specific function
>> +bool op_bfd::parse_line_sysmap(string const & line, bfd_vma &  
>> addr, char & type, string & name)
>> +{
>> +     char * sym = NULL;
>> +     char * end = NULL;
>> +     int c = -1;
>> +#ifdef BFD64
>> +     c = sscanf(line.c_str(), "%Lx %c %as %as", &addr, &type,  
>> &sym, &end);
>
> Compilation error . . . . . . . --^
>
>> +#else
>> +     c = sscanf(line.c_str(), "%lx %c %as %as", &addr, &type,  
>> &sym, &end);
>> +#endif
>> +     if (c < 3) {
>> +             if (sym)
>> +                     free(sym);
>> +             return false;
>> +     }
>> +     name = sym;
>> +     free(sym);
>> +     return true;
>> +}
>> +
>> +//  sysmap specific function
>> +//  the ctor sets all the sizes and several BFD type of symbols
>> +void op_bfd::add_sym_sysmap(string_filter const & symbol_filter,
>> +                     string const & name, bfd_vma start, bfd_vma  
>> end)
>
>                                        ^-- maybe change to  
> 'previous_name'?
>
>> +{
>> +     if (symbol_filter.match(name)) {
>> +             syms.push_back(op_bfd_symbol(start-section_vma, end- 
>> start, name,
>> +                               section_vma, section_filepos));
>> +     }
>> +}
>>
>> void op_bfd::get_symbols(op_bfd::symbols_found_t & symbols)
>> {
> [snip]
>

With help from Vitali I contacted Brian and he said he was happy to  
sign off. I assume you need an email directly from Brian with the sign- 
off statement when I am done. Does he need to also send in a copy of  
the patch or is it sufficient that he email the "signature" in  
pointing to the specific patch?

-myke

On Feb 9, 2010, at 1:57 PM, Maynard Johnson wrote:

> Myke Smith wrote:
>> Found and fixed a bug in opcontrol.
>>
>> This occurred if you chose not to identify any kernel symbols, did  
>> not
>> set --use-kallsyms. This caused a hang in opcontrol get_image().
>>
>> The attached patch has everything in the previous patch plus this  
>> fix.
>> Not sure if this is the appropriate way or if you want a patch to  
>> just
>> opcontrol. If you just want a patch to opcontrol let me know and I  
>> will
>> make it.
>>
>>
>>
>>
>> Signed-off-by: Myke Smith <myke.smith@...>
>>
>>
>> On Jan 28, 2010, at 10:44 AM, Myke Smith wrote:
>
> I apologize for the delay in reviewing this patch.  Other priorities  
> got in the
> way.  Thanks for the submission.  Comments embedded below.
>
> -Maynard
>
>>
>>>
>>> On hand held devices we are -typically- very memory constrained and
>>> as such the standard vmlinux image is not available. The attached
>>> patch re-uses and re-implements some of the ideas in Brian Eaton's
>>> patch from the summer of 2006.
>
> Adding the "Signed-off-by" line below, implies you have read and are  
> complying
> with the "Developer's Certificate of Origin 1.1" (DCO) from
> http://lwn.net/Articles/139918/.  Since by your own admission, this  
> patch is
> based on an original patch from Brian Eaton, you need to get Brian  
> to sign off
> on this patch.
>
>>>
>>> Specifically it adds a flag to opcontrol --use-kallsyms which in
>>> combination with --vm-nolinux  passes /proc/kallsyms to oprofiled.
> I presume you mean "--no-vmlinux".
>>>
>>> There are modifications to libutil++/op_bfd.* to detect the use of
>>> /proc/kallsyms and complementary routines to the standard BFD based
>>> routines to handle the text versus binary file.
>>>
>>>
>>>
>>>
>>> Signed-off-by: Myke Smith <myke.smith@...>
>>>
>> Index: oprofile/ChangeLog
>> ===================================================================
>
> Your patch does not compile for me.  I get the following error:
>
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> +++++
> make[3]: Entering directory `/home/mpj/oprof-work/op-tmp/libutil++'
> if g++ -DHAVE_CONFIG_H -I. -I. -I..  -I ../libutil -I ../libop -I ../ 
> libpp  -W
> -Wall -fno-common -ftemplate-depth-50 -Werror -g -O2 -MT op_bfd.o - 
> MD -MP -MF
> ".deps/op_bfd.Tpo" -c -o op_bfd.o op_bfd.cpp; \
> then mv -f ".deps/op_bfd.Tpo" ".deps/op_bfd.Po"; else rm -f ".deps/ 
> op_bfd.Tpo";
> exit 1; fi
> cc1plus: warnings being treated as errors
> op_bfd.h: In constructor ?op_bfd::op_bfd(const std::string&, const
> string_filter&, const extra_images&, bool&)?:
> op_bfd.h:332: warning: ?op_bfd::section_vma? will be initialized after
> op_bfd.h:329: warning:   ?long unsigned int op_bfd::section_filepos?
> op_bfd.cpp:114: warning:   when initialized here
> make[3]: *** [op_bfd.o] Error 1
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> +++++
>
> Member initializers are executed in the order in which they are  
> found in the
> class defintiion.  The warning message is telling you that the order  
> in which
> you coded the member initializers in the class constructor differs  
> from that
> found in the class definition.  If you patched and built this with  
> an oprofile
> cvs checkout, the warning should have caused a compile error for  
> you.  Maybe
> your C++ compiler just isn't catching this.
>
> After fixing the above compile error, I ran into another one.  This  
> second error
> was only failing on a more recent g++ version (4.3):
>
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> +++++
> make[3]: Entering directory `/home/mpj/temp/oprofile/libutil++'
> g++ -DHAVE_CONFIG_H -I. -I..  -I ../libutil -I ../libop -I ../libpp   
> -W -Wall
> -fno-common -ftemplate-depth-50 -Werror -g -O2 -MT op_bfd.o -MD -MP - 
> MF
> .deps/op_bfd.Tpo -c -o op_bfd.o op_bfd.cpp
> cc1plus: warnings being treated as errors
> op_bfd.cpp: In member function 'bool op_bfd::parse_line_sysmap(const
> std::string&, bfd_vma&, char&, std::string&)':
> op_bfd.cpp:369: error: format '%Lx' expects type 'long long unsigned  
> int*', but
> argument 3 has type 'bfd_vma*'
> make[3]: *** [op_bfd.o] Error 1
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> +++++
>
> After fixing this compile error, I was able to build oprofile OK.
>
> Some general comments:
>
> 1. The new opcontrol option for using kallsyms must be documented in  
> both the
> opcontrol man page and in the oprofile user manual (doc/oprofile.xml).
> 2. Once set, there's no way to change your new option.  So instead of
> '--use-kallsyms', perhaps it should be '--use-kallsys=[y | n]'.
> 3. Should it be an illegal operation to set '--vmlinux' and '--use- 
> kallsyms' at
> the same time?  Currently, it's not.
> 4. I think there should be a new op_bfd constructor for the /proc/ 
> kallsyms.
> Inserting all the special case code into the existing ctor is  
> messy.  Besides, I
> don't think there's a need for extra_images to be passed for a "/ 
> proc/kallsyms"
> op_bfd ctor.  Or am I wrong about that?
> 5. Please look at all of your comments and documentation and clean  
> them up for
> grammar and clarity.
>
>
>> RCS file: /cvsroot/oprofile/oprofile/ChangeLog,v
>> retrieving revision 1.1918
>> diff -p -a -u -r1.1918 ChangeLog
>> --- oprofile/ChangeLog        26 Jan 2010 14:42:00 -0000      1.1918
>> +++ oprofile/ChangeLog        29 Jan 2010 00:21:59 -0000
>> @@ -1,3 +1,19 @@
>> +2010-01-28  Myke Smith <myke.smith@...>
>> +     * utils/opcontrol
>> +      Changes made to support using /proc/kallsyms as a source of  
>> kernel symbols.
>> +      New flag added --use-kallsyms and it requires --no-vmlinux  
>> to be active.
>> +
>> +     * libpp/poulate.cpp
>> +       If the file is /proc/kallsyms ignore mtime check since this  
>> file can be
>> +             updated at any time with modules being loaded/unloaded
>> +
>> +     * libutils++/op_bfd.cpp
>> +       Added a constructor to handle /proc/kallsyms.
>> +       Added support routines for parsing and retrieving symbols  
>> from
>> +             /proc/kallsyms.
>> +       Added a boolean "sysmap" which when set short-circuits  
>> somei tests that are
>> +             BFD oriented.
>> +
>> 2010-01-20  Maynard Johnson  <maynardj@...>
>>
>>      * m4/qt.m4: Fix qt lib check so it works on base 64-bit system
>> Index: oprofile/libpp/populate.cpp
>> ===================================================================
>> RCS file: /cvsroot/oprofile/oprofile/libpp/populate.cpp,v
>> retrieving revision 1.14
>> diff -p -a -u -r1.14 populate.cpp
>> --- oprofile/libpp/populate.cpp       21 Jan 2008 21:35:17  
>> -0000      1.14
>> +++ oprofile/libpp/populate.cpp       29 Jan 2010 00:21:59 -0000
>> @@ -102,6 +102,9 @@ populate_for_image(profile_container & s
>>              string filename =
>>                      samples.extra_found_images.find_image_path(
>>                              ip.image, error, true);
>> +
>> +             // don't check mtime for proc/kallsyms cause always  
>> changing with modules in & out
>> +             if (filename.compare("/proc/kallsyms") != 0)
>>              check_mtime(filename, header);
>                 ^--- indentation needed
>>      }
>>
>> Index: oprofile/libutil++/bfd_support.h
>> ===================================================================
>> RCS file: /cvsroot/oprofile/oprofile/libutil++/bfd_support.h,v
>> retrieving revision 1.9
>> diff -p -a -u -r1.9 bfd_support.h
>> --- oprofile/libutil++/bfd_support.h  26 Oct 2009 13:01:09  
>> -0000      1.9
>> +++ oprofile/libutil++/bfd_support.h  29 Jan 2010 00:21:59 -0000
>> @@ -117,6 +117,10 @@ bfd * open_bfd(std::string const & file)
>> /// open the given BFD from the fd
>> bfd * fdopen_bfd(std::string const & file, int fd);
>>
>> +/// open the given BFD from -in this SPECIFIC case is a map file  
>> and not binary- the fd
>> +/// different from above in that we do not do a format_check()
>> +bfd * fdopen_map(std::string const & file, int fd);
>> +
>> /// Return a BFD for an SPU ELF embedded in PPE binary file
>> bfd * spu_open_bfd(std::string const name, int fd, uint64_t  
>> offset_to_spu_elf);
>>
>> Index: oprofile/libutil++/op_bfd.cpp
>> ===================================================================
>> RCS file: /cvsroot/oprofile/oprofile/libutil++/op_bfd.cpp,v
>> retrieving revision 1.87
>> diff -p -a -u -r1.87 op_bfd.cpp
>> --- oprofile/libutil++/op_bfd.cpp     20 Jul 2009 13:24:47  
>> -0000      1.87
>> +++ oprofile/libutil++/op_bfd.cpp     29 Jan 2010 00:21:59 -0000
>> @@ -24,6 +24,9 @@
>> #include <iostream>
>> #include <iomanip>
>> #include <sstream>
>> +// for sysmap
>> +#include <errno.h>
>> +#include <fstream>
>>
>> #include "op_bfd.h"
>> #include "locate_images.h"
>> @@ -85,6 +88,16 @@ op_bfd_symbol::op_bfd_symbol(bfd_vma vma
>> {
>> }
>>
>> +// for sysmap
>> +op_bfd_symbol::op_bfd_symbol(bfd_vma vma, size_t size, string  
>> const & name,
>> +                             bfd_vma sectionvma, unsigned long  
>> sectionfilepos)
>> +     : bfd_symbol(0), symb_value(vma),
>> +       section_filepos(sectionfilepos), section_vma(sectionvma),
>> +       symb_size(size), symb_name(name),
>> +       symb_hidden(false), symb_weak(false), symb_artificial(false)
>> +{
>> +}
>> +
>>
>> bool op_bfd_symbol::operator<(op_bfd_symbol const & rhs) const
>> {
>> @@ -104,7 +117,11 @@ op_bfd::op_bfd(string const & fname, str
>>      archive_path(extra_images.get_archive_path()),
>>      extra_found_images(extra_images),
>>      file_size(-1),
>> -     anon_obj(false)
>> +     anon_obj(false),
>> +     sysmap(false),
>> +     section_vma(0),
>
> Compilation error --^
>
>> +     section_filepos(0),
>> +     arch_len(0)
>> {
>>      int fd;
>>      struct stat st;
>> @@ -116,10 +133,16 @@ op_bfd::op_bfd(string const & fname, str
>>      string suf = ".jo";
>>
>>      image_error img_ok;
>> +
>> +     if ((filename.compare("/proc/kallsyms") == 0)) {
>> +             sysmap = true;
>> +             cverb << vbfd << "sysmap enabled by  " << filename <<  
>> endl;
>> +     }
>> +
>>      string const image_path =
>>              extra_images.find_image_path(filename, img_ok, true);
>>
>> -     cverb << vbfd << "op_bfd ctor for " << image_path << endl;
>> +     cverb << vbfd << "op_bfd ctor for " << image_path << "  
>> filename was " << filename << endl;
>>
>>      // if there's a problem already, don't try to open it
>>      if (!ok || img_ok != image_ok) {
>> @@ -127,6 +150,12 @@ op_bfd::op_bfd(string const & fname, str
>>              goto out_fail;
>>      }
>>
>> +     /* Since the sysmap is NOT a binary file we can't use some of  
>> the functions *
>> +      * to get and add symbols. So we will call sysmap specific  
>> versions to do   *
>> +      *  
>> work 
>>                                                                      */
>> +     if (sysmap) goto sysmap_image;
>> +
>> +
>>      fd = open(image_path.c_str(), O_RDONLY);
>>      if (fd == -1) {
>>              cverb << vbfd << "open failed for " << image_path <<  
>> endl;
>> @@ -184,6 +213,126 @@ out_fail:
>>      // make the fake symbol fit within the fake file
>>      file_size = -1;
>>      goto out;
>> +
>> +sysmap_image:
>> +     // First find the start and end of the text segment.
>> +     bfd_vma stext = 0;
>> +     bfd_vma etext = 0;
>> +
>> +     ifstream in(image_path.c_str());
>> +     if (!in) {
>> +             cverb << vbfd << "Unable to open '" << image_path <<
>> +                   "': " << strerror(errno) << endl;
>> +             ok = false;
>> +             goto out_fail;
>> +     }
>> +     string sysmap_line;
>> +     while (getline(in, sysmap_line)) {
>> +             bfd_vma sysmap_addr;
>> +             string sysmap_name;
>> +             char sysmap_type;
>> +             if (!parse_line_sysmap(sysmap_line, sysmap_addr,  
>> sysmap_type, sysmap_name)) {
>> +                     cverb << vbfd << "Unexpected input on symbol  
>> line '" <<
>> +                             sysmap_line << "' in /proc/kallsyms "  
>> <<
>> +                             image_path << endl;
>> +                     ok = false;
>> +                     goto out_fail;
>> +             }
>> +
>> +             if (   (  (sysmap_name == "_text")
>> +                    || (sysmap_name == "_stext"))
>> +                 && (stext == 0)) {
>> +                     stext = sysmap_addr;
>> +
>> +             } else if (sysmap_name == "_etext")
>> +                     etext = sysmap_addr;
>> +
>> +             // We keep parsing even if we have both _text/_stext  
>> and
>> +             // _etext, just to make sure the file looks right.
>> +
>> +
>> +
>> +     }
>> +     if (in.bad()) {
>> +             cverb << vbfd << "Error on symbol line '" <<  
>> sysmap_line <<
>> +                     "' in /proc/kallsyms " << image_path << ": " <<
>> +                     strerror(errno) << endl;
>> +             ok = false;
>> +             goto out_fail;
>> +     }
>> +
>> +     if (stext == 0 || etext == 0) {
>> +             cverb << vbfd << "Unable to find _text and _etext in  
>> " << image_path << endl;
>> +             ok = false;
>> +             goto out_fail;
>> +     }
>> +
>> +     section_vma = stext;
>> +
>> +     // Read all of the code symbols in the text segment
>> +     int number_symbols = 0;
>> +     in.clear();
>> +     in.seekg(0, ios::beg);
>> +     string prevname;
>> +     bfd_vma prevaddr = 0;
>> +     while (getline(in, sysmap_line)) {
>> +             bfd_vma sysmap_addr;
>> +             string sysmap_name;
>> +             char sysmap_type;
>> +             if (!parse_line_sysmap(sysmap_line, sysmap_addr,  
>> sysmap_type, sysmap_name)) {
>> +                     cverb << vbfd << "Unexpected input on symbol  
>> line '" <<
>> +                             sysmap_line << "' in " << image_path  
>> << endl;
>> +                     ok = false;
>> +                     goto out_fail;
>> +             }
>> +             if (sysmap_addr < stext)
>> +                     continue;
>> +             if (sysmap_addr > etext)
>> +                     break;
>> +
>> +             // Can have multiple symbols of different types at  
>> same VMA,
>> +             // we only want to complete a code symbol if we've  
>> moved
>> +             // on to the next VMA.  If there are multiple code  
>> symbols
>> +             // at the same VMA, we use the last symbol.
>> +
>> +             // This covers the boring_symbols routine for BFD  
>> images
>> +             if (prevaddr != 0 && prevaddr != sysmap_addr) {
>> +
>> +                     add_sym_sysmap(symbol_filter, prevname,  
>> prevaddr, sysmap_addr);
>> +                     prevname.clear();
>> +                     prevaddr = 0;
>> +                     number_symbols++;
>> +             }
>> +
>> +             if (sysmap_type == 'T' || sysmap_type == 't') {
>> +                     // it's code
>> +                     // but we want to remove uninteresting symbols
>> +                     // ARM assembler internal mapping symbols
>> +                     // C++ exception stuff
>> +
>> +                     // taken from the routine interesting_symbols  
>> for BFD images
>> +                     if  (   (sysmap_name != "$a")
>> +                          && (sysmap_name != "$t")
>> +                          && (sysmap_name != "$d")
>> +                          && (sysmap_name != ".L")) {
>> +
>> +                             prevname = sysmap_name;
>> +                             prevaddr = sysmap_addr;
>> +                     }
>> +             }
>> +     }
>> +
>> +     cverb << vbfd << "stext:" << hex << stext << " etext:" << hex  
>> << etext << " added "
>> +           << dec << number_symbols << " symbols from " <<  
>> image_path << endl;
>> +
>> +
>> +     arch_len = 64;
>> +     if (stext < 0xffffffff)
>> +             arch_len = 32;
>> +
>> +     ok = true;
>> +
>> +     return;
>> }
>>
>>
>> @@ -194,16 +343,53 @@ op_bfd::~op_bfd()
>>
>> unsigned long op_bfd::get_start_offset(bfd_vma vma) const
>> {
>> +     if (sysmap) {
>> +             if (!vma || vma == section_vma)
>> +                     return section_filepos;
>> +             return 0;
>> +     } else {
>>      if (!vma || !ibfd.valid()) {
>>              filepos_map_t::const_iterator it =  
>> filepos_map.find(".text");
>>              if (it != filepos_map.end())
>>                      return it->second;
>>              return 0;
>>      }
>
> Please indent this under the new "else" clause.
>
>> +     }
>>
>>      return 0;
>> }
>>
>> +//  sysmap specific function
>> +bool op_bfd::parse_line_sysmap(string const & line, bfd_vma &  
>> addr, char & type, string & name)
>> +{
>> +     char * sym = NULL;
>> +     char * end = NULL;
>> +     int c = -1;
>> +#ifdef BFD64
>> +     c = sscanf(line.c_str(), "%Lx %c %as %as", &addr, &type,  
>> &sym, &end);
>
> Compilation error . . . . . . . --^
>
>> +#else
>> +     c = sscanf(line.c_str(), "%lx %c %as %as", &addr, &type,  
>> &sym, &end);
>> +#endif
>> +     if (c < 3) {
>> +             if (sym)
>> +                     free(sym);
>> +             return false;
>> +     }
>> +     name = sym;
>> +     free(sym);
>> +     return true;
>> +}
>> +
>> +//  sysmap specific function
>> +//  the ctor sets all the sizes and several BFD type of symbols
>> +void op_bfd::add_sym_sysmap(string_filter const & symbol_filter,
>> +                     string const & name, bfd_vma start, bfd_vma  
>> end)
>
>                                        ^-- maybe change to  
> 'previous_name'?
>
>> +{
>> +     if (symbol_filter.match(name)) {
>> +             syms.push_back(op_bfd_symbol(start-section_vma, end- 
>> start, name,
>> +                               section_vma, section_filepos));
>> +     }
>> +}
>>
>> void op_bfd::get_symbols(op_bfd::symbols_found_t & symbols)
>> {
> [snip]
>

Myke Smith wrote:
> 
> With help from Vitali I contacted Brian and he said he was happy to sign 
> off. I assume you need an email directly from Brian with the sign-off 
> statement when I am done. Does he need to also send in a copy of the 
> patch or is it sufficient that he email the "signature" in pointing to 
> the specific patch?

That's great, Myke.  Brian can reply to your version 3 posting and add his 
"Signed-off-by" line and point to some publicly accessible place where he posted 
his original patch.  He could post it here on the oprofile-list if he 
wanted/needed to.

Thanks!

-Maynard
> 
> -myke
> 
> On Feb 9, 2010, at 1:57 PM, Maynard Johnson wrote:
> 
>> Myke Smith wrote:
>>> Found and fixed a bug in opcontrol.
>>>
>>> This occurred if you chose not to identify any kernel symbols, did not
>>> set --use-kallsyms. This caused a hang in opcontrol get_image().
>>>
>>> The attached patch has everything in the previous patch plus this fix.
>>> Not sure if this is the appropriate way or if you want a patch to just
>>> opcontrol. If you just want a patch to opcontrol let me know and I will
>>> make it.
>>>
>>>
>>>
>>>
>>> Signed-off-by: Myke Smith <myke.smith@...>
>>>
>>>
>>> On Jan 28, 2010, at 10:44 AM, Myke Smith wrote:
>>
>> I apologize for the delay in reviewing this patch.  Other priorities 
>> got in the
>> way.  Thanks for the submission.  Comments embedded below.
>>
>> -Maynard
>>
>>>
>>>>
>>>> On hand held devices we are -typically- very memory constrained and
>>>> as such the standard vmlinux image is not available. The attached
>>>> patch re-uses and re-implements some of the ideas in Brian Eaton's
>>>> patch from the summer of 2006.
>>
>> Adding the "Signed-off-by" line below, implies you have read and are 
>> complying
>> with the "Developer's Certificate of Origin 1.1" (DCO) from
>> http://lwn.net/Articles/139918/.  Since by your own admission, this 
>> patch is
>> based on an original patch from Brian Eaton, you need to get Brian to 
>> sign off
>> on this patch.
>>
>>>>
>>>> Specifically it adds a flag to opcontrol --use-kallsyms which in
>>>> combination with --vm-nolinux  passes /proc/kallsyms to oprofiled.
>> I presume you mean "--no-vmlinux".
>>>>
>>>> There are modifications to libutil++/op_bfd.* to detect the use of
>>>> /proc/kallsyms and complementary routines to the standard BFD based
>>>> routines to handle the text versus binary file.
>>>>
>>>>
>>>>
>>>>
>>>> Signed-off-by: Myke Smith <myke.smith@...>
>>>>
>>> Index: oprofile/ChangeLog
>>> ===================================================================
>>
>> Your patch does not compile for me.  I get the following error:
>>
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
>>
>> make[3]: Entering directory `/home/mpj/oprof-work/op-tmp/libutil++'
>> if g++ -DHAVE_CONFIG_H -I. -I. -I..  -I ../libutil -I ../libop -I 
>> ../libpp  -W
>> -Wall -fno-common -ftemplate-depth-50 -Werror -g -O2 -MT op_bfd.o -MD 
>> -MP -MF
>> ".deps/op_bfd.Tpo" -c -o op_bfd.o op_bfd.cpp; \
>> then mv -f ".deps/op_bfd.Tpo" ".deps/op_bfd.Po"; else rm -f 
>> ".deps/op_bfd.Tpo";
>> exit 1; fi
>> cc1plus: warnings being treated as errors
>> op_bfd.h: In constructor ?op_bfd::op_bfd(const std::string&, const
>> string_filter&, const extra_images&, bool&)?:
>> op_bfd.h:332: warning: ?op_bfd::section_vma? will be initialized after
>> op_bfd.h:329: warning:   ?long unsigned int op_bfd::section_filepos?
>> op_bfd.cpp:114: warning:   when initialized here
>> make[3]: *** [op_bfd.o] Error 1
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
>>
>>
>> Member initializers are executed in the order in which they are found 
>> in the
>> class defintiion.  The warning message is telling you that the order 
>> in which
>> you coded the member initializers in the class constructor differs 
>> from that
>> found in the class definition.  If you patched and built this with an 
>> oprofile
>> cvs checkout, the warning should have caused a compile error for you.  
>> Maybe
>> your C++ compiler just isn't catching this.
>>
>> After fixing the above compile error, I ran into another one.  This 
>> second error
>> was only failing on a more recent g++ version (4.3):
>>
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
>>
>> make[3]: Entering directory `/home/mpj/temp/oprofile/libutil++'
>> g++ -DHAVE_CONFIG_H -I. -I..  -I ../libutil -I ../libop -I ../libpp  
>> -W -Wall
>> -fno-common -ftemplate-depth-50 -Werror -g -O2 -MT op_bfd.o -MD -MP -MF
>> .deps/op_bfd.Tpo -c -o op_bfd.o op_bfd.cpp
>> cc1plus: warnings being treated as errors
>> op_bfd.cpp: In member function 'bool op_bfd::parse_line_sysmap(const
>> std::string&, bfd_vma&, char&, std::string&)':
>> op_bfd.cpp:369: error: format '%Lx' expects type 'long long unsigned 
>> int*', but
>> argument 3 has type 'bfd_vma*'
>> make[3]: *** [op_bfd.o] Error 1
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
>>
>>
>> After fixing this compile error, I was able to build oprofile OK.
>>
>> Some general comments:
>>
>> 1. The new opcontrol option for using kallsyms must be documented in 
>> both the
>> opcontrol man page and in the oprofile user manual (doc/oprofile.xml).
>> 2. Once set, there's no way to change your new option.  So instead of
>> '--use-kallsyms', perhaps it should be '--use-kallsys=[y | n]'.
>> 3. Should it be an illegal operation to set '--vmlinux' and 
>> '--use-kallsyms' at
>> the same time?  Currently, it's not.
>> 4. I think there should be a new op_bfd constructor for the 
>> /proc/kallsyms.
>> Inserting all the special case code into the existing ctor is messy.  
>> Besides, I
>> don't think there's a need for extra_images to be passed for a 
>> "/proc/kallsyms"
>> op_bfd ctor.  Or am I wrong about that?
>> 5. Please look at all of your comments and documentation and clean 
>> them up for
>> grammar and clarity.
>>
>>
>>> RCS file: /cvsroot/oprofile/oprofile/ChangeLog,v
>>> retrieving revision 1.1918
>>> diff -p -a -u -r1.1918 ChangeLog
>>> --- oprofile/ChangeLog        26 Jan 2010 14:42:00 -0000      1.1918
>>> +++ oprofile/ChangeLog        29 Jan 2010 00:21:59 -0000
>>> @@ -1,3 +1,19 @@
>>> +2010-01-28  Myke Smith <myke.smith@...>
>>> +     * utils/opcontrol
>>> +      Changes made to support using /proc/kallsyms as a source of 
>>> kernel symbols.
>>> +      New flag added --use-kallsyms and it requires --no-vmlinux to 
>>> be active.
>>> +
>>> +     * libpp/poulate.cpp
>>> +       If the file is /proc/kallsyms ignore mtime check since this 
>>> file can be
>>> +             updated at any time with modules being loaded/unloaded
>>> +
>>> +     * libutils++/op_bfd.cpp
>>> +       Added a constructor to handle /proc/kallsyms.
>>> +       Added support routines for parsing and retrieving symbols from
>>> +             /proc/kallsyms.
>>> +       Added a boolean "sysmap" which when set short-circuits somei 
>>> tests that are
>>> +             BFD oriented.
>>> +
>>> 2010-01-20  Maynard Johnson  <maynardj@...>
>>>
>>>      * m4/qt.m4: Fix qt lib check so it works on base 64-bit system
>>> Index: oprofile/libpp/populate.cpp
>>> ===================================================================
>>> RCS file: /cvsroot/oprofile/oprofile/libpp/populate.cpp,v
>>> retrieving revision 1.14
>>> diff -p -a -u -r1.14 populate.cpp
>>> --- oprofile/libpp/populate.cpp       21 Jan 2008 21:35:17 -0000      
>>> 1.14
>>> +++ oprofile/libpp/populate.cpp       29 Jan 2010 00:21:59 -0000
>>> @@ -102,6 +102,9 @@ populate_for_image(profile_container & s
>>>              string filename =
>>>                      samples.extra_found_images.find_image_path(
>>>                              ip.image, error, true);
>>> +
>>> +             // don't check mtime for proc/kallsyms cause always 
>>> changing with modules in & out
>>> +             if (filename.compare("/proc/kallsyms") != 0)
>>>              check_mtime(filename, header);
>>                 ^--- indentation needed
>>>      }
>>>
>>> Index: oprofile/libutil++/bfd_support.h
>>> ===================================================================
>>> RCS file: /cvsroot/oprofile/oprofile/libutil++/bfd_support.h,v
>>> retrieving revision 1.9
>>> diff -p -a -u -r1.9 bfd_support.h
>>> --- oprofile/libutil++/bfd_support.h  26 Oct 2009 13:01:09 -0000      
>>> 1.9
>>> +++ oprofile/libutil++/bfd_support.h  29 Jan 2010 00:21:59 -0000
>>> @@ -117,6 +117,10 @@ bfd * open_bfd(std::string const & file)
>>> /// open the given BFD from the fd
>>> bfd * fdopen_bfd(std::string const & file, int fd);
>>>
>>> +/// open the given BFD from -in this SPECIFIC case is a map file and 
>>> not binary- the fd
>>> +/// different from above in that we do not do a format_check()
>>> +bfd * fdopen_map(std::string const & file, int fd);
>>> +
>>> /// Return a BFD for an SPU ELF embedded in PPE binary file
>>> bfd * spu_open_bfd(std::string const name, int fd, uint64_t 
>>> offset_to_spu_elf);
>>>
>>> Index: oprofile/libutil++/op_bfd.cpp
>>> ===================================================================
>>> RCS file: /cvsroot/oprofile/oprofile/libutil++/op_bfd.cpp,v
>>> retrieving revision 1.87
>>> diff -p -a -u -r1.87 op_bfd.cpp
>>> --- oprofile/libutil++/op_bfd.cpp     20 Jul 2009 13:24:47 -0000      
>>> 1.87
>>> +++ oprofile/libutil++/op_bfd.cpp     29 Jan 2010 00:21:59 -0000
>>> @@ -24,6 +24,9 @@
>>> #include <iostream>
>>> #include <iomanip>
>>> #include <sstream>
>>> +// for sysmap
>>> +#include <errno.h>
>>> +#include <fstream>
>>>
>>> #include "op_bfd.h"
>>> #include "locate_images.h"
>>> @@ -85,6 +88,16 @@ op_bfd_symbol::op_bfd_symbol(bfd_vma vma
>>> {
>>> }
>>>
>>> +// for sysmap
>>> +op_bfd_symbol::op_bfd_symbol(bfd_vma vma, size_t size, string const 
>>> & name,
>>> +                             bfd_vma sectionvma, unsigned long 
>>> sectionfilepos)
>>> +     : bfd_symbol(0), symb_value(vma),
>>> +       section_filepos(sectionfilepos), section_vma(sectionvma),
>>> +       symb_size(size), symb_name(name),
>>> +       symb_hidden(false), symb_weak(false), symb_artificial(false)
>>> +{
>>> +}
>>> +
>>>
>>> bool op_bfd_symbol::operator<(op_bfd_symbol const & rhs) const
>>> {
>>> @@ -104,7 +117,11 @@ op_bfd::op_bfd(string const & fname, str
>>>      archive_path(extra_images.get_archive_path()),
>>>      extra_found_images(extra_images),
>>>      file_size(-1),
>>> -     anon_obj(false)
>>> +     anon_obj(false),
>>> +     sysmap(false),
>>> +     section_vma(0),
>>
>> Compilation error --^
>>
>>> +     section_filepos(0),
>>> +     arch_len(0)
>>> {
>>>      int fd;
>>>      struct stat st;
>>> @@ -116,10 +133,16 @@ op_bfd::op_bfd(string const & fname, str
>>>      string suf = ".jo";
>>>
>>>      image_error img_ok;
>>> +
>>> +     if ((filename.compare("/proc/kallsyms") == 0)) {
>>> +             sysmap = true;
>>> +             cverb << vbfd << "sysmap enabled by  " << filename << 
>>> endl;
>>> +     }
>>> +
>>>      string const image_path =
>>>              extra_images.find_image_path(filename, img_ok, true);
>>>
>>> -     cverb << vbfd << "op_bfd ctor for " << image_path << endl;
>>> +     cverb << vbfd << "op_bfd ctor for " << image_path << " filename 
>>> was " << filename << endl;
>>>
>>>      // if there's a problem already, don't try to open it
>>>      if (!ok || img_ok != image_ok) {
>>> @@ -127,6 +150,12 @@ op_bfd::op_bfd(string const & fname, str
>>>              goto out_fail;
>>>      }
>>>
>>> +     /* Since the sysmap is NOT a binary file we can't use some of 
>>> the functions *
>>> +      * to get and add symbols. So we will call sysmap specific 
>>> versions to do   *
>>> +      * 
>>> work                                                                     
>>> */
>>> +     if (sysmap) goto sysmap_image;
>>> +
>>> +
>>>      fd = open(image_path.c_str(), O_RDONLY);
>>>      if (fd == -1) {
>>>              cverb << vbfd << "open failed for " << image_path << endl;
>>> @@ -184,6 +213,126 @@ out_fail:
>>>      // make the fake symbol fit within the fake file
>>>      file_size = -1;
>>>      goto out;
>>> +
>>> +sysmap_image:
>>> +     // First find the start and end of the text segment.
>>> +     bfd_vma stext = 0;
>>> +     bfd_vma etext = 0;
>>> +
>>> +     ifstream in(image_path.c_str());
>>> +     if (!in) {
>>> +             cverb << vbfd << "Unable to open '" << image_path <<
>>> +                   "': " << strerror(errno) << endl;
>>> +             ok = false;
>>> +             goto out_fail;
>>> +     }
>>> +     string sysmap_line;
>>> +     while (getline(in, sysmap_line)) {
>>> +             bfd_vma sysmap_addr;
>>> +             string sysmap_name;
>>> +             char sysmap_type;
>>> +             if (!parse_line_sysmap(sysmap_line, sysmap_addr, 
>>> sysmap_type, sysmap_name)) {
>>> +                     cverb << vbfd << "Unexpected input on symbol 
>>> line '" <<
>>> +                             sysmap_line << "' in /proc/kallsyms " <<
>>> +                             image_path << endl;
>>> +                     ok = false;
>>> +                     goto out_fail;
>>> +             }
>>> +
>>> +             if (   (  (sysmap_name == "_text")
>>> +                    || (sysmap_name == "_stext"))
>>> +                 && (stext == 0)) {
>>> +                     stext = sysmap_addr;
>>> +
>>> +             } else if (sysmap_name == "_etext")
>>> +                     etext = sysmap_addr;
>>> +
>>> +             // We keep parsing even if we have both _text/_stext and
>>> +             // _etext, just to make sure the file looks right.
>>> +
>>> +
>>> +
>>> +     }
>>> +     if (in.bad()) {
>>> +             cverb << vbfd << "Error on symbol line '" << 
>>> sysmap_line <<
>>> +                     "' in /proc/kallsyms " << image_path << ": " <<
>>> +                     strerror(errno) << endl;
>>> +             ok = false;
>>> +             goto out_fail;
>>> +     }
>>> +
>>> +     if (stext == 0 || etext == 0) {
>>> +             cverb << vbfd << "Unable to find _text and _etext in " 
>>> << image_path << endl;
>>> +             ok = false;
>>> +             goto out_fail;
>>> +     }
>>> +
>>> +     section_vma = stext;
>>> +
>>> +     // Read all of the code symbols in the text segment
>>> +     int number_symbols = 0;
>>> +     in.clear();
>>> +     in.seekg(0, ios::beg);
>>> +     string prevname;
>>> +     bfd_vma prevaddr = 0;
>>> +     while (getline(in, sysmap_line)) {
>>> +             bfd_vma sysmap_addr;
>>> +             string sysmap_name;
>>> +             char sysmap_type;
>>> +             if (!parse_line_sysmap(sysmap_line, sysmap_addr, 
>>> sysmap_type, sysmap_name)) {
>>> +                     cverb << vbfd << "Unexpected input on symbol 
>>> line '" <<
>>> +                             sysmap_line << "' in " << image_path << 
>>> endl;
>>> +                     ok = false;
>>> +                     goto out_fail;
>>> +             }
>>> +             if (sysmap_addr < stext)
>>> +                     continue;
>>> +             if (sysmap_addr > etext)
>>> +                     break;
>>> +
>>> +             // Can have multiple symbols of different types at same 
>>> VMA,
>>> +             // we only want to complete a code symbol if we've moved
>>> +             // on to the next VMA.  If there are multiple code symbols
>>> +             // at the same VMA, we use the last symbol.
>>> +
>>> +             // This covers the boring_symbols routine for BFD images
>>> +             if (prevaddr != 0 && prevaddr != sysmap_addr) {
>>> +
>>> +                     add_sym_sysmap(symbol_filter, prevname, 
>>> prevaddr, sysmap_addr);
>>> +                     prevname.clear();
>>> +                     prevaddr = 0;
>>> +                     number_symbols++;
>>> +             }
>>> +
>>> +             if (sysmap_type == 'T' || sysmap_type == 't') {
>>> +                     // it's code
>>> +                     // but we want to remove uninteresting symbols
>>> +                     // ARM assembler internal mapping symbols
>>> +                     // C++ exception stuff
>>> +
>>> +                     // taken from the routine interesting_symbols 
>>> for BFD images
>>> +                     if  (   (sysmap_name != "$a")
>>> +                          && (sysmap_name != "$t")
>>> +                          && (sysmap_name != "$d")
>>> +                          && (sysmap_name != ".L")) {
>>> +
>>> +                             prevname = sysmap_name;
>>> +                             prevaddr = sysmap_addr;
>>> +                     }
>>> +             }
>>> +     }
>>> +
>>> +     cverb << vbfd << "stext:" << hex << stext << " etext:" << hex 
>>> << etext << " added "
>>> +           << dec << number_symbols << " symbols from " << 
>>> image_path << endl;
>>> +
>>> +
>>> +     arch_len = 64;
>>> +     if (stext < 0xffffffff)
>>> +             arch_len = 32;
>>> +
>>> +     ok = true;
>>> +
>>> +     return;
>>> }
>>>
>>>
>>> @@ -194,16 +343,53 @@ op_bfd::~op_bfd()
>>>
>>> unsigned long op_bfd::get_start_offset(bfd_vma vma) const
>>> {
>>> +     if (sysmap) {
>>> +             if (!vma || vma == section_vma)
>>> +                     return section_filepos;
>>> +             return 0;
>>> +     } else {
>>>      if (!vma || !ibfd.valid()) {
>>>              filepos_map_t::const_iterator it = 
>>> filepos_map.find(".text");
>>>              if (it != filepos_map.end())
>>>                      return it->second;
>>>              return 0;
>>>      }
>>
>> Please indent this under the new "else" clause.
>>
>>> +     }
>>>
>>>      return 0;
>>> }
>>>
>>> +//  sysmap specific function
>>> +bool op_bfd::parse_line_sysmap(string const & line, bfd_vma & addr, 
>>> char & type, string & name)
>>> +{
>>> +     char * sym = NULL;
>>> +     char * end = NULL;
>>> +     int c = -1;
>>> +#ifdef BFD64
>>> +     c = sscanf(line.c_str(), "%Lx %c %as %as", &addr, &type, &sym, 
>>> &end);
>>
>> Compilation error . . . . . . . --^
>>
>>> +#else
>>> +     c = sscanf(line.c_str(), "%lx %c %as %as", &addr, &type, &sym, 
>>> &end);
>>> +#endif
>>> +     if (c < 3) {
>>> +             if (sym)
>>> +                     free(sym);
>>> +             return false;
>>> +     }
>>> +     name = sym;
>>> +     free(sym);
>>> +     return true;
>>> +}
>>> +
>>> +//  sysmap specific function
>>> +//  the ctor sets all the sizes and several BFD type of symbols
>>> +void op_bfd::add_sym_sysmap(string_filter const & symbol_filter,
>>> +                     string const & name, bfd_vma start, bfd_vma end)
>>
>>                                        ^-- maybe change to 
>> 'previous_name'?
>>
>>> +{
>>> +     if (symbol_filter.match(name)) {
>>> +             syms.push_back(op_bfd_symbol(start-section_vma, 
>>> end-start, name,
>>> +                               section_vma, section_filepos));
>>> +     }
>>> +}
>>>
>>> void op_bfd::get_symbols(op_bfd::symbols_found_t & symbols)
>>> {
>> [snip]
>>
>

Myke Smith wrote:
> 
> Thank you for your comments.  The biggest issue might be contacting 
> Brian. I emailed him a few months ago at the email in his original 
> patches and got no replies.
> If you have newer contact information I would appreciate it being passed 
> on to me.
> 
> I will start working on the issues you listed.
> 
>> 1. The new opcontrol option for using kallsyms must be documented in 
>> both the
>> opcontrol man page and in the oprofile user manual (doc/oprofile.xml).
> 
> Is this normally done through a patch file as well? Do you want 
> everything in one patch file?
Yes, include the doc changes in the same patch as the code changes.  For 
oprofile user manual changes, edit the doc/oprofile.xml file.  For opcontrol man 
page changes, edit the doc/opcontrol.1.in file.  Run autogen.sh and configure. 
Then, to build all the docs, cd into the doc directory and do 'make chunk'.

-Maynard
> 
> thanks,
> 
> -myke
> 
[snip]

Maynard,

I got some time to go back and work on updating the patch with your  
requests. However,  I am stumbling across request #4

> 4. I think there should be a new op_bfd constructor for the /proc/ 
> kallsyms.
> Inserting all the special case code into the existing ctor is  
> messy.  Besides, I
> don't think there's a need for extra_images to be passed for a "/ 
> proc/kallsyms"
> op_bfd ctor.  Or am I wrong about that?

Currently, within op_bfd I evaluate "fname" to see if it is equal to "/ 
proc/kallsyms". To have a distinct ctor to support /proc/kallsysms  
will require doing this evaluation everywhere the current bfd_op ctor  
is invoked.  To me this seems to go against the grain of what you  
really want and surfaces information best left in the ctor

As to the extra_images, no I don't use this.

I can clean up the current ctor to more clearly and cleanly separate  
out the support for /proc/kallsyms but if you still believe that a  
separate ctor is the best approach I will do so.


regards,
-myke

On Feb 9, 2010, at 1:57 PM, Maynard Johnson wrote:

> Myke Smith wrote:
>> Found and fixed a bug in opcontrol.
>>
>> This occurred if you chose not to identify any kernel symbols, did  
>> not
>> set --use-kallsyms. This caused a hang in opcontrol get_image().
>>
>> The attached patch has everything in the previous patch plus this  
>> fix.
>> Not sure if this is the appropriate way or if you want a patch to  
>> just
>> opcontrol. If you just want a patch to opcontrol let me know and I  
>> will
>> make it.
>>
>>
>>
>>
>> Signed-off-by: Myke Smith <myke.smith@...>
>>
>>
>> On Jan 28, 2010, at 10:44 AM, Myke Smith wrote:
>
> I apologize for the delay in reviewing this patch.  Other priorities  
> got in the
> way.  Thanks for the submission.  Comments embedded below.
>
> -Maynard
>
>>
>>>
>>> On hand held devices we are -typically- very memory constrained and
>>> as such the standard vmlinux image is not available. The attached
>>> patch re-uses and re-implements some of the ideas in Brian Eaton's
>>> patch from the summer of 2006.
>
> Adding the "Signed-off-by" line below, implies you have read and are  
> complying
> with the "Developer's Certificate of Origin 1.1" (DCO) from
> http://lwn.net/Articles/139918/.  Since by your own admission, this  
> patch is
> based on an original patch from Brian Eaton, you need to get Brian  
> to sign off
> on this patch.
>
>>>
>>> Specifically it adds a flag to opcontrol --use-kallsyms which in
>>> combination with --vm-nolinux  passes /proc/kallsyms to oprofiled.
> I presume you mean "--no-vmlinux".
>>>
>>> There are modifications to libutil++/op_bfd.* to detect the use of
>>> /proc/kallsyms and complementary routines to the standard BFD based
>>> routines to handle the text versus binary file.
>>>
>>>
>>>
>>>
>>> Signed-off-by: Myke Smith <myke.smith@...>
>>>
>> Index: oprofile/ChangeLog
>> ===================================================================
>
> Your patch does not compile for me.  I get the following error:
>
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> +++++
> make[3]: Entering directory `/home/mpj/oprof-work/op-tmp/libutil++'
> if g++ -DHAVE_CONFIG_H -I. -I. -I..  -I ../libutil -I ../libop -I ../ 
> libpp  -W
> -Wall -fno-common -ftemplate-depth-50 -Werror -g -O2 -MT op_bfd.o - 
> MD -MP -MF
> ".deps/op_bfd.Tpo" -c -o op_bfd.o op_bfd.cpp; \
> then mv -f ".deps/op_bfd.Tpo" ".deps/op_bfd.Po"; else rm -f ".deps/ 
> op_bfd.Tpo";
> exit 1; fi
> cc1plus: warnings being treated as errors
> op_bfd.h: In constructor ?op_bfd::op_bfd(const std::string&, const
> string_filter&, const extra_images&, bool&)?:
> op_bfd.h:332: warning: ?op_bfd::section_vma? will be initialized after
> op_bfd.h:329: warning:   ?long unsigned int op_bfd::section_filepos?
> op_bfd.cpp:114: warning:   when initialized here
> make[3]: *** [op_bfd.o] Error 1
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> +++++
>
> Member initializers are executed in the order in which they are  
> found in the
> class defintiion.  The warning message is telling you that the order  
> in which
> you coded the member initializers in the class constructor differs  
> from that
> found in the class definition.  If you patched and built this with  
> an oprofile
> cvs checkout, the warning should have caused a compile error for  
> you.  Maybe
> your C++ compiler just isn't catching this.
>
> After fixing the above compile error, I ran into another one.  This  
> second error
> was only failing on a more recent g++ version (4.3):
>
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> +++++
> make[3]: Entering directory `/home/mpj/temp/oprofile/libutil++'
> g++ -DHAVE_CONFIG_H -I. -I..  -I ../libutil -I ../libop -I ../libpp   
> -W -Wall
> -fno-common -ftemplate-depth-50 -Werror -g -O2 -MT op_bfd.o -MD -MP - 
> MF
> .deps/op_bfd.Tpo -c -o op_bfd.o op_bfd.cpp
> cc1plus: warnings being treated as errors
> op_bfd.cpp: In member function 'bool op_bfd::parse_line_sysmap(const
> std::string&, bfd_vma&, char&, std::string&)':
> op_bfd.cpp:369: error: format '%Lx' expects type 'long long unsigned  
> int*', but
> argument 3 has type 'bfd_vma*'
> make[3]: *** [op_bfd.o] Error 1
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> +++++
>
> After fixing this compile error, I was able to build oprofile OK.
>
> Some general comments:
>
> 1. The new opcontrol option for using kallsyms must be documented in  
> both the
> opcontrol man page and in the oprofile user manual (doc/oprofile.xml).
> 2. Once set, there's no way to change your new option.  So instead of
> '--use-kallsyms', perhaps it should be '--use-kallsys=[y | n]'.
> 3. Should it be an illegal operation to set '--vmlinux' and '--use- 
> kallsyms' at
> the same time?  Currently, it's not.
> 4. I think there should be a new op_bfd constructor for the /proc/ 
> kallsyms.
> Inserting all the special case code into the existing ctor is  
> messy.  Besides, I
> don't think there's a need for extra_images to be passed for a "/ 
> proc/kallsyms"
> op_bfd ctor.  Or am I wrong about that?
> 5. Please look at all of your comments and documentation and clean  
> them up for
> grammar and clarity.
>
>
>> RCS file: /cvsroot/oprofile/oprofile/ChangeLog,v
>> retrieving revision 1.1918
>> diff -p -a -u -r1.1918 ChangeLog
>> --- oprofile/ChangeLog        26 Jan 2010 14:42:00 -0000      1.1918
>> +++ oprofile/ChangeLog        29 Jan 2010 00:21:59 -0000
>> @@ -1,3 +1,19 @@
>> +2010-01-28  Myke Smith <myke.smith@...>
>> +     * utils/opcontrol
>> +      Changes made to support using /proc/kallsyms as a source of  
>> kernel symbols.
>> +      New flag added --use-kallsyms and it requires --no-vmlinux  
>> to be active.
>> +
>> +     * libpp/poulate.cpp
>> +       If the file is /proc/kallsyms ignore mtime check since this  
>> file can be
>> +             updated at any time with modules being loaded/unloaded
>> +
>> +     * libutils++/op_bfd.cpp
>> +       Added a constructor to handle /proc/kallsyms.
>> +       Added support routines for parsing and retrieving symbols  
>> from
>> +             /proc/kallsyms.
>> +       Added a boolean "sysmap" which when set short-circuits  
>> somei tests that are
>> +             BFD oriented.
>> +
>> 2010-01-20  Maynard Johnson  <maynardj@...>
>>
>>      * m4/qt.m4: Fix qt lib check so it works on base 64-bit system
>> Index: oprofile/libpp/populate.cpp
>> ===================================================================
>> RCS file: /cvsroot/oprofile/oprofile/libpp/populate.cpp,v
>> retrieving revision 1.14
>> diff -p -a -u -r1.14 populate.cpp
>> --- oprofile/libpp/populate.cpp       21 Jan 2008 21:35:17  
>> -0000      1.14
>> +++ oprofile/libpp/populate.cpp       29 Jan 2010 00:21:59 -0000
>> @@ -102,6 +102,9 @@ populate_for_image(profile_container & s
>>              string filename =
>>                      samples.extra_found_images.find_image_path(
>>                              ip.image, error, true);
>> +
>> +             // don't check mtime for proc/kallsyms cause always  
>> changing with modules in & out
>> +             if (filename.compare("/proc/kallsyms") != 0)
>>              check_mtime(filename, header);
>                 ^--- indentation needed
>>      }
>>
>> Index: oprofile/libutil++/bfd_support.h
>> ===================================================================
>> RCS file: /cvsroot/oprofile/oprofile/libutil++/bfd_support.h,v
>> retrieving revision 1.9
>> diff -p -a -u -r1.9 bfd_support.h
>> --- oprofile/libutil++/bfd_support.h  26 Oct 2009 13:01:09  
>> -0000      1.9
>> +++ oprofile/libutil++/bfd_support.h  29 Jan 2010 00:21:59 -0000
>> @@ -117,6 +117,10 @@ bfd * open_bfd(std::string const & file)
>> /// open the given BFD from the fd
>> bfd * fdopen_bfd(std::string const & file, int fd);
>>
>> +/// open the given BFD from -in this SPECIFIC case is a map file  
>> and not binary- the fd
>> +/// different from above in that we do not do a format_check()
>> +bfd * fdopen_map(std::string const & file, int fd);
>> +
>> /// Return a BFD for an SPU ELF embedded in PPE binary file
>> bfd * spu_open_bfd(std::string const name, int fd, uint64_t  
>> offset_to_spu_elf);
>>
>> Index: oprofile/libutil++/op_bfd.cpp
>> ===================================================================
>> RCS file: /cvsroot/oprofile/oprofile/libutil++/op_bfd.cpp,v
>> retrieving revision 1.87
>> diff -p -a -u -r1.87 op_bfd.cpp
>> --- oprofile/libutil++/op_bfd.cpp     20 Jul 2009 13:24:47  
>> -0000      1.87
>> +++ oprofile/libutil++/op_bfd.cpp     29 Jan 2010 00:21:59 -0000
>> @@ -24,6 +24,9 @@
>> #include <iostream>
>> #include <iomanip>
>> #include <sstream>
>> +// for sysmap
>> +#include <errno.h>
>> +#include <fstream>
>>
>> #include "op_bfd.h"
>> #include "locate_images.h"
>> @@ -85,6 +88,16 @@ op_bfd_symbol::op_bfd_symbol(bfd_vma vma
>> {
>> }
>>
>> +// for sysmap
>> +op_bfd_symbol::op_bfd_symbol(bfd_vma vma, size_t size, string  
>> const & name,
>> +                             bfd_vma sectionvma, unsigned long  
>> sectionfilepos)
>> +     : bfd_symbol(0), symb_value(vma),
>> +       section_filepos(sectionfilepos), section_vma(sectionvma),
>> +       symb_size(size), symb_name(name),
>> +       symb_hidden(false), symb_weak(false), symb_artificial(false)
>> +{
>> +}
>> +
>>
>> bool op_bfd_symbol::operator<(op_bfd_symbol const & rhs) const
>> {
>> @@ -104,7 +117,11 @@ op_bfd::op_bfd(string const & fname, str
>>      archive_path(extra_images.get_archive_path()),
>>      extra_found_images(extra_images),
>>      file_size(-1),
>> -     anon_obj(false)
>> +     anon_obj(false),
>> +     sysmap(false),
>> +     section_vma(0),
>
> Compilation error --^
>
>> +     section_filepos(0),
>> +     arch_len(0)
>> {
>>      int fd;
>>      struct stat st;
>> @@ -116,10 +133,16 @@ op_bfd::op_bfd(string const & fname, str
>>      string suf = ".jo";
>>
>>      image_error img_ok;
>> +
>> +     if ((filename.compare("/proc/kallsyms") == 0)) {
>> +             sysmap = true;
>> +             cverb << vbfd << "sysmap enabled by  " << filename <<  
>> endl;
>> +     }
>> +
>>      string const image_path =
>>              extra_images.find_image_path(filename, img_ok, true);
>>
>> -     cverb << vbfd << "op_bfd ctor for " << image_path << endl;
>> +     cverb << vbfd << "op_bfd ctor for " << image_path << "  
>> filename was " << filename << endl;
>>
>>      // if there's a problem already, don't try to open it
>>      if (!ok || img_ok != image_ok) {
>> @@ -127,6 +150,12 @@ op_bfd::op_bfd(string const & fname, str
>>              goto out_fail;
>>      }
>>
>> +     /* Since the sysmap is NOT a binary file we can't use some of  
>> the functions *
>> +      * to get and add symbols. So we will call sysmap specific  
>> versions to do   *
>> +      *  
>> work 
>>                                                                      */
>> +     if (sysmap) goto sysmap_image;
>> +
>> +
>>      fd = open(image_path.c_str(), O_RDONLY);
>>      if (fd == -1) {
>>              cverb << vbfd << "open failed for " << image_path <<  
>> endl;
>> @@ -184,6 +213,126 @@ out_fail:
>>      // make the fake symbol fit within the fake file
>>      file_size = -1;
>>      goto out;
>> +
>> +sysmap_image:
>> +     // First find the start and end of the text segment.
>> +     bfd_vma stext = 0;
>> +     bfd_vma etext = 0;
>> +
>> +     ifstream in(image_path.c_str());
>> +     if (!in) {
>> +             cverb << vbfd << "Unable to open '" << image_path <<
>> +                   "': " << strerror(errno) << endl;
>> +             ok = false;
>> +             goto out_fail;
>> +     }
>> +     string sysmap_line;
>> +     while (getline(in, sysmap_line)) {
>> +             bfd_vma sysmap_addr;
>> +             string sysmap_name;
>> +             char sysmap_type;
>> +             if (!parse_line_sysmap(sysmap_line, sysmap_addr,  
>> sysmap_type, sysmap_name)) {
>> +                     cverb << vbfd << "Unexpected input on symbol  
>> line '" <<
>> +                             sysmap_line << "' in /proc/kallsyms "  
>> <<
>> +                             image_path << endl;
>> +                     ok = false;
>> +                     goto out_fail;
>> +             }
>> +
>> +             if (   (  (sysmap_name == "_text")
>> +                    || (sysmap_name == "_stext"))
>> +                 && (stext == 0)) {
>> +                     stext = sysmap_addr;
>> +
>> +             } else if (sysmap_name == "_etext")
>> +                     etext = sysmap_addr;
>> +
>> +             // We keep parsing even if we have both _text/_stext  
>> and
>> +             // _etext, just to make sure the file looks right.
>> +
>> +
>> +
>> +     }
>> +     if (in.bad()) {
>> +             cverb << vbfd << "Error on symbol line '" <<  
>> sysmap_line <<
>> +                     "' in /proc/kallsyms " << image_path << ": " <<
>> +                     strerror(errno) << endl;
>> +             ok = false;
>> +             goto out_fail;
>> +     }
>> +
>> +     if (stext == 0 || etext == 0) {
>> +             cverb << vbfd << "Unable to find _text and _etext in  
>> " << image_path << endl;
>> +             ok = false;
>> +             goto out_fail;
>> +     }
>> +
>> +     section_vma = stext;
>> +
>> +     // Read all of the code symbols in the text segment
>> +     int number_symbols = 0;
>> +     in.clear();
>> +     in.seekg(0, ios::beg);
>> +     string prevname;
>> +     bfd_vma prevaddr = 0;
>> +     while (getline(in, sysmap_line)) {
>> +             bfd_vma sysmap_addr;
>> +             string sysmap_name;
>> +             char sysmap_type;
>> +             if (!parse_line_sysmap(sysmap_line, sysmap_addr,  
>> sysmap_type, sysmap_name)) {
>> +                     cverb << vbfd << "Unexpected input on symbol  
>> line '" <<
>> +                             sysmap_line << "' in " << image_path  
>> << endl;
>> +                     ok = false;
>> +                     goto out_fail;
>> +             }
>> +             if (sysmap_addr < stext)
>> +                     continue;
>> +             if (sysmap_addr > etext)
>> +                     break;
>> +
>> +             // Can have multiple symbols of different types at  
>> same VMA,
>> +             // we only want to complete a code symbol if we've  
>> moved
>> +             // on to the next VMA.  If there are multiple code  
>> symbols
>> +             // at the same VMA, we use the last symbol.
>> +
>> +             // This covers the boring_symbols routine for BFD  
>> images
>> +             if (prevaddr != 0 && prevaddr != sysmap_addr) {
>> +
>> +                     add_sym_sysmap(symbol_filter, prevname,  
>> prevaddr, sysmap_addr);
>> +                     prevname.clear();
>> +                     prevaddr = 0;
>> +                     number_symbols++;
>> +             }
>> +
>> +             if (sysmap_type == 'T' || sysmap_type == 't') {
>> +                     // it's code
>> +                     // but we want to remove uninteresting symbols
>> +                     // ARM assembler internal mapping symbols
>> +                     // C++ exception stuff
>> +
>> +                     // taken from the routine interesting_symbols  
>> for BFD images
>> +                     if  (   (sysmap_name != "$a")
>> +                          && (sysmap_name != "$t")
>> +                          && (sysmap_name != "$d")
>> +                          && (sysmap_name != ".L")) {
>> +
>> +                             prevname = sysmap_name;
>> +                             prevaddr = sysmap_addr;
>> +                     }
>> +             }
>> +     }
>> +
>> +     cverb << vbfd << "stext:" << hex << stext << " etext:" << hex  
>> << etext << " added "
>> +           << dec << number_symbols << " symbols from " <<  
>> image_path << endl;
>> +
>> +
>> +     arch_len = 64;
>> +     if (stext < 0xffffffff)
>> +             arch_len = 32;
>> +
>> +     ok = true;
>> +
>> +     return;
>> }
>>
>>
>> @@ -194,16 +343,53 @@ op_bfd::~op_bfd()
>>
>> unsigned long op_bfd::get_start_offset(bfd_vma vma) const
>> {
>> +     if (sysmap) {
>> +             if (!vma || vma == section_vma)
>> +                     return section_filepos;
>> +             return 0;
>> +     } else {
>>      if (!vma || !ibfd.valid()) {
>>              filepos_map_t::const_iterator it =  
>> filepos_map.find(".text");
>>              if (it != filepos_map.end())
>>                      return it->second;
>>              return 0;
>>      }
>
> Please indent this under the new "else" clause.
>
>> +     }
>>
>>      return 0;
>> }
>>
>> +//  sysmap specific function
>> +bool op_bfd::parse_line_sysmap(string const & line, bfd_vma &  
>> addr, char & type, string & name)
>> +{
>> +     char * sym = NULL;
>> +     char * end = NULL;
>> +     int c = -1;
>> +#ifdef BFD64
>> +     c = sscanf(line.c_str(), "%Lx %c %as %as", &addr, &type,  
>> &sym, &end);
>
> Compilation error . . . . . . . --^
>
>> +#else
>> +     c = sscanf(line.c_str(), "%lx %c %as %as", &addr, &type,  
>> &sym, &end);
>> +#endif
>> +     if (c < 3) {
>> +             if (sym)
>> +                     free(sym);
>> +             return false;
>> +     }
>> +     name = sym;
>> +     free(sym);
>> +     return true;
>> +}
>> +
>> +//  sysmap specific function
>> +//  the ctor sets all the sizes and several BFD type of symbols
>> +void op_bfd::add_sym_sysmap(string_filter const & symbol_filter,
>> +                     string const & name, bfd_vma start, bfd_vma  
>> end)
>
>                                        ^-- maybe change to  
> 'previous_name'?
>
>> +{
>> +     if (symbol_filter.match(name)) {
>> +             syms.push_back(op_bfd_symbol(start-section_vma, end- 
>> start, name,
>> +                               section_vma, section_filepos));
>> +     }
>> +}
>>
>> void op_bfd::get_symbols(op_bfd::symbols_found_t & symbols)
>> {
> [snip]
>

Myke Smith wrote:
> Maynard,
> 
> I got some time to go back and work on updating the patch with your 
> requests. However,  I am stumbling across request #4
> 
>> 4. I think there should be a new op_bfd constructor for the 
>> /proc/kallsyms.
>> Inserting all the special case code into the existing ctor is messy.  
>> Besides, I
>> don't think there's a need for extra_images to be passed for a 
>> "/proc/kallsyms"
>> op_bfd ctor.  Or am I wrong about that?
> 
> Currently, within op_bfd I evaluate "fname" to see if it is equal to 
> "/proc/kallsyms". To have a distinct ctor to support /proc/kallsysms 
> will require doing this evaluation everywhere the current bfd_op ctor is 
> invoked.  To me this seems to go against the grain of what you really 
> want and surfaces information best left in the ctor

Yes, you're right . . . easier to let the existing ctor handle this.  Maybe pull 
the sysmap stuff out into a helper function.

-Maynard

> 
> As to the extra_images, no I don't use this.
> 
> I can clean up the current ctor to more clearly and cleanly separate out 
> the support for /proc/kallsyms but if you still believe that a separate 
> ctor is the best approach I will do so.
> 
> 
> regards,
> -myke
> 
> On Feb 9, 2010, at 1:57 PM, Maynard Johnson wrote:
> 
>> Myke Smith wrote:
>>> Found and fixed a bug in opcontrol.
>>>
>>> This occurred if you chose not to identify any kernel symbols, did not
>>> set --use-kallsyms. This caused a hang in opcontrol get_image().
>>>
>>> The attached patch has everything in the previous patch plus this fix.
>>> Not sure if this is the appropriate way or if you want a patch to just
>>> opcontrol. If you just want a patch to opcontrol let me know and I will
>>> make it.
>>>
>>>
>>>
>>>
>>> Signed-off-by: Myke Smith <myke.smith@...>
>>>
>>>
>>> On Jan 28, 2010, at 10:44 AM, Myke Smith wrote:
>>
>> I apologize for the delay in reviewing this patch.  Other priorities 
>> got in the
>> way.  Thanks for the submission.  Comments embedded below.
>>
>> -Maynard
>>
>>>
>>>>
>>>> On hand held devices we are -typically- very memory constrained and
>>>> as such the standard vmlinux image is not available. The attached
>>>> patch re-uses and re-implements some of the ideas in Brian Eaton's
>>>> patch from the summer of 2006.
>>
>> Adding the "Signed-off-by" line below, implies you have read and are 
>> complying
>> with the "Developer's Certificate of Origin 1.1" (DCO) from
>> http://lwn.net/Articles/139918/.  Since by your own admission, this 
>> patch is
>> based on an original patch from Brian Eaton, you need to get Brian to 
>> sign off
>> on this patch.
>>
>>>>
>>>> Specifically it adds a flag to opcontrol --use-kallsyms which in
>>>> combination with --vm-nolinux  passes /proc/kallsyms to oprofiled.
>> I presume you mean "--no-vmlinux".
>>>>
>>>> There are modifications to libutil++/op_bfd.* to detect the use of
>>>> /proc/kallsyms and complementary routines to the standard BFD based
>>>> routines to handle the text versus binary file.
>>>>
>>>>
>>>>
>>>>
>>>> Signed-off-by: Myke Smith <myke.smith@...>
>>>>
>>> Index: oprofile/ChangeLog
>>> ===================================================================
>>
>> Your patch does not compile for me.  I get the following error:
>>
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
>>
>> make[3]: Entering directory `/home/mpj/oprof-work/op-tmp/libutil++'
>> if g++ -DHAVE_CONFIG_H -I. -I. -I..  -I ../libutil -I ../libop -I 
>> ../libpp  -W
>> -Wall -fno-common -ftemplate-depth-50 -Werror -g -O2 -MT op_bfd.o -MD 
>> -MP -MF
>> ".deps/op_bfd.Tpo" -c -o op_bfd.o op_bfd.cpp; \
>> then mv -f ".deps/op_bfd.Tpo" ".deps/op_bfd.Po"; else rm -f 
>> ".deps/op_bfd.Tpo";
>> exit 1; fi
>> cc1plus: warnings being treated as errors
>> op_bfd.h: In constructor ?op_bfd::op_bfd(const std::string&, const
>> string_filter&, const extra_images&, bool&)?:
>> op_bfd.h:332: warning: ?op_bfd::section_vma? will be initialized after
>> op_bfd.h:329: warning:   ?long unsigned int op_bfd::section_filepos?
>> op_bfd.cpp:114: warning:   when initialized here
>> make[3]: *** [op_bfd.o] Error 1
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
>>
>>
>> Member initializers are executed in the order in which they are found 
>> in the
>> class defintiion.  The warning message is telling you that the order 
>> in which
>> you coded the member initializers in the class constructor differs 
>> from that
>> found in the class definition.  If you patched and built this with an 
>> oprofile
>> cvs checkout, the warning should have caused a compile error for you.  
>> Maybe
>> your C++ compiler just isn't catching this.
>>
>> After fixing the above compile error, I ran into another one.  This 
>> second error
>> was only failing on a more recent g++ version (4.3):
>>
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
>>
>> make[3]: Entering directory `/home/mpj/temp/oprofile/libutil++'
>> g++ -DHAVE_CONFIG_H -I. -I..  -I ../libutil -I ../libop -I ../libpp  
>> -W -Wall
>> -fno-common -ftemplate-depth-50 -Werror -g -O2 -MT op_bfd.o -MD -MP -MF
>> .deps/op_bfd.Tpo -c -o op_bfd.o op_bfd.cpp
>> cc1plus: warnings being treated as errors
>> op_bfd.cpp: In member function 'bool op_bfd::parse_line_sysmap(const
>> std::string&, bfd_vma&, char&, std::string&)':
>> op_bfd.cpp:369: error: format '%Lx' expects type 'long long unsigned 
>> int*', but
>> argument 3 has type 'bfd_vma*'
>> make[3]: *** [op_bfd.o] Error 1
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
>>
>>
>> After fixing this compile error, I was able to build oprofile OK.
>>
>> Some general comments:
>>
>> 1. The new opcontrol option for using kallsyms must be documented in 
>> both the
>> opcontrol man page and in the oprofile user manual (doc/oprofile.xml).
>> 2. Once set, there's no way to change your new option.  So instead of
>> '--use-kallsyms', perhaps it should be '--use-kallsys=[y | n]'.
>> 3. Should it be an illegal operation to set '--vmlinux' and 
>> '--use-kallsyms' at
>> the same time?  Currently, it's not.
>> 4. I think there should be a new op_bfd constructor for the 
>> /proc/kallsyms.
>> Inserting all the special case code into the existing ctor is messy.  
>> Besides, I
>> don't think there's a need for extra_images to be passed for a 
>> "/proc/kallsyms"
>> op_bfd ctor.  Or am I wrong about that?
>> 5. Please look at all of your comments and documentation and clean 
>> them up for
>> grammar and clarity.
>>
>>
>>> RCS file: /cvsroot/oprofile/oprofile/ChangeLog,v
>>> retrieving revision 1.1918
>>> diff -p -a -u -r1.1918 ChangeLog
>>> --- oprofile/ChangeLog        26 Jan 2010 14:42:00 -0000      1.1918
>>> +++ oprofile/ChangeLog        29 Jan 2010 00:21:59 -0000
>>> @@ -1,3 +1,19 @@
>>> +2010-01-28  Myke Smith <myke.smith@...>
>>> +     * utils/opcontrol
>>> +      Changes made to support using /proc/kallsyms as a source of 
>>> kernel symbols.
>>> +      New flag added --use-kallsyms and it requires --no-vmlinux to 
>>> be active.
>>> +
>>> +     * libpp/poulate.cpp
>>> +       If the file is /proc/kallsyms ignore mtime check since this 
>>> file can be
>>> +             updated at any time with modules being loaded/unloaded
>>> +
>>> +     * libutils++/op_bfd.cpp
>>> +       Added a constructor to handle /proc/kallsyms.
>>> +       Added support routines for parsing and retrieving symbols from
>>> +             /proc/kallsyms.
>>> +       Added a boolean "sysmap" which when set short-circuits somei 
>>> tests that are
>>> +             BFD oriented.
>>> +
>>> 2010-01-20  Maynard Johnson  <maynardj@...>
>>>
>>>      * m4/qt.m4: Fix qt lib check so it works on base 64-bit system
>>> Index: oprofile/libpp/populate.cpp
>>> ===================================================================
>>> RCS file: /cvsroot/oprofile/oprofile/libpp/populate.cpp,v
>>> retrieving revision 1.14
>>> diff -p -a -u -r1.14 populate.cpp
>>> --- oprofile/libpp/populate.cpp       21 Jan 2008 21:35:17 -0000      
>>> 1.14
>>> +++ oprofile/libpp/populate.cpp       29 Jan 2010 00:21:59 -0000
>>> @@ -102,6 +102,9 @@ populate_for_image(profile_container & s
>>>              string filename =
>>>                      samples.extra_found_images.find_image_path(
>>>                              ip.image, error, true);
>>> +
>>> +             // don't check mtime for proc/kallsyms cause always 
>>> changing with modules in & out
>>> +             if (filename.compare("/proc/kallsyms") != 0)
>>>              check_mtime(filename, header);
>>                 ^--- indentation needed
>>>      }
>>>
>>> Index: oprofile/libutil++/bfd_support.h
>>> ===================================================================
>>> RCS file: /cvsroot/oprofile/oprofile/libutil++/bfd_support.h,v
>>> retrieving revision 1.9
>>> diff -p -a -u -r1.9 bfd_support.h
>>> --- oprofile/libutil++/bfd_support.h  26 Oct 2009 13:01:09 -0000      
>>> 1.9
>>> +++ oprofile/libutil++/bfd_support.h  29 Jan 2010 00:21:59 -0000
>>> @@ -117,6 +117,10 @@ bfd * open_bfd(std::string const & file)
>>> /// open the given BFD from the fd
>>> bfd * fdopen_bfd(std::string const & file, int fd);
>>>
>>> +/// open the given BFD from -in this SPECIFIC case is a map file and 
>>> not binary- the fd
>>> +/// different from above in that we do not do a format_check()
>>> +bfd * fdopen_map(std::string const & file, int fd);
>>> +
>>> /// Return a BFD for an SPU ELF embedded in PPE binary file
>>> bfd * spu_open_bfd(std::string const name, int fd, uint64_t 
>>> offset_to_spu_elf);
>>>
>>> Index: oprofile/libutil++/op_bfd.cpp
>>> ===================================================================
>>> RCS file: /cvsroot/oprofile/oprofile/libutil++/op_bfd.cpp,v
>>> retrieving revision 1.87
>>> diff -p -a -u -r1.87 op_bfd.cpp
>>> --- oprofile/libutil++/op_bfd.cpp     20 Jul 2009 13:24:47 -0000      
>>> 1.87
>>> +++ oprofile/libutil++/op_bfd.cpp     29 Jan 2010 00:21:59 -0000
>>> @@ -24,6 +24,9 @@
>>> #include <iostream>
>>> #include <iomanip>
>>> #include <sstream>
>>> +// for sysmap
>>> +#include <errno.h>
>>> +#include <fstream>
>>>
>>> #include "op_bfd.h"
>>> #include "locate_images.h"
>>> @@ -85,6 +88,16 @@ op_bfd_symbol::op_bfd_symbol(bfd_vma vma
>>> {
>>> }
>>>
>>> +// for sysmap
>>> +op_bfd_symbol::op_bfd_symbol(bfd_vma vma, size_t size, string const 
>>> & name,
>>> +                             bfd_vma sectionvma, unsigned long 
>>> sectionfilepos)
>>> +     : bfd_symbol(0), symb_value(vma),
>>> +       section_filepos(sectionfilepos), section_vma(sectionvma),
>>> +       symb_size(size), symb_name(name),
>>> +       symb_hidden(false), symb_weak(false), symb_artificial(false)
>>> +{
>>> +}
>>> +
>>>
>>> bool op_bfd_symbol::operator<(op_bfd_symbol const & rhs) const
>>> {
>>> @@ -104,7 +117,11 @@ op_bfd::op_bfd(string const & fname, str
>>>      archive_path(extra_images.get_archive_path()),
>>>      extra_found_images(extra_images),
>>>      file_size(-1),
>>> -     anon_obj(false)
>>> +     anon_obj(false),
>>> +     sysmap(false),
>>> +     section_vma(0),
>>
>> Compilation error --^
>>
>>> +     section_filepos(0),
>>> +     arch_len(0)
>>> {
>>>      int fd;
>>>      struct stat st;
>>> @@ -116,10 +133,16 @@ op_bfd::op_bfd(string const & fname, str
>>>      string suf = ".jo";
>>>
>>>      image_error img_ok;
>>> +
>>> +     if ((filename.compare("/proc/kallsyms") == 0)) {
>>> +             sysmap = true;
>>> +             cverb << vbfd << "sysmap enabled by  " << filename << 
>>> endl;
>>> +     }
>>> +
>>>      string const image_path =
>>>              extra_images.find_image_path(filename, img_ok, true);
>>>
>>> -     cverb << vbfd << "op_bfd ctor for " << image_path << endl;
>>> +     cverb << vbfd << "op_bfd ctor for " << image_path << " filename 
>>> was " << filename << endl;
>>>
>>>      // if there's a problem already, don't try to open it
>>>      if (!ok || img_ok != image_ok) {
>>> @@ -127,6 +150,12 @@ op_bfd::op_bfd(string const & fname, str
>>>              goto out_fail;
>>>      }
>>>
>>> +     /* Since the sysmap is NOT a binary file we can't use some of 
>>> the functions *
>>> +      * to get and add symbols. So we will call sysmap specific 
>>> versions to do   *
>>> +      * 
>>> work                                                                     
>>> */
>>> +     if (sysmap) goto sysmap_image;
>>> +
>>> +
>>>      fd = open(image_path.c_str(), O_RDONLY);
>>>      if (fd == -1) {
>>>              cverb << vbfd << "open failed for " << image_path << endl;
>>> @@ -184,6 +213,126 @@ out_fail:
>>>      // make the fake symbol fit within the fake file
>>>      file_size = -1;
>>>      goto out;
>>> +
>>> +sysmap_image:
>>> +     // First find the start and end of the text segment.
>>> +     bfd_vma stext = 0;
>>> +     bfd_vma etext = 0;
>>> +
>>> +     ifstream in(image_path.c_str());
>>> +     if (!in) {
>>> +             cverb << vbfd << "Unable to open '" << image_path <<
>>> +                   "': " << strerror(errno) << endl;
>>> +             ok = false;
>>> +             goto out_fail;
>>> +     }
>>> +     string sysmap_line;
>>> +     while (getline(in, sysmap_line)) {
>>> +             bfd_vma sysmap_addr;
>>> +             string sysmap_name;
>>> +             char sysmap_type;
>>> +             if (!parse_line_sysmap(sysmap_line, sysmap_addr, 
>>> sysmap_type, sysmap_name)) {
>>> +                     cverb << vbfd << "Unexpected input on symbol 
>>> line '" <<
>>> +                             sysmap_line << "' in /proc/kallsyms " <<
>>> +                             image_path << endl;
>>> +                     ok = false;
>>> +                     goto out_fail;
>>> +             }
>>> +
>>> +             if (   (  (sysmap_name == "_text")
>>> +                    || (sysmap_name == "_stext"))
>>> +                 && (stext == 0)) {
>>> +                     stext = sysmap_addr;
>>> +
>>> +             } else if (sysmap_name == "_etext")
>>> +                     etext = sysmap_addr;
>>> +
>>> +             // We keep parsing even if we have both _text/_stext and
>>> +             // _etext, just to make sure the file looks right.
>>> +
>>> +
>>> +
>>> +     }
>>> +     if (in.bad()) {
>>> +             cverb << vbfd << "Error on symbol line '" << 
>>> sysmap_line <<
>>> +                     "' in /proc/kallsyms " << image_path << ": " <<
>>> +                     strerror(errno) << endl;
>>> +             ok = false;
>>> +             goto out_fail;
>>> +     }
>>> +
>>> +     if (stext == 0 || etext == 0) {
>>> +             cverb << vbfd << "Unable to find _text and _etext in " 
>>> << image_path << endl;
>>> +             ok = false;
>>> +             goto out_fail;
>>> +     }
>>> +
>>> +     section_vma = stext;
>>> +
>>> +     // Read all of the code symbols in the text segment
>>> +     int number_symbols = 0;
>>> +     in.clear();
>>> +     in.seekg(0, ios::beg);
>>> +     string prevname;
>>> +     bfd_vma prevaddr = 0;
>>> +     while (getline(in, sysmap_line)) {
>>> +             bfd_vma sysmap_addr;
>>> +             string sysmap_name;
>>> +             char sysmap_type;
>>> +             if (!parse_line_sysmap(sysmap_line, sysmap_addr, 
>>> sysmap_type, sysmap_name)) {
>>> +                     cverb << vbfd << "Unexpected input on symbol 
>>> line '" <<
>>> +                             sysmap_line << "' in " << image_path << 
>>> endl;
>>> +                     ok = false;
>>> +                     goto out_fail;
>>> +             }
>>> +             if (sysmap_addr < stext)
>>> +                     continue;
>>> +             if (sysmap_addr > etext)
>>> +                     break;
>>> +
>>> +             // Can have multiple symbols of different types at same 
>>> VMA,
>>> +             // we only want to complete a code symbol if we've moved
>>> +             // on to the next VMA.  If there are multiple code symbols
>>> +             // at the same VMA, we use the last symbol.
>>> +
>>> +             // This covers the boring_symbols routine for BFD images
>>> +             if (prevaddr != 0 && prevaddr != sysmap_addr) {
>>> +
>>> +                     add_sym_sysmap(symbol_filter, prevname, 
>>> prevaddr, sysmap_addr);
>>> +                     prevname.clear();
>>> +                     prevaddr = 0;
>>> +                     number_symbols++;
>>> +             }
>>> +
>>> +             if (sysmap_type == 'T' || sysmap_type == 't') {
>>> +                     // it's code
>>> +                     // but we want to remove uninteresting symbols
>>> +                     // ARM assembler internal mapping symbols
>>> +                     // C++ exception stuff
>>> +
>>> +                     // taken from the routine interesting_symbols 
>>> for BFD images
>>> +                     if  (   (sysmap_name != "$a")
>>> +                          && (sysmap_name != "$t")
>>> +                          && (sysmap_name != "$d")
>>> +                          && (sysmap_name != ".L")) {
>>> +
>>> +                             prevname = sysmap_name;
>>> +                             prevaddr = sysmap_addr;
>>> +                     }
>>> +             }
>>> +     }
>>> +
>>> +     cverb << vbfd << "stext:" << hex << stext << " etext:" << hex 
>>> << etext << " added "
>>> +           << dec << number_symbols << " symbols from " << 
>>> image_path << endl;
>>> +
>>> +
>>> +     arch_len = 64;
>>> +     if (stext < 0xffffffff)
>>> +             arch_len = 32;
>>> +
>>> +     ok = true;
>>> +
>>> +     return;
>>> }
>>>
>>>
>>> @@ -194,16 +343,53 @@ op_bfd::~op_bfd()
>>>
>>> unsigned long op_bfd::get_start_offset(bfd_vma vma) const
>>> {
>>> +     if (sysmap) {
>>> +             if (!vma || vma == section_vma)
>>> +                     return section_filepos;
>>> +             return 0;
>>> +     } else {
>>>      if (!vma || !ibfd.valid()) {
>>>              filepos_map_t::const_iterator it = 
>>> filepos_map.find(".text");
>>>              if (it != filepos_map.end())
>>>                      return it->second;
>>>              return 0;
>>>      }
>>
>> Please indent this under the new "else" clause.
>>
>>> +     }
>>>
>>>      return 0;
>>> }
>>>
>>> +//  sysmap specific function
>>> +bool op_bfd::parse_line_sysmap(string const & line, bfd_vma & addr, 
>>> char & type, string & name)
>>> +{
>>> +     char * sym = NULL;
>>> +     char * end = NULL;
>>> +     int c = -1;
>>> +#ifdef BFD64
>>> +     c = sscanf(line.c_str(), "%Lx %c %as %as", &addr, &type, &sym, 
>>> &end);
>>
>> Compilation error . . . . . . . --^
>>
>>> +#else
>>> +     c = sscanf(line.c_str(), "%lx %c %as %as", &addr, &type, &sym, 
>>> &end);
>>> +#endif
>>> +     if (c < 3) {
>>> +             if (sym)
>>> +                     free(sym);
>>> +             return false;
>>> +     }
>>> +     name = sym;
>>> +     free(sym);
>>> +     return true;
>>> +}
>>> +
>>> +//  sysmap specific function
>>> +//  the ctor sets all the sizes and several BFD type of symbols
>>> +void op_bfd::add_sym_sysmap(string_filter const & symbol_filter,
>>> +                     string const & name, bfd_vma start, bfd_vma end)
>>
>>                                        ^-- maybe change to 
>> 'previous_name'?
>>
>>> +{
>>> +     if (symbol_filter.match(name)) {
>>> +             syms.push_back(op_bfd_symbol(start-section_vma, 
>>> end-start, name,
>>> +                               section_vma, section_filepos));
>>> +     }
>>> +}
>>>
>>> void op_bfd::get_symbols(op_bfd::symbols_found_t & symbols)
>>> {
>> [snip]
>>
>