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

Jan Kratochvil
2013-04-17T20:33:11Z

On Tue, 16 Apr 2013 17:44:54 +0200, Aleksandar Ristovski wrote:
[...]
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
[...]
> +#define ELFXX_FLD(elf64, hdr, fld) ((elf64) ? (hdr)._64.fld : (hdr)._32.fld)
> +#define ELFXX_SIZEOF(elf64, hdr) ((elf64) ? sizeof ((hdr)._64) \
> + : sizeof ((hdr)._32))
> +/* Round up to next 4 byte boundary. */
> +#define ELFXX_ROUNDUP_4(elf64, what) ((elf64) ? (((what) + 3) \
> + & ~((Elf64_Word)3U)) \

GNU Coding Standards:
& ~((Elf64_Word) 3U)) \

> + : (((what) + 3) \
> + & ~((Elf32_Word) 3U)))

This is still overcomplicated, why the ELF64 parameter at all? There should
not be any.

And all the casts there with Elf64_Word and Elf32_Word:
* both Elf64_Word and Elf32_Word are the same, they are 32-bit.
* despite all the casts the macro is not safe for 64-bit WHAT as the mask
will be just 0xffffffff.

Put there just:
#define ELFXX_ROUNDUP_4(what) (((what) + 3) & ~(ULONGEST) 3)

> +#define BUILD_ID_INVALID "?"
> +
> /* ``all_threads'' is keyed by the LWP ID, which we use as the GDB protocol
> representation of the thread ID.
>
[...]
> +/* 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;

More commented in another mail.

I already wrote when k is const * there should be (const CORE_ADDR *).

It is here even unrelated to the const values unusual in GDB. 'k' is pointing
to const data so it is incorrect to cast it to non-const-target pointer.
In such case 'k' should be void * and not const void *.

> + const mapping_entry_s *const p = b;
> +
> + if (key < p->vaddr)
> + return -1;
> +
> + if (key < p->vaddr + p->size)
> + return 0;
> +
> + return 1;
> +}
[...]
> +/* Read build-id from PT_NOTE.
> + Argument LOAD_ADDR pepresents run time virtual address corresponding to
> + the beginning of the first loadable segment. L_ADDR is displacement
> + as supplied by the dynamic linker. */
> +
> +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)
> +{
> + const int is_elf64 = p->is_elf64;
> + ElfXX_Ehdr ehdr;
> +
> + if (linux_read_memory (load_addr, (unsigned char *) &ehdr,
> + ELFXX_SIZEOF (is_elf64, ehdr)) == 0
> + && ELFXX_FLD (is_elf64, ehdr, e_ident[EI_MAG0]) == ELFMAG0
> + && ELFXX_FLD (is_elf64, ehdr, e_ident[EI_MAG1]) == ELFMAG1
> + && ELFXX_FLD (is_elf64, ehdr, e_ident[EI_MAG2]) == ELFMAG2
> + && ELFXX_FLD (is_elf64, ehdr, e_ident[EI_MAG3]) == ELFMAG3)
> + {
> + const ElfXX_Phdr *phdr;
> + void *phdr_buf;
> + const unsigned e_phentsize = ELFXX_FLD (is_elf64, ehdr, e_phentsize);
> +
> + if (ELFXX_FLD (is_elf64, ehdr, e_phnum) >= 100
> + || e_phentsize != ELFXX_SIZEOF (is_elf64, *phdr))
> + {
> + /* Basic sanity check failed. */
> + warning ("Could not read program header.");

Print LOAD_ADDR, otherwise one has to start debugging gdbserver to find out
why the message was printed.

The message could be different than the message for failed linux_read_memory
below.

> + return;
> + }
> +
> + phdr_buf = alloca (ELFXX_FLD (is_elf64, ehdr, e_phnum) * e_phentsize);
> +
> + if (linux_read_memory (load_addr + ELFXX_FLD (is_elf64, ehdr, e_phoff),
> + phdr_buf,
> + ELFXX_FLD (is_elf64, ehdr, e_phnum) * e_phentsize)
> + != 0)
> + {
> + warning ("Could not read program header.");

Print LOAD_ADDR, otherwise one has to start debugging gdbserver to find out
why the message was printed.

> + return;
> + }
> +
> + phdr = phdr_buf;
> +
> + for (;;)
> + {
> + gdb_byte *pt_note;
> + const gdb_byte *pt_end;
> + const ElfXX_Nhdr *nhdr;
> + CORE_ADDR note_addr;
> +
> + phdr = find_phdr (p->is_elf64, phdr, (gdb_byte *) phdr_buf
> + + ELFXX_FLD (is_elf64, ehdr, e_phnum) * e_phentsize,
> + PT_NOTE);
> + if (phdr == NULL)
> + break;
> + pt_note = xmalloc (ELFXX_FLD (is_elf64, *phdr, p_memsz));
> + note_addr = ELFXX_FLD (is_elf64, *phdr, p_vaddr) + l_addr;
> + if (linux_read_memory (note_addr, pt_note,
> + ELFXX_FLD (is_elf64, *phdr, p_memsz)) != 0)
> + {
> + xfree (pt_note);
> + warning ("Could not read note at address 0x%s",
> + paddress (note_addr));
> + break;
> + }
> +
> + pt_end = pt_note + ELFXX_FLD (is_elf64, *phdr, p_memsz);
> + nhdr = (void *) pt_note;
> + while ((gdb_byte *) nhdr < pt_end)

This should be (const gdb_byte *) as nhdr is const-target pointer.

> + {
> + const size_t namesz
> + = ELFXX_ROUNDUP_4 (is_elf64, ELFXX_FLD (is_elf64, *nhdr,
> + n_namesz));
> + const size_t descsz
> + = ELFXX_ROUNDUP_4 (is_elf64, ELFXX_FLD (is_elf64, *nhdr,
> + n_descsz));
> + const size_t note_sz = ELFXX_SIZEOF (is_elf64, *nhdr) + namesz
> + + descsz;

GNU Coding Standards:
const size_t note_sz = (ELFXX_SIZEOF (is_elf64, *nhdr) + namesz
+ descsz);

> +
> + if (((gdb_byte *) nhdr + note_sz) > pt_end || note_sz == 0

This should be (const gdb_byte *) as nhdr is const-target pointer.

> + || descsz == 0)
> + {
> + warning ("Malformed PT_NOTE at address 0x%s\n",
> + paddress ((CORE_ADDR) nhdr));

NHDR is in gdbserver's memory, that has no useful value for the user.
You should print:

paddress (note_addr + nhdr - pt_note));

> + break;
> + }
[...]
> +/* Get build-id for the given L_LD, where L_LD corresponds to
> + link_map.l_ld as specified by the dynamic linker.
> + DATA must point to already filled list of mapping_entry elements.
> +
> + If build-id had not been read, read it and cache in corresponding
> + list element.
> +
> + 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;
> +
> + 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)
> + {
> + mapping_entry_s *bil_min;
> +
> + bil_min = lrfind_mapping_entry (bil, VEC_address (mapping_entry_s,
> + data->list));
> + if (bil_min != NULL)
> + read_build_id (data, bil, bil_min->vaddr, l_addr);
> + else
> + {
> + /* Do not try to find hex_build_id again. */
> + bil->hex_build_id = xstrdup (BUILD_ID_INVALID);
> + warning ("Could not determine load address; mapping entry with"
> + " offset 0 corresponding to l_ld=0x%s could not be found; "
> + "build-id can not be used.", paddress (l_ld));

GNU Coding Standards s/=/ = / - IMO it applies also to the output.

Also GDB uses continuation space at the end of previous line.

There should be localization.

warning (_("Could not determine load address; mapping entry with "
"offset 0 corresponding to l_ld = 0x%s could not be "
"found; build-id can not be used."), paddress (l_ld));

> + }
> + }
> +
> + return bil->hex_build_id;
> +}
> +
> /* Construct qXfer:libraries-svr4:read reply. */
>
> static int
[...]

Thanks,
Jan