From: Maynard J. <may...@us...> - 2012-11-29 21:50:48
|
Add support for using build-id to locate debuginfo files This patch addresses the issue raised in oprofile bug #3591165. In brief, a new MiniDebuginfo feature was introduced in Fedora18 that changes how runtime RPMs and their corresponding debuginfo RPMs are built such that the CRC stored in the runtime binary no longer matches the CRC we calculate from the contents of the debuginfo file. Other tools that use debuginfo (like gdb) use the the build-id method (i.e., build-id is stored in runtime which can be used to locate the matching debuginfo file), and only use the CRC method as a fallback mechanism. This patch adds the build-id method, making it the primary mechanism for locating debuginfo files. Signed-off-by: Maynard Johnson <may...@us...> --- libutil++/bfd_support.cpp | 117 +++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 114 insertions(+), 3 deletions(-) diff --git a/libutil++/bfd_support.cpp b/libutil++/bfd_support.cpp index d1383f8..6f4e8e1 100644 --- a/libutil++/bfd_support.cpp +++ b/libutil++/bfd_support.cpp @@ -17,7 +17,10 @@ #include "file_manip.h" #include "cverb.h" #include "locate_images.h" +#include "op_libiberty.h" +#include <unistd.h> +#include <limits.h> #include <cstdlib> #include <cstring> #include <cassert> @@ -34,6 +37,11 @@ extern verbose vbfd; namespace { +#define BUILD_ID_SIZE 20 +#ifndef NT_GNU_BUILD_ID +#define NT_GNU_BUILD_ID 3 +#endif + void check_format(string const & file, bfd ** ibfd) { @@ -75,6 +83,95 @@ bool separate_debug_file_exists(string & name, unsigned long const crc, return crc == file_crc; } +static bool find_debuginfo_file_by_buildid(unsigned char * buildid, string & debug_filename) +{ + size_t build_id_fname_size = strlen (DEBUGDIR) + (sizeof "/.build-id/" - 1) + 1 + + (2 * BUILD_ID_SIZE) + (sizeof ".debug" - 1) + 1; + char * buildid_symlink = (char *) xmalloc(build_id_fname_size); + char * sptr = buildid_symlink; + unsigned char * bptr = buildid; + bool retval = false; + size_t build_id_segment_len = strlen("/.build-id/"); + + + memcpy(sptr, DEBUGDIR, strlen(DEBUGDIR)); + sptr += strlen(DEBUGDIR); + memcpy(sptr, "/.build-id/", build_id_segment_len); + sptr += build_id_segment_len; + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); + *sptr++ = '/'; + for (int i = BUILD_ID_SIZE - 1; i > 0; i--) + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); + + strcpy(sptr, ".debug"); + + if (access (buildid_symlink, F_OK) == 0) { + debug_filename = op_realpath (buildid_symlink); + if (debug_filename.compare(buildid_symlink)) { + retval = true; + cverb << vbfd << "Using build-id symlink" << endl; + } + } + free(buildid_symlink); + if (!retval) + cverb << vbfd << "build-id file not found; falling back to CRC method." << endl; + + return retval; +} + +static bool get_build_id(bfd * ibfd, unsigned char * build_id) +{ + struct op_elf_Note_hdr { + unsigned int op_note_namesz; + unsigned int op_note_descsz; + unsigned int op_note_type; + } op_note_hdr; + asection * sect; + char * ptr; + bool retval = false; + + cverb << vbfd << "fetching build-id from runtime binary ..."; + if (!(sect = bfd_get_section_by_name(ibfd, ".note.gnu.build-id"))) { + if (!(sect = bfd_get_section_by_name(ibfd, ".notes"))) { + cverb << vbfd << " No build-id section found" << endl; + return false; + } + } + + bfd_size_type buildid_sect_size = bfd_section_size(ibfd, sect); + char contents[buildid_sect_size]; + + if (!bfd_get_section_contents(ibfd, sect, + reinterpret_cast<unsigned char *>(contents), + static_cast<file_ptr>(0), buildid_sect_size)) { + bfd_perror("bfd_get_section_contents:get_debug:"); + exit(2); + } + + ptr = contents; + while (ptr < (contents + buildid_sect_size)) { + op_note_hdr.op_note_namesz = bfd_get_32(ibfd, + reinterpret_cast<bfd_byte *>(contents)); + op_note_hdr.op_note_descsz = bfd_get_32(ibfd, + reinterpret_cast<bfd_byte *>(contents + 4)); + op_note_hdr.op_note_type = bfd_get_32(ibfd, + reinterpret_cast<bfd_byte *>(contents + 8)); + ptr += sizeof(op_note_hdr); + if ((op_note_hdr.op_note_type == NT_GNU_BUILD_ID) && + (op_note_hdr.op_note_namesz == sizeof("GNU")) && + (strcmp("GNU", ptr ) == 0)) { + memcpy(build_id, ptr + op_note_hdr.op_note_namesz, BUILD_ID_SIZE); + retval = true; + cverb << vbfd << "Found build-id" << endl; + break; + } + ptr += op_note_hdr.op_note_namesz + op_note_hdr.op_note_descsz; + } + if (!retval) + cverb << vbfd << " No build-id found" << endl; + + return retval; +} bool get_debug_link_info(bfd * ibfd, string & filename, unsigned long & crc32) { @@ -298,10 +395,24 @@ bool find_separate_debug_file(bfd * ibfd, string const & filepath_in, { string filepath(filepath_in); string basename; - unsigned long crc32; + bool use_build_id = true; + unsigned long crc32 = 0; + unsigned char buildid[BUILD_ID_SIZE]; - if (!get_debug_link_info(ibfd, basename, crc32)) - return false; + if (!(use_build_id = get_build_id(ibfd, buildid))) + if (!get_debug_link_info(ibfd, basename, crc32)) + return false; + + if (use_build_id && find_debuginfo_file_by_buildid(buildid, debug_filename)) + return true; + + /* Use old method of finding debuginfo file by comparing runtime binary's + * CRC with the CRC we calculate from the debuginfo file's contents. + * NOTE: This method breaks on systems where "MiniDebugInfo" is used + * since the CRC stored in the runtime binary won't match the compressed + * debuginfo file's CRC. But in practice, we shouldn't ever run into such + * a scenario since the build-id should always be available. + */ // Work out the image file's directory prefix string filedir = op_dirname(filepath); -- 1.7.1 |
From: Maynard J. <may...@us...> - 2012-12-11 17:30:41
|
On 11/29/2012 03:50 PM, Maynard Johnson wrote: > Add support for using build-id to locate debuginfo files Can someone please review this patch? Thanks. -Maynard > > This patch addresses the issue raised in oprofile bug #3591165. > In brief, a new MiniDebuginfo feature was introduced in Fedora18 that > changes how runtime RPMs and their corresponding debuginfo RPMs are built > such that the CRC stored in the runtime binary no longer matches the CRC > we calculate from the contents of the debuginfo file. Other tools that use > debuginfo (like gdb) use the the build-id method (i.e., build-id is stored > in runtime which can be used to locate the matching debuginfo file), and > only use the CRC method as a fallback mechanism. This patch adds the > build-id method, making it the primary mechanism for locating debuginfo > files. > > Signed-off-by: Maynard Johnson <may...@us...> > --- > libutil++/bfd_support.cpp | 117 +++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 114 insertions(+), 3 deletions(-) > > diff --git a/libutil++/bfd_support.cpp b/libutil++/bfd_support.cpp > index d1383f8..6f4e8e1 100644 > --- a/libutil++/bfd_support.cpp > +++ b/libutil++/bfd_support.cpp > @@ -17,7 +17,10 @@ > #include "file_manip.h" > #include "cverb.h" > #include "locate_images.h" > +#include "op_libiberty.h" > > +#include <unistd.h> > +#include <limits.h> > #include <cstdlib> > #include <cstring> > #include <cassert> > @@ -34,6 +37,11 @@ extern verbose vbfd; > > namespace { > > +#define BUILD_ID_SIZE 20 > +#ifndef NT_GNU_BUILD_ID > +#define NT_GNU_BUILD_ID 3 > +#endif > + > > void check_format(string const & file, bfd ** ibfd) > { > @@ -75,6 +83,95 @@ bool separate_debug_file_exists(string & name, unsigned long const crc, > return crc == file_crc; > } > > +static bool find_debuginfo_file_by_buildid(unsigned char * buildid, string & debug_filename) > +{ > + size_t build_id_fname_size = strlen (DEBUGDIR) + (sizeof "/.build-id/" - 1) + 1 > + + (2 * BUILD_ID_SIZE) + (sizeof ".debug" - 1) + 1; > + char * buildid_symlink = (char *) xmalloc(build_id_fname_size); > + char * sptr = buildid_symlink; > + unsigned char * bptr = buildid; > + bool retval = false; > + size_t build_id_segment_len = strlen("/.build-id/"); > + > + > + memcpy(sptr, DEBUGDIR, strlen(DEBUGDIR)); > + sptr += strlen(DEBUGDIR); > + memcpy(sptr, "/.build-id/", build_id_segment_len); > + sptr += build_id_segment_len; > + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); > + *sptr++ = '/'; > + for (int i = BUILD_ID_SIZE - 1; i > 0; i--) > + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); > + > + strcpy(sptr, ".debug"); > + > + if (access (buildid_symlink, F_OK) == 0) { > + debug_filename = op_realpath (buildid_symlink); > + if (debug_filename.compare(buildid_symlink)) { > + retval = true; > + cverb << vbfd << "Using build-id symlink" << endl; > + } > + } > + free(buildid_symlink); > + if (!retval) > + cverb << vbfd << "build-id file not found; falling back to CRC method." << endl; > + > + return retval; > +} > + > +static bool get_build_id(bfd * ibfd, unsigned char * build_id) > +{ > + struct op_elf_Note_hdr { > + unsigned int op_note_namesz; > + unsigned int op_note_descsz; > + unsigned int op_note_type; > + } op_note_hdr; > + asection * sect; > + char * ptr; > + bool retval = false; > + > + cverb << vbfd << "fetching build-id from runtime binary ..."; > + if (!(sect = bfd_get_section_by_name(ibfd, ".note.gnu.build-id"))) { > + if (!(sect = bfd_get_section_by_name(ibfd, ".notes"))) { > + cverb << vbfd << " No build-id section found" << endl; > + return false; > + } > + } > + > + bfd_size_type buildid_sect_size = bfd_section_size(ibfd, sect); > + char contents[buildid_sect_size]; > + > + if (!bfd_get_section_contents(ibfd, sect, > + reinterpret_cast<unsigned char *>(contents), > + static_cast<file_ptr>(0), buildid_sect_size)) { > + bfd_perror("bfd_get_section_contents:get_debug:"); > + exit(2); > + } > + > + ptr = contents; > + while (ptr < (contents + buildid_sect_size)) { > + op_note_hdr.op_note_namesz = bfd_get_32(ibfd, > + reinterpret_cast<bfd_byte *>(contents)); > + op_note_hdr.op_note_descsz = bfd_get_32(ibfd, > + reinterpret_cast<bfd_byte *>(contents + 4)); > + op_note_hdr.op_note_type = bfd_get_32(ibfd, > + reinterpret_cast<bfd_byte *>(contents + 8)); > + ptr += sizeof(op_note_hdr); > + if ((op_note_hdr.op_note_type == NT_GNU_BUILD_ID) && > + (op_note_hdr.op_note_namesz == sizeof("GNU")) && > + (strcmp("GNU", ptr ) == 0)) { > + memcpy(build_id, ptr + op_note_hdr.op_note_namesz, BUILD_ID_SIZE); > + retval = true; > + cverb << vbfd << "Found build-id" << endl; > + break; > + } > + ptr += op_note_hdr.op_note_namesz + op_note_hdr.op_note_descsz; > + } > + if (!retval) > + cverb << vbfd << " No build-id found" << endl; > + > + return retval; > +} > > bool get_debug_link_info(bfd * ibfd, string & filename, unsigned long & crc32) > { > @@ -298,10 +395,24 @@ bool find_separate_debug_file(bfd * ibfd, string const & filepath_in, > { > string filepath(filepath_in); > string basename; > - unsigned long crc32; > + bool use_build_id = true; > + unsigned long crc32 = 0; > + unsigned char buildid[BUILD_ID_SIZE]; > > - if (!get_debug_link_info(ibfd, basename, crc32)) > - return false; > + if (!(use_build_id = get_build_id(ibfd, buildid))) > + if (!get_debug_link_info(ibfd, basename, crc32)) > + return false; > + > + if (use_build_id && find_debuginfo_file_by_buildid(buildid, debug_filename)) > + return true; > + > + /* Use old method of finding debuginfo file by comparing runtime binary's > + * CRC with the CRC we calculate from the debuginfo file's contents. > + * NOTE: This method breaks on systems where "MiniDebugInfo" is used > + * since the CRC stored in the runtime binary won't match the compressed > + * debuginfo file's CRC. But in practice, we shouldn't ever run into such > + * a scenario since the build-id should always be available. > + */ > > // Work out the image file's directory prefix > string filedir = op_dirname(filepath); |
From: Maynard J. <may...@us...> - 2012-12-12 17:20:01
|
On 12/11/2012 11:30 AM, Maynard Johnson wrote: > On 11/29/2012 03:50 PM, Maynard Johnson wrote: >> Add support for using build-id to locate debuginfo files > > Can someone please review this patch? Thanks. Suravee, Will . . . Could one of you review this patch? Thanks. -Maynard > > -Maynard >> >> This patch addresses the issue raised in oprofile bug #3591165. >> In brief, a new MiniDebuginfo feature was introduced in Fedora18 that >> changes how runtime RPMs and their corresponding debuginfo RPMs are built >> such that the CRC stored in the runtime binary no longer matches the CRC >> we calculate from the contents of the debuginfo file. Other tools that use >> debuginfo (like gdb) use the the build-id method (i.e., build-id is stored >> in runtime which can be used to locate the matching debuginfo file), and >> only use the CRC method as a fallback mechanism. This patch adds the >> build-id method, making it the primary mechanism for locating debuginfo >> files. >> >> Signed-off-by: Maynard Johnson <may...@us...> >> --- >> libutil++/bfd_support.cpp | 117 +++++++++++++++++++++++++++++++++++++++++++- >> 1 files changed, 114 insertions(+), 3 deletions(-) >> >> diff --git a/libutil++/bfd_support.cpp b/libutil++/bfd_support.cpp >> index d1383f8..6f4e8e1 100644 >> --- a/libutil++/bfd_support.cpp >> +++ b/libutil++/bfd_support.cpp >> @@ -17,7 +17,10 @@ >> #include "file_manip.h" >> #include "cverb.h" >> #include "locate_images.h" >> +#include "op_libiberty.h" >> >> +#include <unistd.h> >> +#include <limits.h> >> #include <cstdlib> >> #include <cstring> >> #include <cassert> >> @@ -34,6 +37,11 @@ extern verbose vbfd; >> >> namespace { >> >> +#define BUILD_ID_SIZE 20 >> +#ifndef NT_GNU_BUILD_ID >> +#define NT_GNU_BUILD_ID 3 >> +#endif >> + >> >> void check_format(string const & file, bfd ** ibfd) >> { >> @@ -75,6 +83,95 @@ bool separate_debug_file_exists(string & name, unsigned long const crc, >> return crc == file_crc; >> } >> >> +static bool find_debuginfo_file_by_buildid(unsigned char * buildid, string & debug_filename) >> +{ >> + size_t build_id_fname_size = strlen (DEBUGDIR) + (sizeof "/.build-id/" - 1) + 1 >> + + (2 * BUILD_ID_SIZE) + (sizeof ".debug" - 1) + 1; >> + char * buildid_symlink = (char *) xmalloc(build_id_fname_size); >> + char * sptr = buildid_symlink; >> + unsigned char * bptr = buildid; >> + bool retval = false; >> + size_t build_id_segment_len = strlen("/.build-id/"); >> + >> + >> + memcpy(sptr, DEBUGDIR, strlen(DEBUGDIR)); >> + sptr += strlen(DEBUGDIR); >> + memcpy(sptr, "/.build-id/", build_id_segment_len); >> + sptr += build_id_segment_len; >> + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); >> + *sptr++ = '/'; >> + for (int i = BUILD_ID_SIZE - 1; i > 0; i--) >> + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); >> + >> + strcpy(sptr, ".debug"); >> + >> + if (access (buildid_symlink, F_OK) == 0) { >> + debug_filename = op_realpath (buildid_symlink); >> + if (debug_filename.compare(buildid_symlink)) { >> + retval = true; >> + cverb << vbfd << "Using build-id symlink" << endl; >> + } >> + } >> + free(buildid_symlink); >> + if (!retval) >> + cverb << vbfd << "build-id file not found; falling back to CRC method." << endl; >> + >> + return retval; >> +} >> + >> +static bool get_build_id(bfd * ibfd, unsigned char * build_id) >> +{ >> + struct op_elf_Note_hdr { >> + unsigned int op_note_namesz; >> + unsigned int op_note_descsz; >> + unsigned int op_note_type; >> + } op_note_hdr; >> + asection * sect; >> + char * ptr; >> + bool retval = false; >> + >> + cverb << vbfd << "fetching build-id from runtime binary ..."; >> + if (!(sect = bfd_get_section_by_name(ibfd, ".note.gnu.build-id"))) { >> + if (!(sect = bfd_get_section_by_name(ibfd, ".notes"))) { >> + cverb << vbfd << " No build-id section found" << endl; >> + return false; >> + } >> + } >> + >> + bfd_size_type buildid_sect_size = bfd_section_size(ibfd, sect); >> + char contents[buildid_sect_size]; >> + >> + if (!bfd_get_section_contents(ibfd, sect, >> + reinterpret_cast<unsigned char *>(contents), >> + static_cast<file_ptr>(0), buildid_sect_size)) { >> + bfd_perror("bfd_get_section_contents:get_debug:"); >> + exit(2); >> + } >> + >> + ptr = contents; >> + while (ptr < (contents + buildid_sect_size)) { >> + op_note_hdr.op_note_namesz = bfd_get_32(ibfd, >> + reinterpret_cast<bfd_byte *>(contents)); >> + op_note_hdr.op_note_descsz = bfd_get_32(ibfd, >> + reinterpret_cast<bfd_byte *>(contents + 4)); >> + op_note_hdr.op_note_type = bfd_get_32(ibfd, >> + reinterpret_cast<bfd_byte *>(contents + 8)); >> + ptr += sizeof(op_note_hdr); >> + if ((op_note_hdr.op_note_type == NT_GNU_BUILD_ID) && >> + (op_note_hdr.op_note_namesz == sizeof("GNU")) && >> + (strcmp("GNU", ptr ) == 0)) { >> + memcpy(build_id, ptr + op_note_hdr.op_note_namesz, BUILD_ID_SIZE); >> + retval = true; >> + cverb << vbfd << "Found build-id" << endl; >> + break; >> + } >> + ptr += op_note_hdr.op_note_namesz + op_note_hdr.op_note_descsz; >> + } >> + if (!retval) >> + cverb << vbfd << " No build-id found" << endl; >> + >> + return retval; >> +} >> >> bool get_debug_link_info(bfd * ibfd, string & filename, unsigned long & crc32) >> { >> @@ -298,10 +395,24 @@ bool find_separate_debug_file(bfd * ibfd, string const & filepath_in, >> { >> string filepath(filepath_in); >> string basename; >> - unsigned long crc32; >> + bool use_build_id = true; >> + unsigned long crc32 = 0; >> + unsigned char buildid[BUILD_ID_SIZE]; >> >> - if (!get_debug_link_info(ibfd, basename, crc32)) >> - return false; >> + if (!(use_build_id = get_build_id(ibfd, buildid))) >> + if (!get_debug_link_info(ibfd, basename, crc32)) >> + return false; >> + >> + if (use_build_id && find_debuginfo_file_by_buildid(buildid, debug_filename)) >> + return true; >> + >> + /* Use old method of finding debuginfo file by comparing runtime binary's >> + * CRC with the CRC we calculate from the debuginfo file's contents. >> + * NOTE: This method breaks on systems where "MiniDebugInfo" is used >> + * since the CRC stored in the runtime binary won't match the compressed >> + * debuginfo file's CRC. But in practice, we shouldn't ever run into such >> + * a scenario since the build-id should always be available. >> + */ >> >> // Work out the image file's directory prefix >> string filedir = op_dirname(filepath); > > > ------------------------------------------------------------------------------ > LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial > Remotely access PCs and mobile devices and provide instant support > Improve your efficiency, and focus on delivering more value-add services > Discover what IT Professionals Know. Rescue delivers > http://p.sf.net/sfu/logmein_12329d2d > _______________________________________________ > oprofile-list mailing list > opr...@li... > https://lists.sourceforge.net/lists/listinfo/oprofile-list > |
From: Maynard J. <may...@us...> - 2012-12-14 19:19:15
|
On 12/12/2012 11:18 AM, Maynard Johnson wrote: > On 12/11/2012 11:30 AM, Maynard Johnson wrote: >> On 11/29/2012 03:50 PM, Maynard Johnson wrote: >>> Add support for using build-id to locate debuginfo files >> >> Can someone please review this patch? Thanks. > Suravee, Will . . . Could one of you review this patch? Thanks. Having received no comments since this patch was posted two weeks ago, I committed it today. Review comments are still welcome. -Maynard > > -Maynard > >> >> -Maynard >>> >>> This patch addresses the issue raised in oprofile bug #3591165. >>> In brief, a new MiniDebuginfo feature was introduced in Fedora18 that >>> changes how runtime RPMs and their corresponding debuginfo RPMs are built >>> such that the CRC stored in the runtime binary no longer matches the CRC >>> we calculate from the contents of the debuginfo file. Other tools that use >>> debuginfo (like gdb) use the the build-id method (i.e., build-id is stored >>> in runtime which can be used to locate the matching debuginfo file), and >>> only use the CRC method as a fallback mechanism. This patch adds the >>> build-id method, making it the primary mechanism for locating debuginfo >>> files. >>> >>> Signed-off-by: Maynard Johnson <may...@us...> >>> --- >>> libutil++/bfd_support.cpp | 117 +++++++++++++++++++++++++++++++++++++++++++- >>> 1 files changed, 114 insertions(+), 3 deletions(-) >>> >>> diff --git a/libutil++/bfd_support.cpp b/libutil++/bfd_support.cpp >>> index d1383f8..6f4e8e1 100644 >>> --- a/libutil++/bfd_support.cpp >>> +++ b/libutil++/bfd_support.cpp >>> @@ -17,7 +17,10 @@ >>> #include "file_manip.h" >>> #include "cverb.h" >>> #include "locate_images.h" >>> +#include "op_libiberty.h" >>> >>> +#include <unistd.h> >>> +#include <limits.h> >>> #include <cstdlib> >>> #include <cstring> >>> #include <cassert> >>> @@ -34,6 +37,11 @@ extern verbose vbfd; >>> >>> namespace { >>> >>> +#define BUILD_ID_SIZE 20 >>> +#ifndef NT_GNU_BUILD_ID >>> +#define NT_GNU_BUILD_ID 3 >>> +#endif >>> + >>> >>> void check_format(string const & file, bfd ** ibfd) >>> { >>> @@ -75,6 +83,95 @@ bool separate_debug_file_exists(string & name, unsigned long const crc, >>> return crc == file_crc; >>> } >>> >>> +static bool find_debuginfo_file_by_buildid(unsigned char * buildid, string & debug_filename) >>> +{ >>> + size_t build_id_fname_size = strlen (DEBUGDIR) + (sizeof "/.build-id/" - 1) + 1 >>> + + (2 * BUILD_ID_SIZE) + (sizeof ".debug" - 1) + 1; >>> + char * buildid_symlink = (char *) xmalloc(build_id_fname_size); >>> + char * sptr = buildid_symlink; >>> + unsigned char * bptr = buildid; >>> + bool retval = false; >>> + size_t build_id_segment_len = strlen("/.build-id/"); >>> + >>> + >>> + memcpy(sptr, DEBUGDIR, strlen(DEBUGDIR)); >>> + sptr += strlen(DEBUGDIR); >>> + memcpy(sptr, "/.build-id/", build_id_segment_len); >>> + sptr += build_id_segment_len; >>> + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); >>> + *sptr++ = '/'; >>> + for (int i = BUILD_ID_SIZE - 1; i > 0; i--) >>> + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); >>> + >>> + strcpy(sptr, ".debug"); >>> + >>> + if (access (buildid_symlink, F_OK) == 0) { >>> + debug_filename = op_realpath (buildid_symlink); >>> + if (debug_filename.compare(buildid_symlink)) { >>> + retval = true; >>> + cverb << vbfd << "Using build-id symlink" << endl; >>> + } >>> + } >>> + free(buildid_symlink); >>> + if (!retval) >>> + cverb << vbfd << "build-id file not found; falling back to CRC method." << endl; >>> + >>> + return retval; >>> +} >>> + >>> +static bool get_build_id(bfd * ibfd, unsigned char * build_id) >>> +{ >>> + struct op_elf_Note_hdr { >>> + unsigned int op_note_namesz; >>> + unsigned int op_note_descsz; >>> + unsigned int op_note_type; >>> + } op_note_hdr; >>> + asection * sect; >>> + char * ptr; >>> + bool retval = false; >>> + >>> + cverb << vbfd << "fetching build-id from runtime binary ..."; >>> + if (!(sect = bfd_get_section_by_name(ibfd, ".note.gnu.build-id"))) { >>> + if (!(sect = bfd_get_section_by_name(ibfd, ".notes"))) { >>> + cverb << vbfd << " No build-id section found" << endl; >>> + return false; >>> + } >>> + } >>> + >>> + bfd_size_type buildid_sect_size = bfd_section_size(ibfd, sect); >>> + char contents[buildid_sect_size]; >>> + >>> + if (!bfd_get_section_contents(ibfd, sect, >>> + reinterpret_cast<unsigned char *>(contents), >>> + static_cast<file_ptr>(0), buildid_sect_size)) { >>> + bfd_perror("bfd_get_section_contents:get_debug:"); >>> + exit(2); >>> + } >>> + >>> + ptr = contents; >>> + while (ptr < (contents + buildid_sect_size)) { >>> + op_note_hdr.op_note_namesz = bfd_get_32(ibfd, >>> + reinterpret_cast<bfd_byte *>(contents)); >>> + op_note_hdr.op_note_descsz = bfd_get_32(ibfd, >>> + reinterpret_cast<bfd_byte *>(contents + 4)); >>> + op_note_hdr.op_note_type = bfd_get_32(ibfd, >>> + reinterpret_cast<bfd_byte *>(contents + 8)); >>> + ptr += sizeof(op_note_hdr); >>> + if ((op_note_hdr.op_note_type == NT_GNU_BUILD_ID) && >>> + (op_note_hdr.op_note_namesz == sizeof("GNU")) && >>> + (strcmp("GNU", ptr ) == 0)) { >>> + memcpy(build_id, ptr + op_note_hdr.op_note_namesz, BUILD_ID_SIZE); >>> + retval = true; >>> + cverb << vbfd << "Found build-id" << endl; >>> + break; >>> + } >>> + ptr += op_note_hdr.op_note_namesz + op_note_hdr.op_note_descsz; >>> + } >>> + if (!retval) >>> + cverb << vbfd << " No build-id found" << endl; >>> + >>> + return retval; >>> +} >>> >>> bool get_debug_link_info(bfd * ibfd, string & filename, unsigned long & crc32) >>> { >>> @@ -298,10 +395,24 @@ bool find_separate_debug_file(bfd * ibfd, string const & filepath_in, >>> { >>> string filepath(filepath_in); >>> string basename; >>> - unsigned long crc32; >>> + bool use_build_id = true; >>> + unsigned long crc32 = 0; >>> + unsigned char buildid[BUILD_ID_SIZE]; >>> >>> - if (!get_debug_link_info(ibfd, basename, crc32)) >>> - return false; >>> + if (!(use_build_id = get_build_id(ibfd, buildid))) >>> + if (!get_debug_link_info(ibfd, basename, crc32)) >>> + return false; >>> + >>> + if (use_build_id && find_debuginfo_file_by_buildid(buildid, debug_filename)) >>> + return true; >>> + >>> + /* Use old method of finding debuginfo file by comparing runtime binary's >>> + * CRC with the CRC we calculate from the debuginfo file's contents. >>> + * NOTE: This method breaks on systems where "MiniDebugInfo" is used >>> + * since the CRC stored in the runtime binary won't match the compressed >>> + * debuginfo file's CRC. But in practice, we shouldn't ever run into such >>> + * a scenario since the build-id should always be available. >>> + */ >>> >>> // Work out the image file's directory prefix >>> string filedir = op_dirname(filepath); >> >> >> ------------------------------------------------------------------------------ >> LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial >> Remotely access PCs and mobile devices and provide instant support >> Improve your efficiency, and focus on delivering more value-add services >> Discover what IT Professionals Know. Rescue delivers >> http://p.sf.net/sfu/logmein_12329d2d >> _______________________________________________ >> oprofile-list mailing list >> opr...@li... >> https://lists.sourceforge.net/lists/listinfo/oprofile-list >> > > > ------------------------------------------------------------------------------ > LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial > Remotely access PCs and mobile devices and provide instant support > Improve your efficiency, and focus on delivering more value-add services > Discover what IT Professionals Know. Rescue delivers > http://p.sf.net/sfu/logmein_12329d2d > _______________________________________________ > oprofile-list mailing list > opr...@li... > https://lists.sourceforge.net/lists/listinfo/oprofile-list > |
From: William C. <wc...@re...> - 2012-12-17 18:24:18
|
On 12/14/2012 02:17 PM, Maynard Johnson wrote: > On 12/12/2012 11:18 AM, Maynard Johnson wrote: >> On 12/11/2012 11:30 AM, Maynard Johnson wrote: >>> On 11/29/2012 03:50 PM, Maynard Johnson wrote: >>>> Add support for using build-id to locate debuginfo files >>> >>> Can someone please review this patch? Thanks. >> Suravee, Will . . . Could one of you review this patch? Thanks. > Having received no comments since this patch was posted two weeks ago, I committed it today. Review comments are still welcome. > > -Maynard >> >> -Maynard >> >>> >>> -Maynard >>>> >>>> This patch addresses the issue raised in oprofile bug #3591165. >>>> In brief, a new MiniDebuginfo feature was introduced in Fedora18 that >>>> changes how runtime RPMs and their corresponding debuginfo RPMs are built >>>> such that the CRC stored in the runtime binary no longer matches the CRC >>>> we calculate from the contents of the debuginfo file. Other tools that use >>>> debuginfo (like gdb) use the the build-id method (i.e., build-id is stored >>>> in runtime which can be used to locate the matching debuginfo file), and >>>> only use the CRC method as a fallback mechanism. This patch adds the >>>> build-id method, making it the primary mechanism for locating debuginfo >>>> files. >>>> >>>> Signed-off-by: Maynard Johnson <may...@us...> >>>> --- >>>> libutil++/bfd_support.cpp | 117 +++++++++++++++++++++++++++++++++++++++++++- >>>> 1 files changed, 114 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/libutil++/bfd_support.cpp b/libutil++/bfd_support.cpp >>>> index d1383f8..6f4e8e1 100644 >>>> --- a/libutil++/bfd_support.cpp >>>> +++ b/libutil++/bfd_support.cpp >>>> @@ -17,7 +17,10 @@ >>>> +#include <limits.h> Is there a particular need for the the <limits.h> include? Didn't see anything in the patch that needed it. >>>> +#define BUILD_ID_SIZE 20 Having a BUILD_ID_SIZE be a magic number for the size seems pretty fragile. How was the value of 20 determined to be correct? The sections should have that size information available. Why not read that size information from the section and use that instead? >>>> +#ifndef NT_GNU_BUILD_ID >>>> +#define NT_GNU_BUILD_ID 3 >>>> +#endif >>>> + >>>> >>>> void check_format(string const & file, bfd ** ibfd) >>>> { >>>> @@ -75,6 +83,95 @@ bool separate_debug_file_exists(string & name, unsigned long const crc, >>>> return crc == file_crc; >>>> } >>>> >>>> +static bool find_debuginfo_file_by_buildid(unsigned char * buildid, string & debug_filename) >>>> +{ >>>> + size_t build_id_fname_size = strlen (DEBUGDIR) + (sizeof "/.build-id/" - 1) + 1 >>>> + + (2 * BUILD_ID_SIZE) + (sizeof ".debug" - 1) + 1; >>>> + char * buildid_symlink = (char *) xmalloc(build_id_fname_size); >>>> + char * sptr = buildid_symlink; >>>> + unsigned char * bptr = buildid; >>>> + bool retval = false; >>>> + size_t build_id_segment_len = strlen("/.build-id/"); >>>> + >>>> + >>>> + memcpy(sptr, DEBUGDIR, strlen(DEBUGDIR)); >>>> + sptr += strlen(DEBUGDIR); >>>> + memcpy(sptr, "/.build-id/", build_id_segment_len); >>>> + sptr += build_id_segment_len; >>>> + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); >>>> + *sptr++ = '/'; >>>> + for (int i = BUILD_ID_SIZE - 1; i > 0; i--) >>>> + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); >>>> + >>>> + strcpy(sptr, ".debug"); >>>> + >>>> + if (access (buildid_symlink, F_OK) == 0) { >>>> + debug_filename = op_realpath (buildid_symlink); >>>> + if (debug_filename.compare(buildid_symlink)) { >>>> + retval = true; >>>> + cverb << vbfd << "Using build-id symlink" << endl; >>>> + } >>>> + } >>>> + free(buildid_symlink); >>>> + if (!retval) >>>> + cverb << vbfd << "build-id file not found; falling back to CRC method." << endl; >>>> + >>>> + return retval; >>>> +} >>>> + >>>> +static bool get_build_id(bfd * ibfd, unsigned char * build_id) >>>> +{ >>>> + struct op_elf_Note_hdr { >>>> + unsigned int op_note_namesz; >>>> + unsigned int op_note_descsz; >>>> + unsigned int op_note_type; >>>> + } op_note_hdr; >>>> + asection * sect; >>>> + char * ptr; >>>> + bool retval = false; >>>> + >>>> + cverb << vbfd << "fetching build-id from runtime binary ..."; >>>> + if (!(sect = bfd_get_section_by_name(ibfd, ".note.gnu.build-id"))) { >>>> + if (!(sect = bfd_get_section_by_name(ibfd, ".notes"))) { >>>> + cverb << vbfd << " No build-id section found" << endl; >>>> + return false; >>>> + } >>>> + } >>>> + >>>> + bfd_size_type buildid_sect_size = bfd_section_size(ibfd, sect); >>>> + char contents[buildid_sect_size]; >>>> + >>>> + if (!bfd_get_section_contents(ibfd, sect, >>>> + reinterpret_cast<unsigned char *>(contents), >>>> + static_cast<file_ptr>(0), buildid_sect_size)) { >>>> + bfd_perror("bfd_get_section_contents:get_debug:"); >>>> + exit(2); >>>> + } >>>> + >>>> + ptr = contents; >>>> + while (ptr < (contents + buildid_sect_size)) { >>>> + op_note_hdr.op_note_namesz = bfd_get_32(ibfd, >>>> + reinterpret_cast<bfd_byte *>(contents)); >>>> + op_note_hdr.op_note_descsz = bfd_get_32(ibfd, >>>> + reinterpret_cast<bfd_byte *>(contents + 4)); >>>> + op_note_hdr.op_note_type = bfd_get_32(ibfd, >>>> + reinterpret_cast<bfd_byte *>(contents + 8)); >>>> + ptr += sizeof(op_note_hdr); >>>> + if ((op_note_hdr.op_note_type == NT_GNU_BUILD_ID) && >>>> + (op_note_hdr.op_note_namesz == sizeof("GNU")) && >>>> + (strcmp("GNU", ptr ) == 0)) { >>>> + memcpy(build_id, ptr + op_note_hdr.op_note_namesz, BUILD_ID_SIZE); >>>> + retval = true; >>>> + cverb << vbfd << "Found build-id" << endl; >>>> + break; >>>> + } >>>> + ptr += op_note_hdr.op_note_namesz + op_note_hdr.op_note_descsz; >>>> + } >>>> + if (!retval) >>>> + cverb << vbfd << " No build-id found" << endl; >>>> + >>>> + return retval; >>>> +} >>>> >>>> bool get_debug_link_info(bfd * ibfd, string & filename, unsigned long & crc32) >>>> { >>>> @@ -298,10 +395,24 @@ bool find_separate_debug_file(bfd * ibfd, string const & filepath_in, >>>> { >>>> string filepath(filepath_in); >>>> string basename; >>>> - unsigned long crc32; >>>> + bool use_build_id = true; >>>> + unsigned long crc32 = 0; >>>> + unsigned char buildid[BUILD_ID_SIZE]; >>>> >>>> - if (!get_debug_link_info(ibfd, basename, crc32)) >>>> - return false; >>>> + if (!(use_build_id = get_build_id(ibfd, buildid))) >>>> + if (!get_debug_link_info(ibfd, basename, crc32)) >>>> + return false; >>>> + >>>> + if (use_build_id && find_debuginfo_file_by_buildid(buildid, debug_filename)) >>>> + return true; The logic above seems convoluted. Why not do get rid of use_build_id and do the following: if (get_build_id(ibfd, buildid) && find_debuginfo_file_by_buildid(buildid, debug_filename)) return true; if (!get_debug_link_info(ibfd, basename, crc32)) return false; >>>> + >>>> + /* Use old method of finding debuginfo file by comparing runtime binary's >>>> + * CRC with the CRC we calculate from the debuginfo file's contents. >>>> + * NOTE: This method breaks on systems where "MiniDebugInfo" is used >>>> + * since the CRC stored in the runtime binary won't match the compressed >>>> + * debuginfo file's CRC. But in practice, we shouldn't ever run into such >>>> + * a scenario since the build-id should always be available. >>>> + */ >>>> >>>> // Work out the image file's directory prefix >>>> string filedir = op_dirname(filepath); >>> >>> >>> ------------------------------------------------------------------------------ -Will |
From: Maynard J. <may...@us...> - 2012-12-17 19:51:09
|
On 12/17/2012 12:24 PM, William Cohen wrote: > On 12/14/2012 02:17 PM, Maynard Johnson wrote: >> On 12/12/2012 11:18 AM, Maynard Johnson wrote: >>> On 12/11/2012 11:30 AM, Maynard Johnson wrote: >>>> On 11/29/2012 03:50 PM, Maynard Johnson wrote: >>>>> Add support for using build-id to locate debuginfo files >>>> >>>> Can someone please review this patch? Thanks. >>> Suravee, Will . . . Could one of you review this patch? Thanks. >> Having received no comments since this patch was posted two weeks ago, I committed it today. Review comments are still welcome. >> >> -Maynard >>> >>> -Maynard >>> >>>> >>>> -Maynard >>>>> >>>>> This patch addresses the issue raised in oprofile bug #3591165. >>>>> In brief, a new MiniDebuginfo feature was introduced in Fedora18 that >>>>> changes how runtime RPMs and their corresponding debuginfo RPMs are built >>>>> such that the CRC stored in the runtime binary no longer matches the CRC >>>>> we calculate from the contents of the debuginfo file. Other tools that use >>>>> debuginfo (like gdb) use the the build-id method (i.e., build-id is stored >>>>> in runtime which can be used to locate the matching debuginfo file), and >>>>> only use the CRC method as a fallback mechanism. This patch adds the >>>>> build-id method, making it the primary mechanism for locating debuginfo >>>>> files. >>>>> >>>>> Signed-off-by: Maynard Johnson <may...@us...> >>>>> --- >>>>> libutil++/bfd_support.cpp | 117 +++++++++++++++++++++++++++++++++++++++++++- >>>>> 1 files changed, 114 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/libutil++/bfd_support.cpp b/libutil++/bfd_support.cpp >>>>> index d1383f8..6f4e8e1 100644 >>>>> --- a/libutil++/bfd_support.cpp >>>>> +++ b/libutil++/bfd_support.cpp >>>>> @@ -17,7 +17,10 @@ > >>>>> +#include <limits.h> > > Is there a particular need for the the <limits.h> include? Didn't see anything in the patch that needed it. No . . . must have been a residual from something I tried. Removed. > > > >>>>> +#define BUILD_ID_SIZE 20 > > Having a BUILD_ID_SIZE be a magic number for the size seems pretty fragile. How was the value of 20 determined to be correct? The sections should have that size information available. Why not read that size information from the section and use that instead? Changed to a variable based on data in the build-id section. > >>>>> +#ifndef NT_GNU_BUILD_ID >>>>> +#define NT_GNU_BUILD_ID 3 >>>>> +#endif >>>>> + >>>>> >>>>> void check_format(string const & file, bfd ** ibfd) >>>>> { >>>>> @@ -75,6 +83,95 @@ bool separate_debug_file_exists(string & name, unsigned long const crc, >>>>> return crc == file_crc; >>>>> } >>>>> >>>>> +static bool find_debuginfo_file_by_buildid(unsigned char * buildid, string & debug_filename) >>>>> +{ >>>>> + size_t build_id_fname_size = strlen (DEBUGDIR) + (sizeof "/.build-id/" - 1) + 1 >>>>> + + (2 * BUILD_ID_SIZE) + (sizeof ".debug" - 1) + 1; > > >>>>> + char * buildid_symlink = (char *) xmalloc(build_id_fname_size); >>>>> + char * sptr = buildid_symlink; >>>>> + unsigned char * bptr = buildid; >>>>> + bool retval = false; >>>>> + size_t build_id_segment_len = strlen("/.build-id/"); >>>>> + >>>>> + >>>>> + memcpy(sptr, DEBUGDIR, strlen(DEBUGDIR)); >>>>> + sptr += strlen(DEBUGDIR); >>>>> + memcpy(sptr, "/.build-id/", build_id_segment_len); >>>>> + sptr += build_id_segment_len; >>>>> + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); >>>>> + *sptr++ = '/'; >>>>> + for (int i = BUILD_ID_SIZE - 1; i > 0; i--) >>>>> + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); >>>>> + >>>>> + strcpy(sptr, ".debug"); >>>>> + >>>>> + if (access (buildid_symlink, F_OK) == 0) { >>>>> + debug_filename = op_realpath (buildid_symlink); >>>>> + if (debug_filename.compare(buildid_symlink)) { >>>>> + retval = true; >>>>> + cverb << vbfd << "Using build-id symlink" << endl; >>>>> + } >>>>> + } >>>>> + free(buildid_symlink); >>>>> + if (!retval) >>>>> + cverb << vbfd << "build-id file not found; falling back to CRC method." << endl; >>>>> + >>>>> + return retval; >>>>> +} >>>>> + >>>>> +static bool get_build_id(bfd * ibfd, unsigned char * build_id) >>>>> +{ >>>>> + struct op_elf_Note_hdr { >>>>> + unsigned int op_note_namesz; >>>>> + unsigned int op_note_descsz; >>>>> + unsigned int op_note_type; >>>>> + } op_note_hdr; >>>>> + asection * sect; >>>>> + char * ptr; >>>>> + bool retval = false; >>>>> + >>>>> + cverb << vbfd << "fetching build-id from runtime binary ..."; >>>>> + if (!(sect = bfd_get_section_by_name(ibfd, ".note.gnu.build-id"))) { >>>>> + if (!(sect = bfd_get_section_by_name(ibfd, ".notes"))) { >>>>> + cverb << vbfd << " No build-id section found" << endl; >>>>> + return false; >>>>> + } >>>>> + } >>>>> + >>>>> + bfd_size_type buildid_sect_size = bfd_section_size(ibfd, sect); >>>>> + char contents[buildid_sect_size]; >>>>> + >>>>> + if (!bfd_get_section_contents(ibfd, sect, >>>>> + reinterpret_cast<unsigned char *>(contents), >>>>> + static_cast<file_ptr>(0), buildid_sect_size)) { >>>>> + bfd_perror("bfd_get_section_contents:get_debug:"); >>>>> + exit(2); >>>>> + } >>>>> + >>>>> + ptr = contents; >>>>> + while (ptr < (contents + buildid_sect_size)) { >>>>> + op_note_hdr.op_note_namesz = bfd_get_32(ibfd, >>>>> + reinterpret_cast<bfd_byte *>(contents)); >>>>> + op_note_hdr.op_note_descsz = bfd_get_32(ibfd, >>>>> + reinterpret_cast<bfd_byte *>(contents + 4)); >>>>> + op_note_hdr.op_note_type = bfd_get_32(ibfd, >>>>> + reinterpret_cast<bfd_byte *>(contents + 8)); >>>>> + ptr += sizeof(op_note_hdr); >>>>> + if ((op_note_hdr.op_note_type == NT_GNU_BUILD_ID) && >>>>> + (op_note_hdr.op_note_namesz == sizeof("GNU")) && >>>>> + (strcmp("GNU", ptr ) == 0)) { >>>>> + memcpy(build_id, ptr + op_note_hdr.op_note_namesz, BUILD_ID_SIZE); >>>>> + retval = true; >>>>> + cverb << vbfd << "Found build-id" << endl; >>>>> + break; >>>>> + } >>>>> + ptr += op_note_hdr.op_note_namesz + op_note_hdr.op_note_descsz; >>>>> + } >>>>> + if (!retval) >>>>> + cverb << vbfd << " No build-id found" << endl; >>>>> + >>>>> + return retval; >>>>> +} >>>>> >>>>> bool get_debug_link_info(bfd * ibfd, string & filename, unsigned long & crc32) >>>>> { >>>>> @@ -298,10 +395,24 @@ bool find_separate_debug_file(bfd * ibfd, string const & filepath_in, >>>>> { >>>>> string filepath(filepath_in); >>>>> string basename; >>>>> - unsigned long crc32; >>>>> + bool use_build_id = true; >>>>> + unsigned long crc32 = 0; >>>>> + unsigned char buildid[BUILD_ID_SIZE]; >>>>> >>>>> - if (!get_debug_link_info(ibfd, basename, crc32)) >>>>> - return false; >>>>> + if (!(use_build_id = get_build_id(ibfd, buildid))) >>>>> + if (!get_debug_link_info(ibfd, basename, crc32)) >>>>> + return false; >>>>> + >>>>> + if (use_build_id && find_debuginfo_file_by_buildid(buildid, debug_filename)) >>>>> + return true; > > The logic above seems convoluted. Why not do get rid of use_build_id and do the following: > > > if (get_build_id(ibfd, buildid) && > find_debuginfo_file_by_buildid(buildid, debug_filename)) > return true; > > if (!get_debug_link_info(ibfd, basename, crc32)) > return false; Yeah, sure, that seems more clear. I'll post a patch soon. Thanks for reviewing. -Maynard > > >>>>> + >>>>> + /* Use old method of finding debuginfo file by comparing runtime binary's >>>>> + * CRC with the CRC we calculate from the debuginfo file's contents. >>>>> + * NOTE: This method breaks on systems where "MiniDebugInfo" is used >>>>> + * since the CRC stored in the runtime binary won't match the compressed >>>>> + * debuginfo file's CRC. But in practice, we shouldn't ever run into such >>>>> + * a scenario since the build-id should always be available. >>>>> + */ >>>>> >>>>> // Work out the image file's directory prefix >>>>> string filedir = op_dirname(filepath); >>>> >>>> >>>> ------------------------------------------------------------------------------ > > -Will > |
From: Suravee S. <sur...@am...> - 2012-12-18 07:55:46
|
Maynard, I must have missed your email last week. I saw that Will already gave you a review. I have a couple minor point to add. Please see my comments below. Suravee On Thu, 2012-11-29 at 15:50 -0600, Maynard Johnson wrote: > Add support for using build-id to locate debuginfo files > > This patch addresses the issue raised in oprofile bug #3591165. > In brief, a new MiniDebuginfo feature was introduced in Fedora18 that > changes how runtime RPMs and their corresponding debuginfo RPMs are built > such that the CRC stored in the runtime binary no longer matches the CRC > we calculate from the contents of the debuginfo file. Other tools that use > debuginfo (like gdb) use the the build-id method (i.e., build-id is stored > in runtime which can be used to locate the matching debuginfo file), and > only use the CRC method as a fallback mechanism. This patch adds the > build-id method, making it the primary mechanism for locating debuginfo > files. > > Signed-off-by: Maynard Johnson <may...@us...> > --- > libutil++/bfd_support.cpp | 117 +++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 114 insertions(+), 3 deletions(-) > > diff --git a/libutil++/bfd_support.cpp b/libutil++/bfd_support.cpp > index d1383f8..6f4e8e1 100644 > --- a/libutil++/bfd_support.cpp > +++ b/libutil++/bfd_support.cpp > @@ -17,7 +17,10 @@ > #include "file_manip.h" > #include "cverb.h" > #include "locate_images.h" > +#include "op_libiberty.h" > > +#include <unistd.h> > +#include <limits.h> > #include <cstdlib> > #include <cstring> > #include <cassert> > @@ -34,6 +37,11 @@ extern verbose vbfd; > > namespace { > > +#define BUILD_ID_SIZE 20 > +#ifndef NT_GNU_BUILD_ID > +#define NT_GNU_BUILD_ID 3 > +#endif > + > > void check_format(string const & file, bfd ** ibfd) > { > @@ -75,6 +83,95 @@ bool separate_debug_file_exists(string & name, unsigned long const crc, > return crc == file_crc; > } > > +static bool find_debuginfo_file_by_buildid(unsigned char * buildid, string & debug_filename) > +{ > + size_t build_id_fname_size = strlen (DEBUGDIR) + (sizeof "/.build-id/" - 1) + 1 > + + (2 * BUILD_ID_SIZE) + (sizeof ".debug" - 1) + 1; > + char * buildid_symlink = (char *) xmalloc(build_id_fname_size); > + char * sptr = buildid_symlink; > + unsigned char * bptr = buildid; > + bool retval = false; > + size_t build_id_segment_len = strlen("/.build-id/"); > + > + > + memcpy(sptr, DEBUGDIR, strlen(DEBUGDIR)); > + sptr += strlen(DEBUGDIR); > + memcpy(sptr, "/.build-id/", build_id_segment_len); > + sptr += build_id_segment_len; > + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); > + *sptr++ = '/'; > + for (int i = BUILD_ID_SIZE - 1; i > 0; i--) > + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); > + > + strcpy(sptr, ".debug"); > + > + if (access (buildid_symlink, F_OK) == 0) { > + debug_filename = op_realpath (buildid_symlink); > + if (debug_filename.compare(buildid_symlink)) { > + retval = true; > + cverb << vbfd << "Using build-id symlink" << endl; > + } > + } > + free(buildid_symlink); > + if (!retval) > + cverb << vbfd << "build-id file not found; falling back to CRC method." << endl; > + > + return retval; > +} > + > +static bool get_build_id(bfd * ibfd, unsigned char * build_id) > +{ > + struct op_elf_Note_hdr { > + unsigned int op_note_namesz; > + unsigned int op_note_descsz; > + unsigned int op_note_type; > + } op_note_hdr; > + asection * sect; > + char * ptr; > + bool retval = false; > + > + cverb << vbfd << "fetching build-id from runtime binary ..."; > + if (!(sect = bfd_get_section_by_name(ibfd, ".note.gnu.build-id"))) { > + if (!(sect = bfd_get_section_by_name(ibfd, ".notes"))) { > + cverb << vbfd << " No build-id section found" << endl; > + return false; > + } > + } > + > + bfd_size_type buildid_sect_size = bfd_section_size(ibfd, sect); > + char contents[buildid_sect_size]; [SURAVEE] the "contents" char array should not be declared with variable "build_id_sect_size" which is variable size. This could lead to memory issue. > + > + if (!bfd_get_section_contents(ibfd, sect, > + reinterpret_cast<unsigned char *>(contents), > + static_cast<file_ptr>(0), buildid_sect_size)) { > + bfd_perror("bfd_get_section_contents:get_debug:"); > + exit(2); [SURAVEE] Normally, I dont think library should return with "exit". But may be if you want to exit, we could use "EXIT_FAILURE" here. > + } > + > + ptr = contents; > + while (ptr < (contents + buildid_sect_size)) { > + op_note_hdr.op_note_namesz = bfd_get_32(ibfd, > + reinterpret_cast<bfd_byte *>(contents)); > + op_note_hdr.op_note_descsz = bfd_get_32(ibfd, > + reinterpret_cast<bfd_byte *>(contents + 4)); > + op_note_hdr.op_note_type = bfd_get_32(ibfd, > + reinterpret_cast<bfd_byte *>(contents + 8)); [SURAVEE] I'm not quite familiar with this part. Where is the structure of .note.gnu.build-id.section defined (i.e. namesz, descsz, note_type). Suravee > + ptr += sizeof(op_note_hdr); > + if ((op_note_hdr.op_note_type == NT_GNU_BUILD_ID) && > + (op_note_hdr.op_note_namesz == sizeof("GNU")) && > + (strcmp("GNU", ptr ) == 0)) { > + memcpy(build_id, ptr + op_note_hdr.op_note_namesz, BUILD_ID_SIZE); > + retval = true; > + cverb << vbfd << "Found build-id" << endl; > + break; > + } > + ptr += op_note_hdr.op_note_namesz + op_note_hdr.op_note_descsz; > + } > + if (!retval) > + cverb << vbfd << " No build-id found" << endl; > + > + return retval; > +} > > bool get_debug_link_info(bfd * ibfd, string & filename, unsigned long & crc32) > { > @@ -298,10 +395,24 @@ bool find_separate_debug_file(bfd * ibfd, string const & filepath_in, > { > string filepath(filepath_in); > string basename; > - unsigned long crc32; > + bool use_build_id = true; > + unsigned long crc32 = 0; > + unsigned char buildid[BUILD_ID_SIZE]; > > - if (!get_debug_link_info(ibfd, basename, crc32)) > - return false; > + if (!(use_build_id = get_build_id(ibfd, buildid))) > + if (!get_debug_link_info(ibfd, basename, crc32)) > + return false; > + > + if (use_build_id && find_debuginfo_file_by_buildid(buildid, debug_filename)) > + return true; > + > + /* Use old method of finding debuginfo file by comparing runtime binary's > + * CRC with the CRC we calculate from the debuginfo file's contents. > + * NOTE: This method breaks on systems where "MiniDebugInfo" is used > + * since the CRC stored in the runtime binary won't match the compressed > + * debuginfo file's CRC. But in practice, we shouldn't ever run into such > + * a scenario since the build-id should always be available. > + */ > > // Work out the image file's directory prefix > string filedir = op_dirname(filepath); |
From: Maynard J. <may...@us...> - 2012-12-18 15:10:34
|
On 12/18/2012 01:40 AM, Suravee Suthikulpanit wrote: > Maynard, > > I must have missed your email last week. I saw that Will already gave > you a review. I have a couple minor point to add. Please see my comments > below. Thanks. Responses below. -Maynard > > Suravee > > On Thu, 2012-11-29 at 15:50 -0600, Maynard Johnson wrote: >> Add support for using build-id to locate debuginfo files >> >> This patch addresses the issue raised in oprofile bug #3591165. >> In brief, a new MiniDebuginfo feature was introduced in Fedora18 that >> changes how runtime RPMs and their corresponding debuginfo RPMs are built >> such that the CRC stored in the runtime binary no longer matches the CRC >> we calculate from the contents of the debuginfo file. Other tools that use >> debuginfo (like gdb) use the the build-id method (i.e., build-id is stored >> in runtime which can be used to locate the matching debuginfo file), and >> only use the CRC method as a fallback mechanism. This patch adds the >> build-id method, making it the primary mechanism for locating debuginfo >> files. >> >> Signed-off-by: Maynard Johnson <may...@us...> >> --- >> libutil++/bfd_support.cpp | 117 +++++++++++++++++++++++++++++++++++++++++++- >> 1 files changed, 114 insertions(+), 3 deletions(-) >> >> diff --git a/libutil++/bfd_support.cpp b/libutil++/bfd_support.cpp >> index d1383f8..6f4e8e1 100644 >> --- a/libutil++/bfd_support.cpp >> +++ b/libutil++/bfd_support.cpp >> @@ -17,7 +17,10 @@ >> #include "file_manip.h" >> #include "cverb.h" >> #include "locate_images.h" >> +#include "op_libiberty.h" >> >> +#include <unistd.h> >> +#include <limits.h> >> #include <cstdlib> >> #include <cstring> >> #include <cassert> >> @@ -34,6 +37,11 @@ extern verbose vbfd; >> >> namespace { >> >> +#define BUILD_ID_SIZE 20 >> +#ifndef NT_GNU_BUILD_ID >> +#define NT_GNU_BUILD_ID 3 >> +#endif >> + >> >> void check_format(string const & file, bfd ** ibfd) >> { >> @@ -75,6 +83,95 @@ bool separate_debug_file_exists(string & name, unsigned long const crc, >> return crc == file_crc; >> } >> >> +static bool find_debuginfo_file_by_buildid(unsigned char * buildid, string & debug_filename) >> +{ >> + size_t build_id_fname_size = strlen (DEBUGDIR) + (sizeof "/.build-id/" - 1) + 1 >> + + (2 * BUILD_ID_SIZE) + (sizeof ".debug" - 1) + 1; >> + char * buildid_symlink = (char *) xmalloc(build_id_fname_size); >> + char * sptr = buildid_symlink; >> + unsigned char * bptr = buildid; >> + bool retval = false; >> + size_t build_id_segment_len = strlen("/.build-id/"); >> + >> + >> + memcpy(sptr, DEBUGDIR, strlen(DEBUGDIR)); >> + sptr += strlen(DEBUGDIR); >> + memcpy(sptr, "/.build-id/", build_id_segment_len); >> + sptr += build_id_segment_len; >> + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); >> + *sptr++ = '/'; >> + for (int i = BUILD_ID_SIZE - 1; i > 0; i--) >> + sptr += sprintf(sptr, "%02x", (unsigned) *bptr++); >> + >> + strcpy(sptr, ".debug"); >> + >> + if (access (buildid_symlink, F_OK) == 0) { >> + debug_filename = op_realpath (buildid_symlink); >> + if (debug_filename.compare(buildid_symlink)) { >> + retval = true; >> + cverb << vbfd << "Using build-id symlink" << endl; >> + } >> + } >> + free(buildid_symlink); >> + if (!retval) >> + cverb << vbfd << "build-id file not found; falling back to CRC method." << endl; >> + >> + return retval; >> +} >> + >> +static bool get_build_id(bfd * ibfd, unsigned char * build_id) >> +{ >> + struct op_elf_Note_hdr { >> + unsigned int op_note_namesz; >> + unsigned int op_note_descsz; >> + unsigned int op_note_type; >> + } op_note_hdr; >> + asection * sect; >> + char * ptr; >> + bool retval = false; >> + >> + cverb << vbfd << "fetching build-id from runtime binary ..."; >> + if (!(sect = bfd_get_section_by_name(ibfd, ".note.gnu.build-id"))) { >> + if (!(sect = bfd_get_section_by_name(ibfd, ".notes"))) { >> + cverb << vbfd << " No build-id section found" << endl; >> + return false; >> + } >> + } >> + >> + bfd_size_type buildid_sect_size = bfd_section_size(ibfd, sect); >> + char contents[buildid_sect_size]; > [SURAVEE] the "contents" char array should not be declared with variable > "build_id_sect_size" which is variable size. This could lead to memory > issue. Quite right. I'll change to use dynamically allocated memory. > >> + >> + if (!bfd_get_section_contents(ibfd, sect, >> + reinterpret_cast<unsigned char *>(contents), >> + static_cast<file_ptr>(0), buildid_sect_size)) { >> + bfd_perror("bfd_get_section_contents:get_debug:"); >> + exit(2); > [SURAVEE] Normally, I dont think library should return with "exit". But > may be if you want to exit, we could use "EXIT_FAILURE" here. Original code in get_debug_link_info() was doing 'bfd_perror" and then exit(2) if section contents could not be read, so I just did the same here. I think both of these should be changed to op_fatal_error(msg). > >> + } >> + >> + ptr = contents; >> + while (ptr < (contents + buildid_sect_size)) { >> + op_note_hdr.op_note_namesz = bfd_get_32(ibfd, >> + reinterpret_cast<bfd_byte *>(contents)); >> + op_note_hdr.op_note_descsz = bfd_get_32(ibfd, >> + reinterpret_cast<bfd_byte *>(contents + 4)); >> + op_note_hdr.op_note_type = bfd_get_32(ibfd, >> + reinterpret_cast<bfd_byte *>(contents + 8)); > [SURAVEE] I'm not quite familiar with this part. Where is the structure > of .note.gnu.build-id.section defined (i.e. namesz, descsz, note_type). In elf.h, there are the following typedefs: typedef struct { Elf32_Word n_namesz; /* Length of the note's name. */ Elf32_Word n_descsz; /* Length of the note's descriptor. */ Elf32_Word n_type; /* Type of the note. */ } Elf32_Nhdr; typedef struct { Elf64_Word n_namesz; /* Length of the note's name. */ Elf64_Word n_descsz; /* Length of the note's descriptor. */ Elf64_Word n_type; /* Type of the note. */ } Elf64_Nhdr; Both Elf64_Word and Elf32_Word are defined as uint32_t, so they're really equivalent definitions. I defined op_note_hdr above, based on these definitions, but in retrospect, I should have probably just picked one of the typedefs already defined in elf.h. I'll make that change. > > Suravee > >> + ptr += sizeof(op_note_hdr); >> + if ((op_note_hdr.op_note_type == NT_GNU_BUILD_ID) && >> + (op_note_hdr.op_note_namesz == sizeof("GNU")) && >> + (strcmp("GNU", ptr ) == 0)) { >> + memcpy(build_id, ptr + op_note_hdr.op_note_namesz, BUILD_ID_SIZE); >> + retval = true; >> + cverb << vbfd << "Found build-id" << endl; >> + break; >> + } >> + ptr += op_note_hdr.op_note_namesz + op_note_hdr.op_note_descsz; >> + } >> + if (!retval) >> + cverb << vbfd << " No build-id found" << endl; >> + >> + return retval; >> +} >> >> bool get_debug_link_info(bfd * ibfd, string & filename, unsigned long & crc32) >> { >> @@ -298,10 +395,24 @@ bool find_separate_debug_file(bfd * ibfd, string const & filepath_in, >> { >> string filepath(filepath_in); >> string basename; >> - unsigned long crc32; >> + bool use_build_id = true; >> + unsigned long crc32 = 0; >> + unsigned char buildid[BUILD_ID_SIZE]; >> >> - if (!get_debug_link_info(ibfd, basename, crc32)) >> - return false; >> + if (!(use_build_id = get_build_id(ibfd, buildid))) >> + if (!get_debug_link_info(ibfd, basename, crc32)) >> + return false; >> + >> + if (use_build_id && find_debuginfo_file_by_buildid(buildid, debug_filename)) >> + return true; >> + >> + /* Use old method of finding debuginfo file by comparing runtime binary's >> + * CRC with the CRC we calculate from the debuginfo file's contents. >> + * NOTE: This method breaks on systems where "MiniDebugInfo" is used >> + * since the CRC stored in the runtime binary won't match the compressed >> + * debuginfo file's CRC. But in practice, we shouldn't ever run into such >> + * a scenario since the build-id should always be available. >> + */ >> >> // Work out the image file's directory prefix >> string filedir = op_dirname(filepath); > > > |