Re: [PATCH 6/8] gdbserver build-id attribute generator

Aleksandar Ristovski
2013-04-16T15:44:04Z

On 13-04-14 10:16 AM, Jan Kratochvil wrote:
> On Tue, 09 Apr 2013 17:27:43 +0200, Aleksandar Ristovski wrote:
> [...]
>> +
>> +#define ELFXX_FLD(hdr, fld) ((is_elf64) ? (hdr)._64.fld : (hdr)._32.fld)
>> +#define ELFXX_SIZEOF(hdr) ((is_elf64) ? sizeof ((hdr)._64) \
>> + : sizeof ((hdr)._32))
>
> Macros should not depend on external variables, that makes them tricky, it was
> acceptable when the variable was present in the scope where the macro was
> defined. But that is no longer true when the macro is global.
>
> is_elf64 should be a parameter of each of these macros.
>

Ok.

>
>> +#define ELFXX_ROUNDUP(what) ((is_elf64) ? (((what) + sizeof (Elf64_Word) - 1) \
>> + & ~(sizeof (Elf64_Word) - 1)) \
>> + : (((what) + sizeof (Elf32_Word) - 1) \
>> + & ~(sizeof (Elf32_Word) - 1)))
>
> This is overcomplicated. The ELF standard defines it as "4-byte alignment".
> While both sizeof (Elf64_Word) == 4 and sizeof (Elf32_Word) == 4 I find that
> incorrect, the standard talks about 4 bytes (for both elf32 and elf64), not
> about sizeof of anything.

Ok. I re-read it and indeed it says 4 bytes, and not how I
mis-remembered, word boundary.

>
>
...
>>
>> +/* Linearly traverse pheaders given in PHDR until supplied
>> + predicate function returns 1. If supplied predicate function
>> + did return 1, stop traversal and return that PHDR. */
>
> There is no predicate function anymore.

Ok.

>
>
>> +
>>
...
>
>> + if (phdr == NULL)
>> + return NULL;
>
> I do not see a reason for this check, callers never pass it NULL. It should
> be rather gdb_assert in such case if anything.

Ok. removed.

>
>
...
>> + {
>> + if (is_elf64)
>> + {
>> + const Elf64_Phdr *const p = phdr;
>
> When the 32-vs-64 ELF framework is provided here I expected this existing code
> could be simplified on top of it. Provided such patch at the end of this
> mail.

Yes, indeed. Incorporated.

>
>
...
>> +static void
>> +free_mapping_entry (VEC (mapping_entry_s) *lst)
>
> It does not free mapping_entry. It frees the vector of them. Therefore it
> should be name for example free_mapping_entry_vec. And the function is
> missing comment.

I consider this nitpicking - argument type does convey this very
clearly. But added _vec as per your suggestion.

>
>
...
>> + const ULONGEST key = *(CORE_ADDR*) k;
>
> GNU Coding Standards:
> const ULONGEST key = *(CORE_ADDR *) k;
> And when it is all const it should not be temporarily de-const-ed:
> const ULONGEST key = *(const CORE_ADDR *) k;
>

Ok.
...
>> + VEC (mapping_entry_s) *list;
>> +};
>> +
>> +static linux_find_memory_region_ftype find_memory_region_callback;
>
> Why is it here? It should be before the find_memory_region_callback
> definition.
>

Ok.

>
>> +
>> +/* Read build-id from PT_NOTE. */
>
> Describe function parameters. It is essential to see the LOAD_ADDR vs. L_ADDR
> difference.

Ok.

>
>
...
>> + gdb_assert (ELFXX_FLD (ehdr, e_phnum) < 100); /* Basic sanity check. */
>
> I am aware a similar gdb_assert is in get_dynamic but that is a bug.
> gdbserver should not crash on weird inferior data.
> There should be a warning + return.
>
> Also comments should be on their own line:
> /* Basic sanity check. */
> gdb_assert (ELFXX_FLD (ehdr, e_phnum) < 100);
>
>
>> + gdb_assert (e_phentsize == ELFXX_SIZEOF (*phdr));
>
> Likewise.

Ok, replaced with a check + warning.

>
>
...
>> + pt_note = xmalloc (ELFXX_FLD (*phdr, p_memsz));
>> + pt_end = (gdb_byte*) pt_note + ELFXX_FLD (*phdr, p_memsz);
>
> GNU Coding Standards:
> pt_end = (gdb_byte *) pt_note + ELFXX_FLD (*phdr, p_memsz);
> But pt_note is already gdb_byte * so that cast is useless, therefore:
> pt_end = pt_note + ELFXX_FLD (*phdr, p_memsz);
>

Ok.

>
>> +
>> + if (linux_read_memory (ELFXX_FLD (*phdr, p_vaddr) + l_addr, pt_note,
>> + ELFXX_FLD (*phdr, p_memsz)) != 0)
>> + {
>> + xfree (pt_note);
>> + warning ("Could not read note.");
>
> Print also the note address when there is a warning at all.

Ok.

>
>
>> + break;
>> + }
>> +
>> + nhdr = (void *) pt_note;
>> + while ((void *) nhdr < (void *) pt_end)
>
> When it is all const there should be (const void *). But in fact it is easier
> to use const gdb_byte * when pt_end is already such type:
> while ((const gdb_byte *) nhdr < pt_end)

gdb_byte * is o.k., but there is no really a need for const - when
casting is for the purpose of pointer comparison, IMO const only
clutters the reading.

>
>
>> + {
>> + const size_t namesz
>> + = ELFXX_ROUNDUP (ELFXX_FLD (*nhdr, n_namesz));
>> + const size_t descsz
>> + = ELFXX_ROUNDUP (ELFXX_FLD (*nhdr, n_descsz));
>> + const size_t note_sz = ELFXX_SIZEOF (*nhdr) + namesz + descsz;
>> +
>> + if (((gdb_byte *) nhdr + note_sz) > pt_end || note_sz == 0
>
> It should be (const gdb_byte *) because nhdr is already const *.

I like using const, as you may have noticed, unless it only clutters,
like is the case when casting is used for the purpose of pointer
comparison like here.

>
>
>> + || descsz == 0)
>> + {
>> + warning ("Malformed PT_NOTE\n");
>
> Print also the note address when there is a warning at all.

Ok.

>
>
>> + break;
>> + }
>> + if (ELFXX_FLD (*nhdr, n_type) == NT_GNU_BUILD_ID
>> + && ELFXX_FLD (*nhdr, n_namesz) == 4)
>> + {
>> + const char gnu[4] = "GNU\0";
>> + const char *const pname
>> + = (char *) nhdr + ELFXX_SIZEOF (*nhdr);
>
> It should be (const char *) because nhdr is already const *.

When assigning to const, then const in the cast for pointer arithmetic
does not matter and IMO only clutters.

>
>
>> +
>> + if (memcmp (pname, gnu, 4) == 0)
>> + {
>> + const size_t n_descsz = ELFXX_FLD (*nhdr, n_descsz);
>> +
>> + bil->hex_build_id = xmalloc (n_descsz * 2 + 1);
>> + bin2hex ((gdb_byte*) pname + namesz, bil->hex_build_id,
>
> It should be (const gdb_byte *) because pname is already const *.

Ok.

>
> Additionally according to the GNU Coding Standards spacing it should be:
> bin2hex ((const gdb_byte *) pname + namesz, bil->hex_build_id,
>
>
>> + n_descsz);
>> + xfree (pt_note);
>> + return;
>> + }
>> + }
>> + nhdr = (void*) ((gdb_byte *) nhdr + note_sz);
>> + }
>> + xfree (pt_note);
>> + }
>> + }
>> +}
>> +
>> +/* Add mapping_entry. */
>
> When the line
> static linux_find_memory_region_ftype find_memory_region_callback;
> will be here the parameters would be described. But still one could write:
> /* Add mapping_entry. See linux_find_memory_region_ftype for the
> parameters description. */

Ok.

>
>
...
>> +
>> +/* Get build-id for the given L_LD. DATA must point to
>
> Maybe one could describe more what is "L_LD".
> L_LD is the link_map.l_ld field from libc shared library list.
>
> And that L_ADDR parameter is also:
> L_ADDR is the link_map.l_addr field from libc shared library list.
>

Ok.

>
>> + 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;
>
> I do not think this check should be / needs to be here. bsearch with NMEMB ==
> 0 should return NULL.

You are probably right. Removed.

>
>
...
>> + {
>> + /* Do not try to find hex_build_id again. */
>> + bil->hex_build_id = xstrdup (BUILD_ID_INVALID);
>> + warning ("Could not determine load address; "
>> + "build-id can not be used.");
>
> You should print some identification for troubleshooting when the warning is
> there at all, probably L_LD here is the best one.

Ok.

>
>
...
>> + }
>> {
>> /* 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;
>> }
>
> This "code cleanup" change is unrelated to this patch. But it is IMO not
> worth checking in separately, it does not improve it anyhow IMO.

It uses correct type for pointer difference and does not repeat
arithmetic. I removed it, it will probably reduce clash with Gary's patch.

>
>
>>
>> - name = xml_escape_text ((char *) libname);
>
> Why did you move this line to several lines above? It is a needless change.

Ok.

>
>
>> p += sprintf (p, "<library name=\"%s\" lm=\"0x%lx\" "
>> - "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
>> + && strcmp (hex_enc_build_id, BUILD_ID_INVALID) != 0)
>> + p += sprintf (p, " build-id=\"%s\"", hex_enc_build_id);
>
>> + p += sprintf(p, "/>");
>
> GNU Coding Standards:
> p += sprintf (p, "/>");
>

Ok.

---
Aleksandar