From: graydon h. <gr...@re...> - 2003-06-30 20:14:30
|
hi, the following is a patch which adds support for looking up debugging symbols in "separated debuginfo files", which appear in newer versions of RHL (possibly elsewhere?). they're ELF files which have the debugging sections (but none of the .text sections) of some other ELF file. they can be installed separately, from a -debuginfo RPM, so when you realize you *want* to profile or debug a file, you just install its -debuginfo RPM and run the debugger or profiler again. the connection between the "main" binary and its debuginfo file is made via some well defined name mangling and a crc32 of the debuginfo file which is embedded in the main file. comments / corrections? ok to apply? -graydon Index: configure.in =================================================================== RCS file: /cvsroot/oprofile/oprofile/configure.in,v retrieving revision 1.171 diff -u -r1.171 configure.in --- configure.in 17 Jun 2003 20:00:16 -0000 1.171 +++ configure.in 30 Jun 2003 20:01:43 -0000 @@ -98,6 +98,20 @@ fi AM_CONDITIONAL(enable_abi, test "$enable_abi" = yes) + +dnl enable code to handle separate debug info +debugdir=/usr/lib/debug + +AC_ARG_WITH(separate-debug-dir, +[ --with-separate-debug-dir=path Look for global separate debug info in this path [LIBDIR/debug]], +[debugdir="${withval}"]) + +if test "z$debugdir" == "zyes"; then + debugdir=/usr/lib/debug +fi + +AC_DEFINE_UNQUOTED([DEBUGDIR], "$debugdir", [Directory with separate debug info files]) + AX_CHECK_DOCBOOK dnl finally restore the original libs setting Index: libutil++/op_bfd.cpp =================================================================== RCS file: /cvsroot/oprofile/oprofile/libutil++/op_bfd.cpp,v retrieving revision 1.26 diff -u -r1.26 op_bfd.cpp --- libutil++/op_bfd.cpp 12 Jun 2003 01:21:53 -0000 1.26 +++ libutil++/op_bfd.cpp 30 Jun 2003 20:01:43 -0000 @@ -17,8 +17,10 @@ #include <algorithm> #include <iostream> +#include <fstream> #include <iomanip> #include <sstream> +#include <assert.h> #include "op_exception.h" #include "op_bfd.h" @@ -28,12 +30,215 @@ extern ostream cverb; -op_bfd_symbol::op_bfd_symbol(asymbol const * a) +unsigned long +calc_crc32 (unsigned long crc, unsigned char *buf, size_t len) +{ + static const unsigned long crc32_table[256] = + { + 0x00000000, 0x77073096, 0xee0e612c, 0x990951ba, 0x076dc419, + 0x706af48f, 0xe963a535, 0x9e6495a3, 0x0edb8832, 0x79dcb8a4, + 0xe0d5e91e, 0x97d2d988, 0x09b64c2b, 0x7eb17cbd, 0xe7b82d07, + 0x90bf1d91, 0x1db71064, 0x6ab020f2, 0xf3b97148, 0x84be41de, + 0x1adad47d, 0x6ddde4eb, 0xf4d4b551, 0x83d385c7, 0x136c9856, + 0x646ba8c0, 0xfd62f97a, 0x8a65c9ec, 0x14015c4f, 0x63066cd9, + 0xfa0f3d63, 0x8d080df5, 0x3b6e20c8, 0x4c69105e, 0xd56041e4, + 0xa2677172, 0x3c03e4d1, 0x4b04d447, 0xd20d85fd, 0xa50ab56b, + 0x35b5a8fa, 0x42b2986c, 0xdbbbc9d6, 0xacbcf940, 0x32d86ce3, + 0x45df5c75, 0xdcd60dcf, 0xabd13d59, 0x26d930ac, 0x51de003a, + 0xc8d75180, 0xbfd06116, 0x21b4f4b5, 0x56b3c423, 0xcfba9599, + 0xb8bda50f, 0x2802b89e, 0x5f058808, 0xc60cd9b2, 0xb10be924, + 0x2f6f7c87, 0x58684c11, 0xc1611dab, 0xb6662d3d, 0x76dc4190, + 0x01db7106, 0x98d220bc, 0xefd5102a, 0x71b18589, 0x06b6b51f, + 0x9fbfe4a5, 0xe8b8d433, 0x7807c9a2, 0x0f00f934, 0x9609a88e, + 0xe10e9818, 0x7f6a0dbb, 0x086d3d2d, 0x91646c97, 0xe6635c01, + 0x6b6b51f4, 0x1c6c6162, 0x856530d8, 0xf262004e, 0x6c0695ed, + 0x1b01a57b, 0x8208f4c1, 0xf50fc457, 0x65b0d9c6, 0x12b7e950, + 0x8bbeb8ea, 0xfcb9887c, 0x62dd1ddf, 0x15da2d49, 0x8cd37cf3, + 0xfbd44c65, 0x4db26158, 0x3ab551ce, 0xa3bc0074, 0xd4bb30e2, + 0x4adfa541, 0x3dd895d7, 0xa4d1c46d, 0xd3d6f4fb, 0x4369e96a, + 0x346ed9fc, 0xad678846, 0xda60b8d0, 0x44042d73, 0x33031de5, + 0xaa0a4c5f, 0xdd0d7cc9, 0x5005713c, 0x270241aa, 0xbe0b1010, + 0xc90c2086, 0x5768b525, 0x206f85b3, 0xb966d409, 0xce61e49f, + 0x5edef90e, 0x29d9c998, 0xb0d09822, 0xc7d7a8b4, 0x59b33d17, + 0x2eb40d81, 0xb7bd5c3b, 0xc0ba6cad, 0xedb88320, 0x9abfb3b6, + 0x03b6e20c, 0x74b1d29a, 0xead54739, 0x9dd277af, 0x04db2615, + 0x73dc1683, 0xe3630b12, 0x94643b84, 0x0d6d6a3e, 0x7a6a5aa8, + 0xe40ecf0b, 0x9309ff9d, 0x0a00ae27, 0x7d079eb1, 0xf00f9344, + 0x8708a3d2, 0x1e01f268, 0x6906c2fe, 0xf762575d, 0x806567cb, + 0x196c3671, 0x6e6b06e7, 0xfed41b76, 0x89d32be0, 0x10da7a5a, + 0x67dd4acc, 0xf9b9df6f, 0x8ebeeff9, 0x17b7be43, 0x60b08ed5, + 0xd6d6a3e8, 0xa1d1937e, 0x38d8c2c4, 0x4fdff252, 0xd1bb67f1, + 0xa6bc5767, 0x3fb506dd, 0x48b2364b, 0xd80d2bda, 0xaf0a1b4c, + 0x36034af6, 0x41047a60, 0xdf60efc3, 0xa867df55, 0x316e8eef, + 0x4669be79, 0xcb61b38c, 0xbc66831a, 0x256fd2a0, 0x5268e236, + 0xcc0c7795, 0xbb0b4703, 0x220216b9, 0x5505262f, 0xc5ba3bbe, + 0xb2bd0b28, 0x2bb45a92, 0x5cb36a04, 0xc2d7ffa7, 0xb5d0cf31, + 0x2cd99e8b, 0x5bdeae1d, 0x9b64c2b0, 0xec63f226, 0x756aa39c, + 0x026d930a, 0x9c0906a9, 0xeb0e363f, 0x72076785, 0x05005713, + 0x95bf4a82, 0xe2b87a14, 0x7bb12bae, 0x0cb61b38, 0x92d28e9b, + 0xe5d5be0d, 0x7cdcefb7, 0x0bdbdf21, 0x86d3d2d4, 0xf1d4e242, + 0x68ddb3f8, 0x1fda836e, 0x81be16cd, 0xf6b9265b, 0x6fb077e1, + 0x18b74777, 0x88085ae6, 0xff0f6a70, 0x66063bca, 0x11010b5c, + 0x8f659eff, 0xf862ae69, 0x616bffd3, 0x166ccf45, 0xa00ae278, + 0xd70dd2ee, 0x4e048354, 0x3903b3c2, 0xa7672661, 0xd06016f7, + 0x4969474d, 0x3e6e77db, 0xaed16a4a, 0xd9d65adc, 0x40df0b66, + 0x37d83bf0, 0xa9bcae53, 0xdebb9ec5, 0x47b2cf7f, 0x30b5ffe9, + 0xbdbdf21c, 0xcabac28a, 0x53b39330, 0x24b4a3a6, 0xbad03605, + 0xcdd70693, 0x54de5729, 0x23d967bf, 0xb3667a2e, 0xc4614ab8, + 0x5d681b02, 0x2a6f2b94, 0xb40bbe37, 0xc30c8ea1, 0x5a05df1b, + 0x2d02ef8d + }; + unsigned char *end; + + crc = ~crc & 0xffffffff; + for (end = buf + len; buf < end; ++buf) + crc = crc32_table[(crc ^ *buf) & 0xff] ^ (crc >> 8); + return ~crc & 0xffffffff;; +} + +static bool +separate_debug_file_exists(string const & name, + unsigned long const crc) +{ + unsigned long file_crc = 0; + char buffer[8*1024]; + + ifstream file(name.c_str()); + if (!file) + return false; + + cverb << "found " << name; + while (file) { + file.read(buffer, sizeof(buffer)); + file_crc = calc_crc32(file_crc, + reinterpret_cast<unsigned char *>(&buffer[0]), + file.gcount()); + } + cverb << " with crc32 = " << hex << file_crc << endl; + return crc == file_crc; +} + + +static bool +get_debug_link_info (bfd * ibfd, + string & filename_out, + unsigned long & crc32_out) +{ + asection * sect; + + cverb << "fetching .gnu_debuglink section" << endl; + sect = bfd_get_section_by_name(ibfd, ".gnu_debuglink"); + + if (sect == NULL) + return false; + + bfd_size_type debuglink_size = bfd_section_size(ibfd, sect); + char contents[debuglink_size]; + cverb << ".gnu_debuglink section has size " << debuglink_size << endl; + + bfd_get_section_contents(ibfd, sect, + reinterpret_cast<unsigned char *>(contents), + static_cast<file_ptr>(0), debuglink_size); + + /* Crc value is stored after the filename, aligned up to 4 bytes. */ + size_t filename_len = strlen(contents); + size_t crc_offset = filename_len + 1; + crc_offset = (crc_offset + 3) & ~3; + + crc32_out = bfd_get_32(ibfd, + reinterpret_cast<bfd_byte *>(contents + crc_offset)); + filename_out = string(contents, filename_len); + cverb << ".gnu_debuglink filename is " << filename_out << endl; + return true; +} + + +static bool +find_separate_debug_file (bfd * ibfd, + string const & dir_in, + string const & global_in, + string & filename_out) +{ + string dir(dir_in); + string global(global_in); + string basename; + unsigned long crc32; + + if (! get_debug_link_info(ibfd, basename, crc32)) + return false; + + if (dir.size() > 0 && dir.at(dir.size() - 1) != '/') + dir += '/'; + + if (global.size() > 0 && global.at(global.size() - 1) != '/') + global += '/'; + + cverb << "looking for debugging file " << basename + << " with crc32 = " << hex << crc32 << endl; + + string first_try(dir + basename); + string second_try(dir + ".debug/" + basename); + + if (dir.size() > 0 && dir[0] == '/') + dir = dir.substr(1); + + string third_try(global + dir + basename); + + if (separate_debug_file_exists(first_try, crc32)) + filename_out = first_try; + else if (separate_debug_file_exists(second_try, crc32)) + filename_out = second_try; + else if (separate_debug_file_exists(third_try, crc32)) + filename_out = third_try; + else + return false; + + return true; +} + +static bfd * open_bfd(string const & file) +{ + bfd * ibfd = bfd_openr(file.c_str(), NULL); + if (!ibfd) { + cerr << "bfd_openr of " << file << " failed." << endl; + cerr << strerror(errno) << endl; + exit(EXIT_FAILURE); + } + char ** matching; + if (!bfd_check_format_matches(ibfd, bfd_object, &matching)) { + cerr << "BFD format failure for " << file << endl; + exit(EXIT_FAILURE); + } + return ibfd; +} + + +void op_bfd::add_to_section_map(bfd * abfd, asection * sect, PTR obj) +{ + op_bfd * the_bfd = reinterpret_cast<op_bfd *>(obj); + assert(abfd); + assert(the_bfd); + assert(sect); + assert(sect->name); + the_bfd->section_offsets[sect->name] = sect->filepos; +} + +op_bfd_symbol::op_bfd_symbol(asymbol const * a, + std::map<std::string,u32> const & section_offsets) : bfd_symbol(a), symb_value(a->value), section_filepos(a->section->filepos), section_vma(a->section->vma), symb_size(0) { + + // it's possible we're looking at a symbol from separated + // debuginfo, in which case the section_filepos is wrong. + // need to check against the section offset map + + if (section_offsets.find(a->section->name) != + section_offsets.end()) + section_filepos = section_offsets.find(a->section->name)->second; + // Some sections have unnamed symbols in them. If // we just ignore them then we end up sticking // things like .plt hits inside of _init. So instead @@ -91,6 +296,18 @@ cverb << ".text filepos " << hex << text_offset << endl; } + bfd_map_over_sections (ibfd, add_to_section_map, this); + + string global(DEBUGDIR); + string dirname(filename.substr(0, filename.rfind('/'))); + string debug_filename; + off_t dummy_file_size; + if (find_separate_debug_file (ibfd, dirname, global, debug_filename)) { + cverb << "now loading: " << debug_filename << endl; + op_get_fsize(debug_filename.c_str(), &dummy_file_size); + ibfd = open_bfd(debug_filename); + } + for (sect = ibfd->sections; sect; sect = sect->next) { if (sect->flags & SEC_DEBUGGING) { debug_info = true; @@ -232,7 +449,8 @@ for (symbol_index_t i = 0; i < nr_all_syms; i++) { if (interesting_symbol(bfd_syms[i])) { - symbols.push_back(op_bfd_symbol(bfd_syms[i])); + symbols.push_back(op_bfd_symbol(bfd_syms[i], + section_offsets)); } } Index: libutil++/op_bfd.h =================================================================== RCS file: /cvsroot/oprofile/oprofile/libutil++/op_bfd.h,v retrieving revision 1.18 diff -u -r1.18 op_bfd.h --- libutil++/op_bfd.h 31 May 2003 18:21:41 -0000 1.18 +++ libutil++/op_bfd.h 30 Jun 2003 20:01:43 -0000 @@ -19,6 +19,7 @@ #include <vector> #include <string> #include <list> +#include <map> #include "utility.h" #include "op_types.h" @@ -39,7 +40,8 @@ public: /// ctor for real symbols - op_bfd_symbol(asymbol const * a); + op_bfd_symbol(asymbol const * a, + std::map<std::string,u32> const & section_offsets); /// ctor for artificial symbols op_bfd_symbol(bfd_vma vma, size_t size, std::string const & name); @@ -168,6 +170,12 @@ /// true if at least one section has (flags & SEC_DEBUGGING) != 0 bool debug_info; + + // section offsets in original file; these may change if we move + // to a debuginfo BFD but we should use the original file offsets + // when constructing symbols. + std::map<std::string,u32> section_offsets; + static void add_to_section_map (bfd *abfd, asection *sect, PTR obj); /// temporary container for getting symbols typedef std::list<op_bfd_symbol> symbols_found_t; |
From: Philippe E. <ph...@wa...> - 2003-07-02 08:07:47
|
graydon hoare wrote: > hi, > > the following is a patch which adds support for looking up debugging > symbols in "separated debuginfo files", which appear in newer versions > of RHL (possibly elsewhere?). they're ELF files which have the is this already in production distro or only as alpha code ? > debugging sections (but none of the .text sections) of some other ELF > file. they can be installed separately, from a -debuginfo RPM, so when > you realize you *want* to profile or debug a file, you just install > its -debuginfo RPM and run the debugger or profiler again. > > the connection between the "main" binary and its debuginfo file is > made via some well defined name mangling and a crc32 of the debuginfo > file which is embedded in the main file. "well defined" ? reading the code I'm unusure of that. Elaborate please I'm also confused by the name debuginfo, is opannotate --source working with this patch or is it working only for symbol name information ? > > comments / corrections? ok to apply? I think it's a good idea to support separate debug info but I prefer John comment this idea. > Index: libutil++/op_bfd.cpp > -op_bfd_symbol::op_bfd_symbol(asymbol const * a) > +unsigned long > +calc_crc32 (unsigned long crc, unsigned char *buf, size_t len) > +{ > + static const unsigned long crc32_table[256] = > + { > + 0x00000000, 0x77073096, 0xee0e612c, 0x990951ba, 0x076dc419, ... > + 0x2d02ef8d > + }; > + unsigned char *end; > + > + crc = ~crc & 0xffffffff; > + for (end = buf + len; buf < end; ++buf) > + crc = crc32_table[(crc ^ *buf) & 0xff] ^ (crc >> 8); > + return ~crc & 0xffffffff;; spurious ; > +} calc_crc32 in a separate file but I don't like the fact we need to crc32 the whole debug info file, these files can be numberous and voluminous, if this feature is not already in RH production distro can I ask the debuginfo file is augmented with a section containing the check sum, then we don't need to recalcultate the check sum but just compare the checksum in .gnu_debuglink and the checksum in the debug file. It'll speed up a lot operation like opreport -l. Also is this features added to binutils or need a separate RH only tools ? It'll be usefull to add it to ld or via a new utility in binutils. > + > +static bool > +find_separate_debug_file (bfd * ibfd, > + string const & dir_in, > + string const & global_in, > + string & filename_out) > +{ > + string dir(dir_in); > + string global(global_in); > + string basename; > + unsigned long crc32; > + > + if (! get_debug_link_info(ibfd, basename, crc32)) > + return false; > + > + if (dir.size() > 0 && dir.at(dir.size() - 1) != '/') > + dir += '/'; > + > + if (global.size() > 0 && global.at(global.size() - 1) != '/') > + global += '/'; > + > + cverb << "looking for debugging file " << basename > + << " with crc32 = " << hex << crc32 << endl; > + > + string first_try(dir + basename); first_try look like the same as the original binary name, or perhaps I'm mis-reading the code ? > + string second_try(dir + ".debug/" + basename); > + > + if (dir.size() > 0 && dir[0] == '/') > + dir = dir.substr(1); > + > + string third_try(global + dir + basename); third_try seem evil except if .gnu_debuglink contain the full pathname minus leading '/' > + > + if (separate_debug_file_exists(first_try, crc32)) > + filename_out = first_try; > + else if (separate_debug_file_exists(second_try, crc32)) > + filename_out = second_try; > + else if (separate_debug_file_exists(third_try, crc32)) > + filename_out = third_try; > + else > + return false; Ok, repeating myself a bit: it seems evil than to support *one* distro we need to handle three different name encoding. > + > + > +void op_bfd::add_to_section_map(bfd * abfd, asection * sect, PTR obj) (obj == this) remove this paramater please > +{ > + op_bfd * the_bfd = reinterpret_cast<op_bfd *>(obj); the_bfd == this > + assert(abfd); > + assert(the_bfd); remove it > + assert(sect); > + assert(sect->name); > + the_bfd->section_offsets[sect->name] = sect->filepos; section_offset[sect->name] = sect->filepos; > +} > + |
From: John L. <le...@mo...> - 2003-07-02 12:17:09
|
On Wed, Jul 02, 2003 at 10:19:17AM +0000, Philippe Elie wrote: > Also is this features added to binutils or need a separate Strongly agree, I have no idea why RH are making every tool duplicate this code instead of having libbfd find the debug images itself. Can you explain please Graydon ? regards john |
From: Philippe E. <ph...@wa...> - 2003-07-02 19:44:33
|
John Levon wrote: > On Wed, Jul 02, 2003 at 10:19:17AM +0000, Philippe Elie wrote: > > >>Also is this features added to binutils or need a separate > > > Strongly agree, I have no idea why RH are making every tool duplicate > this code instead of having libbfd find the debug images itself. Can you > explain please Graydon ? an attempt has been made http://sources.redhat.com/ml/binutils/2003-01/msg00541.html regards, Phil |
From: John L. <le...@mo...> - 2003-07-02 22:36:59
|
On Wed, Jul 02, 2003 at 09:56:07PM +0000, Philippe Elie wrote: > http://sources.redhat.com/ml/binutils/2003-01/msg00541.html But Daniel's question here : http://sources.redhat.com/ml/binutils/2003-01/msg00547.html was never answered. If merging can be avoided, then perhaps libbfd could still do it. GDB seems to have a tradition of duplicating stuff anyway. I'd rather rely on libbfd if we possibly can. regards, john |
From: graydon h. <gr...@re...> - 2003-07-02 22:47:49
|
Philippe Elie <ph...@wa...> writes: > John Levon wrote: > > On Wed, Jul 02, 2003 at 10:19:17AM +0000, Philippe Elie wrote: > > > >>Also is this features added to binutils or need a separate > > Strongly agree, I have no idea why RH are making every tool duplicate > > this code instead of having libbfd find the debug images itself. Can you > > explain please Graydon ? > > an attempt has been made > > http://sources.redhat.com/ml/binutils/2003-01/msg00541.html not only that, but they accepted and applied it upstream... http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/bfd/opncls.c?rev=1.18&content-type=text/x-cvsweb-markup&cvsroot=src three cheers for mandatory code review, I'd forgotten that. we were just cleaning up leftover oprofile patches which haven't made it upstream yet, and I ran across that one. I'm just now working out exactly how far the skew on our end is; it looks like some of our recent binutils packages are a bit out of date relative to the upstream. anyways, you definitely shouldn't use that patch, you're right. I'll post a better one shortly. -graydon |
From: graydon h. <gr...@re...> - 2003-07-02 23:39:08
|
oh, also, to answer your questions: Philippe Elie <ph...@wa...> writes: > is this already in production distro or only as alpha code ? it's been in our production distro for a couple releases now, I think. was introduced in either 8.0 or somewhere in the 8.0.1 / 8.1 beta releases before 9. I don't recall precisely. 9 has it for sure. > > the connection between the "main" binary and its debuginfo file is > > made via some well defined name mangling and a crc32 of the debuginfo > > file which is embedded in the main file. > > "well defined" ? reading the code I'm unusure of that. Elaborate please for ELF image 'foo', the algorithm looks for a section in foo called '.gnu_debuglink'. that section contains a filename and a crc32 checksum. the filename is typically 'foo.debug'. the algorithm then looks for the filename in the following locations, in order: - in the same directory as the open BFD for foo - a subdirectory called .debug - a global directory (/usr/lib/debug or such) followed by the dirname of the open BFD for foo. if it finds such a file, it checksums the file and compares against the embedded crc32 value. if the checksums match, the debuginfo file is loaded. > I'm also confused by the name debuginfo, is opannotate --source > working with this patch or is it working only for symbol name > information ? symbol names and line numbers, both. typically anything in a dwarf section. opannotate will use the info too, if it reads those. > calc_crc32 in a separate file but I don't like the fact we > need to crc32 the whole debug info file, these files can > be numberous and voluminous, if this feature is not already > in RH production distro can I ask the debuginfo file is > augmented with a section containing the check sum, then we > don't need to recalcultate the check sum but just compare > the checksum in .gnu_debuglink and the checksum in the debug > file. It'll speed up a lot operation like opreport -l. hm, but then the checksum may be incorrect. in fact it is kind of pointless to use a checksum at all if it is not checked; we might as well just use a large random binary string. > first_try look like the same as the original binary name, or perhaps > I'm mis-reading the code ? suppose we are looking at /usr/bin/foo. then first_try is /usr/bin/foo.debuginfo; it's the base name which is read out of the .gnu_debuglink section appended to the dirname of the initial BFD. > third_try seem evil except if .gnu_debuglink contain the full > pathname minus leading '/' third_try is the global path, followed by the dirname of the BFD, followed by the basename pulled from the .gnu_debuglink section. so in this case say /usr/lib/debug/usr/bin/foo.debug > Ok, repeating myself a bit: it seems evil than to support > *one* distro we need to handle three different name > encoding. the purpose of the multiple paths, as I understand it, has to do with supporting private builds of a tool side-by-side with a system-wide installed version. but I didn't come up with the name encoding and can't really defend it. it seems pretty arbitrary to me too. I think gdb people thought it up. anyways, this stuff is all distro-agnostic. it may be that we're the only distro doing it right now, but the capability was contributed to standard tools (eg. stock binutils and gdb) and I wouldn't be surprised if others adopt it. it works pretty well. > > +void op_bfd::add_to_section_map(bfd * abfd, asection * sect, PTR obj) > > (obj == this) remove this paramater please I'm afraid I cannot. this is a static function in the op_bfd namespace, not a method of an op_bfd object. the 'this' pointer doesn't exist. the function is a callback that libbfd is calling, as it iterates over the sections, and PTR is a 'user data' parameter that the BFD iteration function threads through for you. > > + assert(abfd); > > + assert(the_bfd); > > remove it er.. ok. those pointers are coming back in from libbfd though, so I don't particularly trust them. if you really don't like extra asserts I guess I'll remove them. -graydon |
From: Philippe E. <ph...@wa...> - 2003-07-03 06:25:05
|
cc'ing binutils list hi, we was discussing about the separate debug info files. I've some doubt than crc is the right way to check for debug file match. graydon hoare wrote: > oh, also, to answer your questions: > > Philippe Elie <ph...@wa...> writes: >>calc_crc32 in a separate file but I don't like the fact we >>need to crc32 the whole debug info file, these files can >>be numberous and voluminous, if this feature is not already >>in RH production distro can I ask the debuginfo file is >>augmented with a section containing the check sum, then we >>don't need to recalcultate the check sum but just compare >>the checksum in .gnu_debuglink and the checksum in the debug >>file. It'll speed up a lot operation like opreport -l. Changed my mind, now I think a timestamp is better. This sort of problem is generally solved by a timestamp (64 bits) not a crc, both file contain the same timestamp and the timestamp must be equal, imho crc is usefull to check file corruption not file matching > hm, but then the checksum may be incorrect. in fact it is kind of > pointless to use a checksum at all if it is not checked; we might as > well just use a large random binary string. crc *can*(1) be here if someone want to check if the debug file is corrupted but this must be a client decision to check the crc not a bfd once. With the current scheme I'm unusure than an operport -l will work enough well since we can get easily a few GB of debug info leading to an opreport slower by a few magnitude order. It can be usefull for gdb itself, what will occur if I try to get a bt for OpenOffice and it's 1.2 GB of debug info in separate debug file ? doing it in this step seems feasible: @item Run @code{objcopy --add-gnu-debuglink=foo.dbg foo} to add a link to the debugging info into the stripped executable. objcopy --add-gnu-link will add a timestamp section in both debug file and binary which can be used to check file match by bfd lib. BFD maintainers is this scheme sensible? (1) this sort of problem is generally solved by a timestamp (64 bits) not a crc, both file contain the same timestamp and the timestamp must be equal, imho crc is usefull to check file corruption not file matching regards, Phil |
From: Nick C. <ni...@re...> - 2003-07-04 10:05:01
|
Hi Philippe, > cc'ing binutils list [And adding gdb since this affects how debuginfo files are processed]. > hi, we was discussing about the separate debug info files. > I've some doubt than crc is the right way to check for debug > file match. > Changed my mind, now I think a timestamp is better. > This sort of problem is generally solved by a timestamp > (64 bits) not a crc, both file contain the same timestamp and > the timestamp must be equal, imho crc is usefull to check file > corruption not file matching Although by having a crc you can do both at the same time. > crc *can*(1) be here if someone want to check if the debug file > is corrupted but this must be a client decision to check the > crc not a bfd one. With the current scheme I'm unusure than an > operport -l will work enough well since we can get easily a few > GB of debug info leading to an opreport slower by a few magnitude > order. It can be usefull for gdb itself, what will occur if I try > to get a bt for OpenOffice and it's 1.2 GB of debug info in separate > debug file ? > > doing it in this way seems feasible: > > objcopy --add-gnu-link will add a timestamp section in both > debug file and binary which can be used to check file match > by bfd lib. > > BFD maintainers is this scheme sensible? The problem binutils has is that the debuginfo file and .gnu-debuginfo section in the stripped file are created by two different steps (1). Thus adding the same timestamp to both files is tricky and would require that the debuginfo file be first created with a blank timestamp section and then later, when the .gnu-debuginfo section is added, the timestamp is initialised. The other problem with timestamps is that it means that bootstrap sequences that involve the creation of debuginfo file will no longer work. ie the debuginfo files and stripped executables from two different bootstrap stages will no longer compare as identical because of the timestamps. This problem could be avoided by using a unique-binary-identifier that is specified on the command line, rather than a timestamp that is generated by the objcopy/strip program. ie objcopy could have a new switch such as: --set-debuglink-timestamp=<number> If this switch is not specified when --add-gnu-debuglink is used, then a timestamp is added, otherwise <number> is added. Generating a non-trivial value for <number> however might prove difficult... Still, I am not totally opposed to the idea, and if the gdb guys agree I would be happy to consider a patch adding these features. (I am a bit swamped at the moment, so I do not have time to create the patch myself). It should not be too hard to do. We just need to agree on the format and name of the timestamp section in the debuginfo file, and how to distinguish old .gnu-debuglink sections (only containing a crc) from new ones (containing a crc and a timestamp). Possibilities for the new section in the debuginfo file include: .gnu_debuglink (ie reuse the section name that is found in the stripped executable. The version in the debuginfo file could be exactly the same format, or it could be a shortened version with only the timestamp and not the filename or crc). .gnu_debuglink_timestamp (a bit wordy, but self documenting) .note.gnu.debuglink.timestamp (even longer, but who's counting ? :-) Cheers Nick (1) This was a deliberate choice on my part since the strip and objcopy tools historically only ever have one output file. I could have changed their internals to handle multiple outputs, but this was more work than I wanted to do, and I believe in keeping things simple unless forced to add complications. |
From: Philippe E. <ph...@wa...> - 2003-07-04 23:38:39
|
Nick Clifton wrote: > Hi Philippe, [trying to avoid crc'ing the separate debug file] I need to know how GDB guys want I deal with the gdb part, for now gdb.diff just remove (#if 0) all duplicated code from bfd and use bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it ok to remove this code or must I update the duplicated code according to the change in bfd ? > The problem binutils has is that the debuginfo file and .gnu-debuginfo > section in the stripped file are created by two different steps (1). > Thus adding the same timestamp to both files is tricky and would > require that the debuginfo file be first created with a blank > timestamp section and then later, when the .gnu-debuginfo section is > added, the timestamp is initialised. I avoided it by forcing user to specify the timestamp in both step > > The other problem with timestamps is that it means that bootstrap > sequences that involve the creation of debuginfo file will no longer > work. ie the debuginfo files and stripped executables from two > different bootstrap stages will no longer compare as identical because > of the timestamps. > > This problem could be avoided by using a unique-binary-identifier that > is specified on the command line, rather than a timestamp that is > generated by the objcopy/strip program. ie objcopy could have a new > switch such as: > > --set-debuglink-timestamp=<number> > > If this switch is not specified when --add-gnu-debuglink is used, then > a timestamp is added, otherwise <number> is added. Generating a > non-trivial value for <number> however might prove difficult... I like it, such number is packager responsiblility, for gnu based system `date +%s` should be sufficient. I changed slightly it, number is not optionnal and must be provided by caller, is it a real problem ? > We just need to agree on the format and name of the timestamp section > in the debuginfo file, and how to distinguish old .gnu-debuglink > sections (only containing a crc) from new ones (containing a crc and a > timestamp). I disambiguite the two case by looking the section size. See bfd.diff, it's a bit ugly but I don't see how I can deal cleanly with this problem. > > Possibilities for the new section in the debuginfo file include: > > .gnu_debuglink (ie reuse the section name that is found in the > stripped executable. The version in the debuginfo > file could be exactly the same format, or it could > be a shortened version with only the timestamp and > not the filename or crc). > > .gnu_debuglink_timestamp (a bit wordy, but self documenting) I tried to re-use exactly the same format but putting a valid filename in the .gnu_debuglink in debug info file conduct gdb into an infinite loop so on I preferred to play safe and use a separate section. The patch is a RFC rather than a submission, it needs some cleanup, updating bfd documentation etc. btw bfd documentation gives two alternative to use this feature but the first doesn't works with or w/o the attached patch: @item Run @code{objcopy --strip-debug foo} to create a stripped executable. @item Run @code{objcopy --add-gnu-debuglink=foo.dbg foo} to add a link to the debugging info into the stripped executable. gdb doesn't like file produced in this way and gdb document only the second way which works @item Copy @code{foo} to @code{foo.full} @item Run @code{objcopy --strip-debug foo} @item Run @code{objcopy --add-gnu-debuglink=foo.full foo} The patch is tested with gdb code with and w/o gdeb.diff applied, it works transparentely for gdb except obviously the old code always does the crc. patch can be tested through this two ways: gcc -g -O2 test.cpp cp a.out a.out.dbg strip --strip-unneeded a.out objcopy --set-debuglink-timestamp=12 a.out.dbg objcopy --set-debuglink-timestamp=12 --add-gnu-debuglink=a.out.dbg a.out gcc -g -O2 test.cpp cp a.out a.out.dbg strip --strip-unneeded a.out objcopy --add-gnu-debuglink=a.out.dbg a.out the first is now the preferred way. I don't like at all than gdb need the full file it makes separate debug info less usefull, can a GDB wizard gives some clues if it's feasible to use the file stripped from it's code/data section and if it'll be hard to implement ? regards, Philippe Elie |
From: Andrew C. <ac1...@re...> - 2003-07-18 13:42:38
|
Philippe, > > [trying to avoid crc'ing the separate debug file] > > I need to know how GDB guys want I deal with the gdb part, for now > gdb.diff just remove (#if 0) all duplicated code from bfd and use > bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it > ok to remove this code or must I update the duplicated code according > to the change in bfd ? I just wonder if it should eventually be made more transparent? bfd_openr (file, FOLLOW_DEBUG_LINK). Doing things like: objdump --follow-debug-link would then become possible. Regardless, it makes sense to put the algorighm in BFD. Nick wrote: > Overall though I like the patch and the solution. If we can get the > GDB maintainers to agree (or at least not object to) adding the extra > field at the end of the .gnu-debuglink section then I would be happy > to review a final version of the patch. (Note - you will need a FSF > copyright assignment as well...) Well, the so called GNU debuglink mechanism was never actually discussed on a GNU list, re-visiting it now sounds like a good idea. Looks to me like you've come up with something actually useful. Perhaps someone should post a revised description and have it added to the BFD doco. Here's the current description from GDB: http://sources.redhat.com/gdb/current/onlinedocs/gdb_16.html#SEC134 Andrew |
From: John L. <le...@mo...> - 2003-07-03 10:54:22
|
On Wed, Jul 02, 2003 at 07:38:48PM -0400, graydon hoare wrote: > > > +void op_bfd::add_to_section_map(bfd * abfd, asection * sect, PTR obj) > > > > (obj == this) remove this paramater please > > I'm afraid I cannot. this is a static function in the op_bfd > namespace, not a method of an op_bfd object. the 'this' pointer > doesn't exist. the function is a callback that libbfd is calling, as My preferred idiom for this is a file-scope C callback that does the pointer manipulation : extern "C" { static void add_to_map(bfd * abfd, asection * sect, void * obj) { op_bfd * opbfd = static_cast<op_bfd *>(obj); assert(obbfd); opbfd->add_to_section_map(abfd, sect); } }; I vaguely remember discussing whether static C++ methods can be safely called from C code or not ... I forget the conclusion. But I prefer the above anyway. regards john |
From: Philippe E. <ph...@wa...> - 2003-07-04 23:41:01
|
Philippe Elie wrote: > Nick Clifton wrote: > >> Hi Philippe, > sorry, the missing patch ... regards, Phil |
From: Nick C. <ni...@re...> - 2003-07-11 16:01:51
|
Hi Philippe, Sorry for the long delay in replying - I have been very busy over the last few weeks. > I need to know how GDB guys want I deal with the gdb part, for now > gdb.diff just remove (#if 0) all duplicated code from bfd and use > bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it > ok to remove this code or must I update the duplicated code according > to the change in bfd ? Well this is up to the gdb maintainers to decide, but it certainly seems like a good idea to avoid the code duplication. > > We just need to agree on the format and name of the timestamp > > section in the debuginfo file, and how to distinguish old > > .gnu-debuglink sections (only containing a crc) from new ones > > (containing a crc and a timestamp). > > I disambiguate the two case by looking the section size. See > bfd.diff, it's a bit ugly but I don't see how I can deal cleanly > with this problem. Actually I rather like this solution. Adding the timestamp as an extra field at the end of the .gnu-debuglink section seems rather elegant and it eliminates the need for a new section. Some comments on the patch: > + #define GNU_DEBUGLINK_TIMESTAMP ".gnu_debuglink_timestamp" This is not needed, since the timestamp is going to be held in the .gnu-debuglink section. > INTERNAL_FUNCTION > + bfd_get_debug_timestamp_info > + > + SYNOPSIS > + bfd_boolean bfd_get_debug_timestamp_info > + (bfd *abfd, unsigned long *timestamp); > + > + DESCRIPTION > + fetch the timestamp for any separate debuginfo associated with > + @var{abfd}. Similarly this function should extract the timestamp from the .gnu-debuglink section. > + printf("try %s %ld\n", name, timestamp); I assume that this is left over debugging ? > ! debuglink_size += 8; We ought to add a comment explaining why the value 8 is used here. Overall though I like the patch and the solution. If we can get the GDB maintainers to agree (or at least not object to) adding the extra field at the end of the .gnu-debuglink section then I would be happy to review a final version of the patch. (Note - you will need a FSF copyright assignment as well...) Cheers Nick |
From: Jim B. <ji...@re...> - 2003-07-18 06:42:24
|
Nick Clifton <ni...@re...> writes: > Hi Philippe, > > Sorry for the long delay in replying - I have been very busy over > the last few weeks. > > > > I need to know how GDB guys want I deal with the gdb part, for now > > gdb.diff just remove (#if 0) all duplicated code from bfd and use > > bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it > > ok to remove this code or must I update the duplicated code according > > to the change in bfd ? > > Well this is up to the gdb maintainers to decide, but it certainly > seems like a good idea to avoid the code duplication. Sure, the plan has long been for GDB to just use the function in BFD. The code was added to GDB before BFD; that's the only reason it's there at all. Just to be sure --- under this arrangement, the old-style debug links will continue to work, right? One could use something like '(date; ps auxww; vmstat) | md5sum | cut -b 1-33' to generate nice unique ID strings. |
From: Nick C. <ni...@re...> - 2003-07-18 08:06:39
|
Hi Jim, >> > I need to know how GDB guys want I deal with the gdb part, for now >> > gdb.diff just remove (#if 0) all duplicated code from bfd and use >> > bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it >> > ok to remove this code or must I update the duplicated code according >> > to the change in bfd ? >> >> Well this is up to the gdb maintainers to decide, but it certainly >> seems like a good idea to avoid the code duplication. > > Sure, the plan has long been for GDB to just use the function in BFD. > The code was added to GDB before BFD; that's the only reason it's > there at all. > > Just to be sure --- under this arrangement, the old-style debug links > will continue to work, right? Yes. > One could use something like '(date; ps auxww; vmstat) | md5sum | cut > -b 1-33' to generate nice unique ID strings. yikes! Well that ought to be a reasonably randon number. The only problem would be making sure that the same number was added to both the debuginfo file and the stripped binary. I guess you would need to make sure your makefile only computed the value once, before it creates either file. Cheers Nick |
From: Daniel J. <dr...@mv...> - 2003-07-18 14:03:43
|
On Fri, Jul 18, 2003 at 09:42:18AM -0400, Andrew Cagney wrote: > Philippe, > > > >[trying to avoid crc'ing the separate debug file] > > > >I need to know how GDB guys want I deal with the gdb part, for now > >gdb.diff just remove (#if 0) all duplicated code from bfd and use > >bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it > >ok to remove this code or must I update the duplicated code according > >to the change in bfd ? > > I just wonder if it should eventually be made more transparent? > bfd_openr (file, FOLLOW_DEBUG_LINK). > Doing things like: > objdump --follow-debug-link > would then become possible. Regardless, it makes sense to put the > algorighm in BFD. It should. I talked Graydon into trying this at the time he submitted the BFD part - it turned out to be a world of trouble given BFDs current data structure, and we bailed out. -- Daniel Jacobowitz MontaVista Software Debian GNU/Linux Developer |
From: graydon h. <gr...@re...> - 2003-07-18 14:41:20
|
Daniel Jacobowitz <dr...@mv...> writes: > On Fri, Jul 18, 2003 at 09:42:18AM -0400, Andrew Cagney wrote: > > Philippe, > > > > > >[trying to avoid crc'ing the separate debug file] > > > > > >I need to know how GDB guys want I deal with the gdb part, for now > > >gdb.diff just remove (#if 0) all duplicated code from bfd and use > > >bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it > > >ok to remove this code or must I update the duplicated code according > > >to the change in bfd ? > > > > I just wonder if it should eventually be made more transparent? > > bfd_openr (file, FOLLOW_DEBUG_LINK). > > Doing things like: > > objdump --follow-debug-link > > would then become possible. Regardless, it makes sense to put the > > algorighm in BFD. > > It should. I talked Graydon into trying this at the time he submitted > the BFD part - it turned out to be a world of trouble given BFDs > current data structure, and we bailed out. yeah, we gave up on "transparent" access in bfd_openr due to the ugliness of merging symbols from separate bfds; nonetheless the requisite (distinct) debuglink-following function was added upstream. http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/bfd/opncls.c.diff?r1=1.13&r2=1.14&cvsroot=src&f=h -graydon |