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

[Aleksandar Ristovski <aristovski@qnx.com>]

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