Re: [PATCH 1/2] Initial support for GNU_PROPERTY_1_NEEDED

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 07/10/2021 12:11, H.J. Lu wrote:
> On Thu, Oct 7, 2021 at 7:53 AM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 03/08/2021 18:59, H.J. Lu via Libc-alpha wrote:
>>> 1. Add GNU_PROPERTY_1_NEEDED:
>>>
>>>  #define GNU_PROPERTY_1_NEEDED      GNU_PROPERTY_UINT32_OR_LO
>>>
>>> to indicate the needed properties by the object file.
>>> 2. Add GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS:
>>>
>>>  #define GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (1U << 0)
>>>
>>> to indicate that the object file requires canonical function pointers and
>>> cannot be used with copy relocation.
>>> 3. Scan GNU_PROPERTY_1_NEEDED property and store it in l_1_needed.
>>
>> It was added by binutils with commit 6320fd00dc374f74658, maybe add on
>> the commit the version expected to have this support.
>>
>> LGTM, with a small suggestion below.
>>
>> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>
>>> ---
>>>  elf/elf.h                  | 17 +++++++++++++++++
>>>  sysdeps/generic/dl-prop.h  |  9 ++++++++-
>>>  sysdeps/generic/link_map.h |  3 ++-
>>>  sysdeps/x86/dl-prop.h      | 19 ++++++++++++++-----
>>>  sysdeps/x86/link_map.h     |  2 ++
>>>  5 files changed, 43 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/elf/elf.h b/elf/elf.h
>>> index 4738dfa28f..50f87baceb 100644
>>> --- a/elf/elf.h
>>> +++ b/elf/elf.h
>>> @@ -1312,6 +1312,23 @@ typedef struct
>>>  /* No copy relocation on protected data symbol.  */
>>>  #define GNU_PROPERTY_NO_COPY_ON_PROTECTED    2
>>>
>>> +/* A 4-byte unsigned integer property: A bit is set if it is set in all
>>> +   relocatable inputs.  */
>>> +#define GNU_PROPERTY_UINT32_AND_LO   0xb0000000
>>> +#define GNU_PROPERTY_UINT32_AND_HI   0xb0007fff
>>> +
>>> +/* A 4-byte unsigned integer property: A bit is set if it is set in any
>>> +   relocatable inputs.  */
>>> +#define GNU_PROPERTY_UINT32_OR_LO    0xb0008000
>>> +#define GNU_PROPERTY_UINT32_OR_HI    0xb000ffff
>>> +
>>> +/* The needed properties by the object file.  */
>>> +#define GNU_PROPERTY_1_NEEDED                GNU_PROPERTY_UINT32_OR_LO
>>> +
>>> +/* Set if the object file requires canonical function pointers and
>>> +   cannot be used with copy relocation.  */
>>> +#define GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (1U << 0)
>>> +
>>>  /* Processor-specific semantics, lo */
>>>  #define GNU_PROPERTY_LOPROC                  0xc0000000
>>>  /* Processor-specific semantics, hi */
>>
>> Ok.
>>
>>> diff --git a/sysdeps/generic/dl-prop.h b/sysdeps/generic/dl-prop.h
>>> index eaee8052b6..207aadb35d 100644
>>> --- a/sysdeps/generic/dl-prop.h
>>> +++ b/sysdeps/generic/dl-prop.h
>>> @@ -47,7 +47,14 @@ static inline int __attribute__ ((always_inline))
>>>  _dl_process_gnu_property (struct link_map *l, int fd, uint32_t type,
>>>                         uint32_t datasz, void *data)
>>>  {
>>> -  return 0;
>>> +  /* Continue until GNU_PROPERTY_1_NEEDED is found.  */
>>> +  if (type == GNU_PROPERTY_1_NEEDED)
>>> +    {
>>> +      if (datasz == 4)
>>> +     l->l_1_needed = *(unsigned int *) data;
>>
>> Although the pointer seems to be aligned, I think it would be better to
>> use memcpy here:
>>
>>    unsigned int t;
>>    memcpy (&t, data, sizeof (t));
>>    l->l_1_needed = t;
>>
>> (the x86 header might use a more relax since the architecture always
>> support unaligned access)
> 
> The GNU property is guaranteed to be properly aligned:
> 
>  void
> _dl_process_pt_gnu_property (struct link_map *l, int fd, const ElfW(Phdr) *ph)
> {
>   const ElfW(Nhdr) *note = (const void *) (ph->p_vaddr + l->l_addr);
>   const ElfW(Addr) size = ph->p_memsz;
>   const ElfW(Addr) align = ph->p_align;
> 
>   /* The NT_GNU_PROPERTY_TYPE_0 note must be aligned to 4 bytes in
>      32-bit objects and to 8 bytes in 64-bit objects.  Skip notes
>      with incorrect alignment.  */
>   if (align != (__ELF_NATIVE_CLASS / 8))
>     return;
> 
> data passed to _dl_process_gnu_property should be properly aligned.

Alright, the memcpy seems unnecessary then.