Re: [PATCH] Support reading /proc/kallsyms if no vmlinux is available (version 3)

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

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 <myk...@pa...>
>>
>>
>> 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 <myk...@pa...>
>>>
>> 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 <myk...@pa...>
>> +     * 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  <may...@us...>
>>
>>      * 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]
>