On Fri, Mar 24, 2017 at 5:22 AM, Nick Clifton <nickc@redhat.com> wrote:
> Hi H.J.
>
>   Sorry for not responding to this earlier.
>
>> Any comments?
>
> Are the contents of the .note.gnu.property section documented somewhere ?
> (Other than an email that is).

Put my draft at

https://github.com/hjl-tools/linux-abi/wiki/property-draft.pdf

But it doesn't cover GNU_PROPERTY_X86_ISA_1_USED nor
GNU_PROPERTY_X86_ISA_1_NEEDED.

> I think that you ought to add a ld/NEWS entry for this feature.

Done.

> Did you check this patch with any non-x86 targets ?  (Just to make sure...)

I tried cross arm-linux on x86-64.  There is no regression.

>
>> +typedef struct elf_property
>> +{
>> +  unsigned int type;
>> +  unsigned int datasz;
>> +  union
>> +    {
>> +      bfd_vma value;
>> +    } u;
>> +  enum elf_property_kind kind;
>> +} elf_property;
>
> I found it very confusing that you have the same 'type' and 'datasz' fields
> as an Elf_Internal_Note structure, but with different sizes and different
> meanings.  Would it be possible to use different field names in this structure ?

Done.

> Also why do you have a union with only one element ?  I am guessing
> that it is for future extensions, but it would be nice to know.
>

Done.

>> +/* Get a property, allocate a new one if needed.  */
>> +
>> +elf_property *
>> +_bfd_elf_get_property (bfd *abfd, unsigned int type, unsigned int datasz)
>> +{
>> +  elf_property_list *p, **lastp;
>> +
>> +  /* Keep the property list in order of type.  */
>> +  lastp = &elf_properties (abfd);
>
> Paranoia - I would suggest checking that ABFD is an elf format bfd before
> attempting to access the elf_properties list.  (I know that you have already
> checked the bfd type in _bfd_elf_link_setup_gnu_properties, but this function
> can be called from elsewhere and the caller might not have been so vigilant).

I added a check:

  if (bfd_get_flavour (abfd) != bfd_target_elf_flavour)
    {
      /* Never should happen.  */
      abort ();
    }

>
>> +  for (p = *lastp; p; p = p->next)
>> +    {
>> +      /* Reuse the existing entry.  */
>> +      if (type == p->property.type)
>> +        return &p->property;
>
> Should you check that datasz == p->property.datasz as well ?

I added

          if (datasz > p->property.pr_datasz)
            {
              /* This can happen when mixing 32-bit and 64-bit objects.  */
              p->property.pr_datasz = datasz;
            }

>
>> +  p = (elf_property_list *) bfd_alloc (abfd, sizeof (*p));
>
> Paranoia again - bfd_alloc() can return NULL...

I added

  if (p == NULL)
    {
      _bfd_error_handler (_("%B: out of memory in _bfd_elf_get_property"),
                          abfd);
      _exit (EXIT_FAILURE);
    }

since we can't recover from this.

>
>> +       if (type < GNU_PROPERTY_LOUSER && bed->parse_gnu_properties)
>> +         {
>> +           enum elf_property_kind kind
>> +             = bed->parse_gnu_properties (abfd, type, ptr, datasz);
>> +           if (kind == property_corrupt)
>> +             {
>> +               /* Clear all properties.  */
>> +               elf_properties (abfd) = NULL;
>> +               return FALSE;
>
> Presumably no error message is printed here because it is assumed that this
> has been done in bed->parse_gnu_properties.  The (very) brief comment in elf-bfd.h
> however makes no mention of this requirement.

I added

  /* Parse GNU properties.  Return the property kind.  If the property
     is corrupt, issue an error message and return property_corrupt.  */
  enum elf_property_kind (*parse_gnu_properties) (bfd *, unsigned int,
                                                  bfd_byte *,
                                                  unsigned int);

>
>> +static bfd_boolean
>> +elf_merge_gnu_properties (bfd *abfd, elf_property *prop, elf_property *p)
>
> Slightly confusing parameter names.  I would have gone with prop_1 and prop_2 or
> propA and propB.  Something like that.

Done.

>
>> +         /* Discard .note.gnu.property section in the rest inputs.  */
>> +         sec = bfd_get_section_by_name (abfd, ".note.gnu.property");
>
> Why not use NOTE_GNU_PROPERTY_SECTION_NAME here ?  It is already defined in include/elf/common.h

Done.

>
> That's all that I can see.
>

Here is the updated patch.

Thanks.


-- 
H.J.