Menu
▾
▴
Re: [PATCH] Add support for using build-id to locate debuginfo files
|
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
|