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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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