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

["H.J. Lu via Libc-alpha" <libc-alpha@sourceware.org>]

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.

> > +      return 0;
> > +    }
> > +  return 1;
> >  }
> >
> >  #endif /* _DL_PROP_H */
> > diff --git a/sysdeps/generic/link_map.h b/sysdeps/generic/link_map.h
> > index a056184690..9f482b8c20 100644
> > --- a/sysdeps/generic/link_map.h
> > +++ b/sysdeps/generic/link_map.h
> > @@ -1 +1,2 @@
> > -/* No architecture specific definitions.  */
> > +/* GNU_PROPERTY_1_NEEDED of this object.  */
> > +unsigned int l_1_needed;
> > diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h
> > index 56bd020b3c..385548fad3 100644
> > --- a/sysdeps/x86/dl-prop.h
> > +++ b/sysdeps/x86/dl-prop.h
> > @@ -97,6 +97,7 @@ _dl_process_property_note (struct link_map *l, const ElfW(Nhdr) *note,
> >
> >    const ElfW(Addr) start = (ElfW(Addr)) note;
> >
> > +  unsigned int needed_1 = 0;
> >    unsigned int feature_1_and = 0;
> >    unsigned int isa_1_needed = 0;
> >    unsigned int last_type = 0;
> > @@ -141,7 +142,8 @@ _dl_process_property_note (struct link_map *l, const ElfW(Nhdr) *note,
> >             last_type = type;
> >
> >             if (type == GNU_PROPERTY_X86_FEATURE_1_AND
> > -               || type == GNU_PROPERTY_X86_ISA_1_NEEDED)
> > +               || type == GNU_PROPERTY_X86_ISA_1_NEEDED
> > +               || type == GNU_PROPERTY_1_NEEDED)
> >               {
> >                 /* The sizes of types which we are searching for are
> >                    4 bytes.  There is no point to continue if this
>
> Ok.
>
> > @@ -151,12 +153,18 @@ _dl_process_property_note (struct link_map *l, const ElfW(Nhdr) *note,
> >
> >                 /* NB: Stop the scan only after seeing all types which
> >                    we are searching for.  */
> > -               _Static_assert ((GNU_PROPERTY_X86_ISA_1_NEEDED >
> > -                                GNU_PROPERTY_X86_FEATURE_1_AND),
> > +               _Static_assert (((GNU_PROPERTY_X86_ISA_1_NEEDED
> > +                                 > GNU_PROPERTY_X86_FEATURE_1_AND)
> > +                                && (GNU_PROPERTY_X86_FEATURE_1_AND
> > +                                    > GNU_PROPERTY_1_NEEDED)),
> >                                 "GNU_PROPERTY_X86_ISA_1_NEEDED > "
> > -                               "GNU_PROPERTY_X86_FEATURE_1_AND");
> > +                               "GNU_PROPERTY_X86_FEATURE_1_AND && "
> > +                               "GNU_PROPERTY_X86_FEATURE_1_AND > "
> > +                               "GNU_PROPERTY_1_NEEDED");
> >                 if (type == GNU_PROPERTY_X86_FEATURE_1_AND)
> >                   feature_1_and = *(unsigned int *) ptr;
> > +               else if (type == GNU_PROPERTY_1_NEEDED)
> > +                 needed_1 = *(unsigned int *) ptr;
> >                 else
> >                   {
> >                     isa_1_needed = *(unsigned int *) ptr;
>
> Ok.
>
> > @@ -187,9 +195,10 @@ _dl_process_property_note (struct link_map *l, const ElfW(Nhdr) *note,
> >      }
> >
> >    /* We get here only if there is one or no GNU property note.  */
> > -  if (isa_1_needed != 0 || feature_1_and != 0)
> > +  if (needed_1 != 0 || isa_1_needed != 0 || feature_1_and != 0)
> >      {
> >        l->l_property = lc_property_valid;
> > +      l->l_1_needed = needed_1;
> >        l->l_x86_isa_1_needed = isa_1_needed;
> >        l->l_x86_feature_1_and = feature_1_and;
> >      }
>
> Ok.
>
> > diff --git a/sysdeps/x86/link_map.h b/sysdeps/x86/link_map.h
> > index 4c46a25f83..0c7e25dc96 100644
> > --- a/sysdeps/x86/link_map.h
> > +++ b/sysdeps/x86/link_map.h
> > @@ -29,3 +29,5 @@ unsigned int l_x86_feature_1_and;
> >
> >  /* GNU_PROPERTY_X86_ISA_1_NEEDED of this object.  */
> >  unsigned int l_x86_isa_1_needed;
> > +
> > +#include <sysdeps/generic/link_map.h>
> >
>
> Ok.



-- 
H.J.