Attached patch addresses your comments. See inline for details. On 13-03-31 01:43 PM, Jan Kratochvil wrote: > On Thu, 28 Mar 2013 21:53:38 +0100, Aleksandar Ristovski wrote: >> Fixed patch. I haven't addressed any of your concerns except fixed >> what was broken. There are two things changed: >> >> 1) get_dynamic, you will see I left it unfinished when switched to >> generic "find_phdr" to address phdr traversal duplication code. > > I wrote at "find_phdr_p": > But I do not understand why this function exists > > which probably corresponds to this your comment. > > >> 2) lrfind_mapping_entry can not check for vaddr + offset as offset >> is file offset, and for some shared objects this will not match even >> though the vaddr of the entry with zero offset is valid. > > Oops, you are right: > > 7ffff7ddc000-7ffff7dfd000 r-xp 00000000 fd:01 51415762 /usr/lib64/ld-2.17.so > 7ffff7ffc000-7ffff7ffe000 rw-p 00020000 fd:01 51415762 /usr/lib64/ld-2.17.so > (gdb) p/x 0x7ffff7ffc000-0x7ffff7ddc000 > $1 = 0x220000 > > It is mapped by additional displacement of 2MB. Adjustment is suggested below. > > > BTW for gdbserver compatibility I have posted: > [patch 1/2+7.6] /proc/PID/smaps: filename fix > http://sourceware.org/ml/gdb-patches/2013-03/msg01120.html > [patch 2/2+7.6] /proc/PID/smaps: Linux kernel 3.8.3 compat. > http://sourceware.org/ml/gdb-patches/2013-03/msg01119.html > but you probably did not face this compat. problem. > > >> Stepping through code now shows some of the things you couldn't see, >> like e.g. why is there so->build_id, and where is it being set (you >> couldn't see it being set before as qXfer_library was broken). >> >> >> --- >> Aleksandar >> >> >> >> On 13-03-27 04:17 PM, Aleksandar Ristovski wrote: >>> Addressed Jan's comments. >>> >>> >>> >>> On 13-03-27 10:50 AM, Jan Kratochvil wrote: >>>> On Wed, 27 Mar 2013 15:38:29 +0100, Aleksandar Ristovski wrote: >>>>> On 13-03-26 04:41 PM, Jan Kratochvil wrote: >>>>>>>> + if (build_id_list_p) >>>>>>>> + qsort (VEC_address (build_id_list_s, data.list), >>>>>>>> + VEC_length (build_id_list_s, data.list), >>>>>>>> + sizeof (build_id_list_s), compare_build_id_list); >>>>>> It is always already sorted by Linux kernel, rather a for cycle to >>>>>> verify it >>>>>> really is sorted. >>>>> >>>>> Can we guarantee this is always the case? >>>> >>>> Yes. >>>> >>>> The problem is that if it is unsorted there is a bug somewhere and >>>> that qsort >>>> will hide that bug. >>> >>> >>> Qsort removed. I didn't put any traversal; we are making assumption that >>> the list will be sorted. The checks in the other bits make sure that we >>> either find the right mapping or none at all, so worst case scenario is >>> we don't get build-id communicated to gdb. >>> >>> >>> >>> Thanks, >>> >>> Aleksandar >>> >> > >> >From 80cd24335bcff6625b5c69c1b2f2d43142db08d1 Mon Sep 17 00:00:00 2001 >> From: Aleksandar Ristovski >> Date: Wed, 27 Mar 2013 11:56:57 -0400 >> Subject: [PATCH 6/8] gdbserver build-id attribute generator >> >> * doc/gdb.texinfo (Library List Format for SVR4 Targets): Add >> 'build-id' in description, example, new attribute in dtd. >> * features/library-list-svr4.dtd (library-list-svr4): New >> 'build-id' attribute. >> * linux-low.c (linux-maps.h, search.h): Include. >> (find_phdr_p_ftype, find_phdr, find_phdr_p): New. >> (get_dynamic): Use find_pdhr to traverse program headers. >> (struct mapping_entry): New structure. >> (mapping_entry_s): New typedef, new vector type def. >> (free_mapping_entry, compare_mapping_entry, >> compare_mapping_entry_range, compare_mapping_entry_inode): New. >> (struct find_memory_region_callback_data): New. >> (find_memory_region_callback): New fwd. declaration. >> (read_build_id, find_memory_region_callback, get_hex_build_id): New. >> (linux_qxfer_libraries_svr4): Add optional build-id attribute >> to reply XML document. >> --- >> gdb/doc/gdb.texinfo | 17 +- >> gdb/features/library-list-svr4.dtd | 13 +- >> gdb/gdbserver/linux-low.c | 388 +++++++++++++++++++++++++++++++++--- >> 3 files changed, 381 insertions(+), 37 deletions(-) >> >> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo >> index 38ce259..7c17209 100644 >> --- a/gdb/doc/gdb.texinfo >> +++ b/gdb/doc/gdb.texinfo >> @@ -40323,6 +40323,8 @@ memory address. It is a displacement of absolute memory address against >> address the file was prelinked to during the library load. >> @item >> @code{l_ld}, which is memory address of the @code{PT_DYNAMIC} segment >> +@item >> +@code{build-id}, hex encoded @code{NT_GNU_BUID_ID} note, if it exists. > > Typo: NT_GNU_BUILD_ID [AR] Done. > > >> @end itemize >> >> Additionally the single @code{main-lm} attribute specifies address of >> @@ -40340,7 +40342,7 @@ looks like this: >> > l_ld="0xe4eefc"/> >> > - l_ld="0x152350"/> >> + l_ld="0x152350" build-id="9afccf7cc41e6293476223fe72480854"/> >> >> @end smallexample >> >> @@ -40349,13 +40351,14 @@ The format of an SVR4 library list is described by this DTD: >> @smallexample >> >> >> - >> - >> + >> + >> >> - >> - >> - >> - >> + >> + >> + >> + >> + >> @end smallexample >> >> @node Memory Map Format >> diff --git a/gdb/features/library-list-svr4.dtd b/gdb/features/library-list-svr4.dtd >> index cae7fd8..fdd6ec0 100644 >> --- a/gdb/features/library-list-svr4.dtd >> +++ b/gdb/features/library-list-svr4.dtd >> @@ -6,11 +6,12 @@ >> >> >> >> - >> - >> + >> + >> >> >> - >> - >> - >> - >> + >> + >> + >> + >> + >> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c >> index 72c51e0..aa248e9 100644 >> --- a/gdb/gdbserver/linux-low.c >> +++ b/gdb/gdbserver/linux-low.c >> @@ -20,6 +20,7 @@ >> #include "linux-low.h" >> #include "linux-osdata.h" >> #include "agent.h" >> +#include "linux-maps.h" >> >> #include "gdb_wait.h" >> #include >> @@ -43,6 +44,7 @@ >> #include "gdb_stat.h" >> #include >> #include >> +#include >> #ifndef ELFMAG0 >> /* Don't include here. If it got included by gdb_proc_service.h >> then ELFMAG0 will have been defined. If it didn't get included by >> @@ -5432,15 +5434,81 @@ get_phdr_phnum_from_proc_auxv (const int pid, const int is_elf64, >> return 0; >> } >> >> + > > Extra empty line, there should be only one empty line. > [AR] Ok. > >> +/* Predicate function type returns 1 if the given phdr is what is >> + being looked for. Returns 0 otherwise. */ >> + >> +typedef int (*find_phdr_p_ftype)(const void *phdr, int is_elf64, >> + const void *data); > > GNU Coding Standards formatting: > typedef int (*find_phdr_p_ftype) (const void *phdr, int is_elf64, > > GDB uses *_ftype types without that * pointer, > use then find_phdr_p_ftype *find_phdr_p. [AR] Ok. > >> + >> +/* Linearly traverse pheaders given in PHDR until supplied > program headers given between PHDR_BEGIN and PHDR_END > >> + predicate function returns 1. If supplied predicate function >> + did return 1, stop traversal and return that PHDR. */ > that program header. > >> + >> +static const void * >> +find_phdr (int is_elf64, const void *const phdr_begin, >> + const void *const phdr_end, find_phdr_p_ftype find_phdr_p, > > Use find_phdr_p_ftype *find_phdr_p. > >> + const void *const data) >> +{ >> +#define SIZEOFHDR(hdr) (is_elf64? sizeof((hdr)._64) : sizeof((hdr)._32)) > > GNU Coding Standards formatting: > #define SIZEOFHDR(hdr) (is_elf64 ? sizeof ((hdr)._64) : sizeof ((hdr)._32)) > > But in fact I do not see why you define a macro which is used only once. [AR]: For readability, so the generic part of the code does not mention "64" or "32". > > >> +#define PHDR_NEXT(hdrp) ((void *) ((char *)(hdrp) + SIZEOFHDR(*hdrp))) > > GNU Coding Standards formatting and also parentheses around each macro > parameter: > #define PHDR_NEXT(hdrp) ((void *) ((char *) (hdrp) + SIZEOFHDR (*hdrp))) > > But as GDB codebase allows void * arithmetics it can be simplified to (also > putting there const otherwise it deconstifies the passed in pointer): > #define PHDR_NEXT(hdrp) (((const void *) (hdrp) + SIZEOFHDR (*hdrp))) [AR]: void* arithmetic is not defined even if gcc allows it. Changed to casting to const gdb_byte for the arithmetic part. > > >> + >> + union ElfXX_Phdr >> + { >> + Elf32_Phdr _32; >> + Elf64_Phdr _64; >> + } const *phdr = phdr_begin; >> + >> + if (phdr == NULL) >> + return NULL; >> + >> + while (PHDR_NEXT (phdr) <= phdr_end) >> + { >> + if (find_phdr_p (phdr, is_elf64, data) == 1) >> + return phdr; >> + phdr = PHDR_NEXT (phdr); >> + } >> + >> + return NULL; >> +#undef PHDR_NEXT >> +#undef SIZEOFHDR >> +} >> + > > Missing function comment. [AR] Ok. > > >> + >> +static int >> +find_phdr_p (const void *const phdr, const int is_elf64, >> + const void *const data) > > But I do not understand why this function exists - it could be integrated in > find_phdr as very every caller of find_phdr passse as find_phdr_p parameter > this find_phdr_p implementation. [AR] Yes, but I am eyeing solib-svr4.c loops over pheaders generalization - find_phdr could be moved to 'common' and possibly called 'iterate_phdrs' with the ability to pass in any function, not necessarily a predicate only. E.g. svr4_exec_displacement, etc...) > > >> +{ >> + const ULONGEST *const type = data; >> + >> + if (is_elf64) >> + { >> + const Elf64_Phdr *const p = phdr; >> + >> + if (p->p_type == *type) >> + return 1; >> + } >> + else >> + { >> + const Elf32_Phdr *const p = phdr; >> + >> + if (p->p_type == *type) >> + return 1; >> + } >> + return 0; >> +} >> + >> /* Return &_DYNAMIC (via PT_DYNAMIC) in the inferior, or 0 if not present. */ >> >> static CORE_ADDR >> get_dynamic (const int pid, const int is_elf64) >> { >> CORE_ADDR phdr_memaddr, relocation; >> - int num_phdr, i; >> + int num_phdr; >> unsigned char *phdr_buf; >> const int phdr_size = is_elf64 ? sizeof (Elf64_Phdr) : sizeof (Elf32_Phdr); >> + const void *phdr; >> + ULONGEST p_type; >> >> if (get_phdr_phnum_from_proc_auxv (pid, is_elf64, &phdr_memaddr, &num_phdr)) >> return 0; >> @@ -5454,21 +5522,24 @@ get_dynamic (const int pid, const int is_elf64) >> /* Compute relocation: it is expected to be 0 for "regular" executables, >> non-zero for PIE ones. */ >> relocation = -1; >> - for (i = 0; relocation == -1 && i < num_phdr; i++) >> - if (is_elf64) >> - { >> - Elf64_Phdr *const p = (Elf64_Phdr *) (phdr_buf + i * phdr_size); >> + p_type = PT_PHDR; >> + phdr = find_phdr (is_elf64, phdr_buf, phdr_buf + num_phdr * phdr_size, >> + find_phdr_p, &p_type); >> + if (phdr != NULL) >> + { >> + if (is_elf64) >> + { >> + const Elf64_Phdr *const p = phdr; >> >> - if (p->p_type == PT_PHDR) >> relocation = phdr_memaddr - p->p_vaddr; >> - } >> - else >> - { >> - Elf32_Phdr *const p = (Elf32_Phdr *) (phdr_buf + i * phdr_size); >> + } >> + else >> + { >> + const Elf32_Phdr *const p = phdr; >> >> - if (p->p_type == PT_PHDR) >> relocation = phdr_memaddr - p->p_vaddr; >> - } >> + } >> + } >> >> if (relocation == -1) >> { >> @@ -5485,21 +5556,23 @@ get_dynamic (const int pid, const int is_elf64) >> return 0; >> } >> >> - for (i = 0; i < num_phdr; i++) >> + p_type = PT_DYNAMIC; >> + phdr = find_phdr (is_elf64, phdr_buf, phdr_buf + num_phdr * phdr_size, >> + find_phdr_p, &p_type); >> + >> + if (phdr != NULL) >> { >> if (is_elf64) >> { >> - Elf64_Phdr *const p = (Elf64_Phdr *) (phdr_buf + i * phdr_size); >> + const Elf64_Phdr *const p = phdr; >> >> - if (p->p_type == PT_DYNAMIC) >> - return p->p_vaddr + relocation; >> + return p->p_vaddr + relocation; >> } >> else >> { >> - Elf32_Phdr *const p = (Elf32_Phdr *) (phdr_buf + i * phdr_size); >> + const Elf32_Phdr *const p = phdr; >> >> - if (p->p_type == PT_DYNAMIC) >> - return p->p_vaddr + relocation; >> + return p->p_vaddr + relocation; >> } >> } >> >> @@ -5641,6 +5714,255 @@ struct link_map_offsets >> int l_prev_offset; >> }; >> >> + >> +/* Structure for holding a mapping. Only mapping >> + containing l_ld can have hex_build_id set. >> + >> + Fields are populated from linux_find_memory_region parameters. */ >> + >> +struct mapping_entry >> +{ > > Here should be the line: > /* Fields are populated from linux_find_memory_region parameters. */ [AR] ok. > > >> + ULONGEST vaddr; >> + ULONGEST size; >> + ULONGEST offset; >> + ULONGEST inode; >> + >> + /* Hex encoded string allocated using xmalloc, and >> + needs to be freed. It can be NULL. */ >> + >> + char *hex_build_id; >> +}; >> + >> +typedef struct mapping_entry mapping_entry_s; >> + >> +DEF_VEC_O(mapping_entry_s); >> + >> +static void >> +free_mapping_entry (VEC (mapping_entry_s) *lst) >> +{ >> + int ix; >> + mapping_entry_s *p; >> + >> + for (ix = 0; VEC_iterate (mapping_entry_s, lst, ix, p); ++ix) >> + xfree (p->hex_build_id); >> + >> + VEC_free (mapping_entry_s, lst); >> +} >> + >> +/* Used for finding a mapping containing the given >> + l_ld passed in K. */ >> + >> +static int >> +compare_mapping_entry_range (const void *const k, const void *const b) >> +{ >> + const ULONGEST key = *(CORE_ADDR*) k; >> + const mapping_entry_s *const p = b; >> + >> + if (key < p->vaddr) >> + return -1; >> + >> + if (key < p->vaddr + p->size) >> + return 0; >> + >> + return 1; >> +} >> + >> +struct find_memory_region_callback_data >> +{ >> + unsigned is_elf64; >> + >> + /* Return. Ordered list of all object mappings sorted in >> + ascending order by VADDR. Must be freed with free_mapping_entry. */ >> + VEC (mapping_entry_s) *list; >> +}; >> + >> +static linux_find_memory_region_ftype find_memory_region_callback; >> + >> +/* Read .note.gnu.build-id from PT_NOTE. */ > > It is NT_GNU_BUILD_ID note from PT_NOTE segment. > > .note.gnu.build-id is section name, PT_NOTE is segment name. Those do not > match. [AR] ok. > > >> + >> +static void >> +read_build_id (struct find_memory_region_callback_data *const p, >> + mapping_entry_s *const bil, const CORE_ADDR load_addr, >> + const CORE_ADDR l_addr) >> +{ >> + union ElfXX_Ehdr >> + { >> + Elf32_Ehdr _32; >> + Elf64_Ehdr _64; >> + } ehdr; >> + union ElfXX_Phdr >> + { >> + Elf32_Phdr _32; >> + Elf64_Phdr _64; >> + } const *phdr; >> + union ElfXX_Nhdr >> + { >> + Elf32_Nhdr _32; >> + Elf64_Nhdr _64; >> + } *nhdr; >> +#define HDR(hdr, fld) (((p)->is_elf64)? (hdr)._64.fld : (hdr)._32.fld) >> +#define SIZEOFHDR(hdr) (((p)->is_elf64)?sizeof((hdr)._64):sizeof((hdr)._32)) > > These macros are already defined above, use only one definition. It would be > appropriate to make their name slightly longer in such case, not required. [AR]. Moved unions and defines up, removed "undef". Changed naming to be slightly less prone to collisions and clearer. > > >> + if (linux_read_memory (load_addr, (unsigned char *) &ehdr, SIZEOFHDR (ehdr)) >> + == 0 >> + && HDR (ehdr, e_ident[EI_MAG0]) == ELFMAG0 >> + && HDR (ehdr, e_ident[EI_MAG1]) == ELFMAG1 >> + && HDR (ehdr, e_ident[EI_MAG2]) == ELFMAG2 >> + && HDR (ehdr, e_ident[EI_MAG3]) == ELFMAG3) >> + { >> + void *phdr_buf; >> + const ULONGEST p_type = PT_NOTE; >> + >> + gdb_assert (HDR (ehdr, e_phnum) < 100); /* Basic sanity check. */ >> + gdb_assert (HDR (ehdr, e_phentsize) == SIZEOFHDR (*phdr)); >> + phdr_buf = alloca (HDR (ehdr, e_phnum) * HDR (ehdr, e_phentsize)); >> + >> + if (linux_read_memory (load_addr + HDR (ehdr, e_phoff), phdr_buf, >> + HDR (ehdr, e_phnum) * HDR (ehdr, e_phentsize)) >> + != 0) >> + { >> + warning ("Could not read program header."); >> + return; >> + } >> + >> + phdr = phdr_buf; >> + >> + while ((phdr = find_phdr (p->is_elf64, phdr, (char *) phdr_buf >> + + HDR (ehdr, e_phnum) * HDR (ehdr, e_phentsize), >> + find_phdr_p, &p_type)) != NULL) > > phdr_buf is void * and GDB codebase allows void * arithmetics so the cast > could be removed. [AR] I really dislike using void* even if gdb uses it. It's not correct. > > find_phdr_p probably should not be passed, as discussed above. [AR] I can remove it, but the idea is to make generic phdr iterator similar to 'maps' parser. > > Assignment needs to be on a separate line according to GNU Coding Standards, > therefore: > for (;;) > { > phdr = find_phdr (p->is_elf64, phdr, > (phdr_buf > + HDR (ehdr, e_phnum) * HDR (ehdr, e_phentsize)), > find_phdr_p, &p_type); > if (phdr == NULL) > break; > > >> + { >> + void *const pt_note = xmalloc (HDR (*phdr, p_memsz)); >> + const void *const pt_end >> + = (char*) pt_note + HDR (*phdr, p_memsz); > > When it does not fit on a single line use separate declaration line: > const void *pt_end; > > pt_end = (gdb_byte *) pt_note + HDR (*phdr, p_memsz); > > And also you use 'char' for byte but GDB - even with recent Pedro's changes > - prefers to use gdb_byte in such case. It is not a printable character. > > >> + >> + if (linux_read_memory (HDR (*phdr, p_vaddr) + l_addr, >> + pt_note, HDR (*phdr, p_memsz)) != 0) >> + { >> + xfree (pt_note); >> + warning ("Could not read note."); >> + break; >> + } >> + >> + nhdr = pt_note; >> + while ((void *) nhdr < pt_end) >> + { >> + const size_t note_sz >> + = HDR (*nhdr, n_namesz) + HDR (*nhdr, n_descsz) >> + + SIZEOFHDR (*nhdr); > > Again it is more readable if split, also please keep the order as present in > the file: > const size_t note_sz; > > note_sz = (SIZEOFHDR (*nhdr) + HDR (*nhdr, n_namesz) > + HDR (*nhdr, n_descsz)); > > But there is also missing alignment to 4 bytes of both n_namesz and n_descsz: > Padding is present, if necessary, to ensure 4-byte alignment for the > descriptor. Such padding is not included in namesz. [AR] Ok, code re-arranged; sizes rounded up to appropriate size. > + > Padding is present, if necessary, to ensure 4-byte alignment for the > next note entry. Such padding is not included in descsz. > > >> + >> + if (((char *) nhdr + note_sz) > (char *) pt_end) > > gdb_byte * > > And I asked for checking NOTE_SZ == 0 but you still do not check it. If there > will be NOTE_SZ == 0 then bin2hex below will use the code path: > /* May use a length, or a nul-terminated string as input. */ > if (count == 0) > count = strlen ((char *) bin); > > which may in a worst case even crash GDB on invalid file running out through > non-zero bytes out of mapped memory. [AR] It is now being checked. > > >> + { >> + warning ("Malformed PT_NOTE\n"); >> + break; >> + } > > You need to check here also the name content, it is "GNU\x00" (n_namesz == 4). [AR], can it be any other name if type is NT_GNU_BUILD_ID? Added the check but seems like an overkill to me. > > >> + if (HDR (*nhdr, n_type) == NT_GNU_BUILD_ID) >> + { >> + bil->hex_build_id = xmalloc (note_sz * 2 + 1); >> + bin2hex ((gdb_byte*) nhdr, bil->hex_build_id, note_sz); > > I wrote last time: > GNU Coding Standard formatting: > bin2hex ((gdb_byte *) nhdr, bil->hex_build_id, note_sz); > > > But this is not the build-id. > > readelf -n > Build ID: 1dfca42f0dac568cf81fedc2a9a37a98789a03e4 > > vs. gdbserver: > > > > You need to send only the real build-id bytes, omitting the note header and the > note name ("GNU\0"). > > Then it will not match GDB itself, it also needs to be updated to process only > the build-id bytes, not the header. [AR] Ok. > > >> + xfree (pt_note); >> + return; >> + } >> + nhdr = (void*) ((char *) nhdr + note_sz); > > I wrote last time: > GNU Coding Standard formatting + simplification: > nhdr = (void *) nhdr + note_sz; [AR] I re-arranged the code. Void * arithmetic is not right, so I'm avoiding it. > > >> + } >> + xfree (pt_note); >> + } >> + } >> + else >> + warning ("Reading build-id failed."); > > I would omit these warnings. But otherwise please put there some better > identifiers, such as vaddr and/or filename. Seeing just many such errors is > not too useful as I have found during the debugging today. [AR] Removed the warning. > > >> +#undef HDR >> +#undef SIZEOFHDR >> +} >> + >> + >> +/* Add mapping_entry. */ >> + >> +static int >> +find_memory_region_callback (ULONGEST vaddr, ULONGEST size, ULONGEST offset, >> + ULONGEST inode, int read, int write, int exec, >> + int modified, const char *filename, void *data) >> +{ >> + if (inode != 0) >> + { >> + struct find_memory_region_callback_data *const p = data; >> + mapping_entry_s bil; >> + >> + bil.vaddr = vaddr; >> + bil.size = size; >> + bil.offset = offset; >> + bil.inode = inode; >> + bil.hex_build_id = NULL; >> + >> + VEC_safe_push (mapping_entry_s, p->list, &bil); >> + } >> + >> + /* Continue the traversal. */ >> + return 0; >> +} >> + >> +/* Linear reverse find starting from RBEGIN towards REND looking for >> + the lowest vaddr mapping of the same inode and zero offset. */ >> + >> +static mapping_entry_s * >> +lrfind_mapping_entry (mapping_entry_s *const rbegin, >> + const mapping_entry_s *const rend) >> +{ >> + mapping_entry_s *p; >> + >> + for (p = rbegin - 1; p >= rend && p->inode == rbegin->inode; --p) >> + if (p->offset == 0) >> + return p; > > I had here this layout: > 7ffff7ddc000-7ffff7dfd000 r-xp 00000000 fd:01 51415762 /usr/lib64/ld-2.17.so > 7ffff7ff9000-7ffff7ffa000 rw-p 00000000 00:00 0 > 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0 [vdso] > 7ffff7ffc000-7ffff7ffe000 rw-p 00020000 fd:01 51415762 /usr/lib64/ld-2.17.so > > so it should rather be: > for (p = rbegin - 1; p >= rend; --p) > if (p->offset == 0 && p->inode == rbegin->inode) > return p; > > Fortunately it should not have any real performance impact. [AR] Ok, though we are not adding mappings with inode == 0. See 'find_memory_region_callback'. I changed it anyway, seems a bit more robust at the expense of rather unlikely event where mapping with offset 0 does not exist. > > >> + >> + return NULL; >> +} >> + >> +/* Get build-id for the given L_LD. DATA must point to >> + already filled list of mapping_entry elements. >> + >> + Return build_id as stored in the list element corresponding >> + to L_LD. >> + >> + NULL may be returned if build-id could not be fetched. >> + >> + Returned string must not be freed explicitly. */ >> + >> +static const char * >> +get_hex_build_id (const CORE_ADDR l_addr, const CORE_ADDR l_ld, >> + struct find_memory_region_callback_data *const data) >> +{ >> + mapping_entry_s *bil; >> + >> + if (VEC_address (mapping_entry_s, data->list) == NULL) >> + return NULL; >> + >> + bil = bsearch (&l_ld, VEC_address (mapping_entry_s, data->list), >> + VEC_length (mapping_entry_s, data->list), >> + sizeof (mapping_entry_s), compare_mapping_entry_range); >> + >> + if (bil == NULL) >> + return NULL; >> + >> + if (bil->hex_build_id == NULL) >> + { >> + CORE_ADDR load_addr; > > This variable declaration should be moved to the more inner block below. > > >> + mapping_entry_s *const bil_min >> + = lrfind_mapping_entry (bil, >> + VEC_address (mapping_entry_s, data->list)); > > When it does not fit the line make the declaration separate, such as: > > mapping_entry_s *const bil_min; > > bil_min = lrfind_mapping_entry (bil, VEC_address (mapping_entry_s, > data->list)); [AR] Not sure what is the advantage, but ok. > > There should be an empty line between declarations and code statements. > >> + if (bil_min != NULL) >> + { >> + load_addr = bil_min->vaddr; >> + read_build_id (data, bil, load_addr, l_addr); >> + } >> + else >> + { >> + /* Do not try to find hex_build_id again. */ >> + bil->hex_build_id = xstrdup (""); >> + warning ("Could not determine load address; " >> + "build_id can not be used."); > build-id > > The name of the feature is "build-id" so it always should be presented this > way to the user. Variable names are not interesting to users. [AR] Ok. Also, please note the change: I assign BUILD_ID_INVALID here rather than "" so that we can determine this at document creation time. I'd rather not emit build-id attribute at all than emitting empty build-id when it could not be fetched. > > >> + } >> + } >> + >> + return bil->hex_build_id; >> +} >> + >> /* Construct qXfer:libraries-svr4:read reply. */ >> >> static int >> @@ -5653,6 +5975,7 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf, >> struct process_info_private *const priv = current_process ()->private; >> char filename[PATH_MAX]; >> int pid, is_elf64; >> + struct find_memory_region_callback_data data; >> >> static const struct link_map_offsets lmo_32bit_offsets = >> { >> @@ -5688,6 +6011,14 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf, >> is_elf64 = elf_64_file_p (filename, &machine); >> lmo = is_elf64 ? &lmo_64bit_offsets : &lmo_32bit_offsets; >> >> + data.is_elf64 = is_elf64; >> + data.list = NULL; >> + VEC_reserve (mapping_entry_s, data.list, 16); >> + if (linux_find_memory_regions_full ( > > There should not be opening paren at the end. [AR] Ok. > >> + lwpid_of (get_thread_lwp (current_inferior)), > > When the parameters are too long for proper indentation use a temporary > variable for the value. [AR] Ok, used 'pid' which had been there already. > > >> + find_memory_region_callback, &data, NULL) < 0) >> + warning ("Finding memory regions failed"); >> + >> if (priv->r_debug == 0) >> priv->r_debug = get_r_debug (pid, is_elf64); >> >> @@ -5762,6 +6093,7 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf, >> /* 6x the size for xml_escape_text below. */ >> size_t len = 6 * strlen ((char *) libname); >> char *name; >> + const char *hex_enc_build_id = NULL; >> >> if (!header_done) >> { >> @@ -5770,21 +6102,28 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf, >> header_done = 1; >> } >> >> - while (allocated < p - document + len + 200) >> + name = xml_escape_text ((char *) libname); >> + hex_enc_build_id = get_hex_build_id (l_addr, l_ld, &data); >> + >> + while (allocated < (p - document + len + 200 >> + + (hex_enc_build_id != NULL >> + ? strlen (hex_enc_build_id) : 0))) >> { >> /* Expand to guarantee sufficient storage. */ >> - uintptr_t document_len = p - document; >> + const ptrdiff_t document_len = p - document; >> >> - document = xrealloc (document, 2 * allocated); >> allocated *= 2; >> + document = xrealloc (document, allocated); >> p = document + document_len; >> } >> >> - name = xml_escape_text ((char *) libname); >> p += sprintf (p, "> - "l_addr=\"0x%lx\" l_ld=\"0x%lx\"/>", >> + "l_addr=\"0x%lx\" l_ld=\"0x%lx\"", >> name, (unsigned long) lm_addr, >> (unsigned long) l_addr, (unsigned long) l_ld); >> + if (hex_enc_build_id != NULL) >> + p += sprintf (p, " build-id=\"%s\"", hex_enc_build_id); >> + p += sprintf(p, "/>"); >> free (name); >> } >> else if (lm_prev == 0) >> @@ -5819,6 +6158,7 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf, >> >> memcpy (readbuf, document + offset, len); >> xfree (document); >> + free_mapping_entry (data.list); >> >> return len; >> } >> -- >> 1.7.10.4 >> > > > > Thanks, > Jan > Thanks, Aleksandar