[PATCH v3] aarch64: add support for hwcap3,4

[Yury Khrustalev]

Hi everyone,

On Wed, Apr 23, 2025 at 08:19:07AM -0400, enh wrote:
> On Wed, Apr 23, 2025 at 5:22 AM Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * Yury Khrustalev:
> >
> > > Hi,
> > >
> > >> From: enh <enh@google.com>
> > >> Sent: 22 April 2025 04:58 PM
> > >> To: Yury Khrustalev
> > >> Cc: libc-alpha@sourceware.org; fweimer@redhat.com; adhemerval.zanella@linaro.org
> > >> Subject: Re: [PATCH v3] aarch64: add support for hwcap3,4
> > >>
> > >> On Tue, Apr 22, 2025 at 11:52 AM enh <enh@google.com> wrote:
> > >> ...
> > >> having updated some ifunc resolvers to see what this looks like, i
> > >> find the "off by one" in the array versus the constants quite
> > >> off-putting.
> > >>
> > >> adding
> > >> ```
> > >> #define _hwcap3 _hwcap_array[2]
> > >> ```
> > >> etc seems quite tempting for better readability at the call sites.
> > >
> > > To be honest, I would just do that:
> > >
> > >   struct __ifunc_arg_t
> > >   {
> > >     unsigned long _size;
> > >     unsigned long _hwcap_array[_IFUNC_HWCAP_MAX];
> > >   };
> > >
> > > And define these macros for the sake of existing code to compiler
> > > without errors:
> > >
> > >   #define _hwcap _hwcap_array[0]
> > >   #define _hwcap2 _hwcap_array[1]
> >
> > We can turn the argument into an const unsigned long int * pointer, with
> > the size in at index 0.  Then _IFUNC_HWCAP_MAX won't be needed, and
> > access can use something like
> >
> > __ifunc_hwcap (const unsigned long int *__arg, unsigned long int __index)
> > {
> >   return __index - 1 < __arg[0] ? __arg[__index] : 0;
> > }
> >
> > So __ifunc_hwcap (arg, 1) to access AT_HWCAP, __ifunc_hwcap (arg, 2) for
> > AT_HWCAP2, and in the future __ifunc_hwcap (arg, 3).
> 
> oh, i love that!
> 
> > Thanks,
> > Florian
> >

Original design of the __ifunc_arg_t struct implied adding new fields
rather than using a array member (that's why we have _size field rather
than _length or something like that).

Now the only problem with adding new fields _hwcap{3,4,..} as was in
the original version of this patch is that it becomes difficult to write
new ifunc resolvers: if you use, for example, _hwcap3, your code won't
compile on existing systems where Glibc headers don't have the definition
of this struct. If __ifunc_arg_t was originally designed as an array, we
wouldn't have this issue.

Replacing existing struct with the new one based on an array member allows
to extend it in the future without repeating the problem described above,
we can't avoid this problem now though.

However, now we also have a bit of naming inconsistency, as pointed by
Chris (on CC):

  AT_HWCAP --> arg->_hwcap_array[0]
  AT_HWCAP2 --> arg->_hwcap_array[1]
  AT_HWCAP3 --> arg->_hwcap_array[2]
  AT_HWCAP4 --> arg->_hwcap_array[3]

So, n-th element in the array is **not** AT_HWCAP{n}. This seems error-prone.
We could introduce an enum for more readable indices or a helper function
discussed earlier in this thread, but it wouldn't be part of the original
interface and nobody might use it, so it won't be too useful anyway.

Another issue raised by Chris in a separate thread is that if we use array
now we will never be able to add new fields to this struct. So, should we
need to extend it with something else than extra hwcap elements, we'll have
to add a new 3rd argument of ifunc resolver or do something else.

Wow, never thought adding new fields to a struct would be so exhausting :)

Consider writing a new ifunc resolver. If we want it to compile with both
older and new Glibc headers, we will have to continue using _hwcap and _hwcap2
when accessing first two elements. Or we will have to define our own struct
and own get-function. So, a resolver that uses, for example, both AT_HWCAP2
and AT_HWCAP3 will look somewhat strange:

  uint64_t hwcap2 = arg->_hwcap2;
#ifdef _IFUNC_HWCAP_MAX 
  uint64_t hwcap3 = arg->_hwcap_array[2];
#else
  uint64_t hwcap3 = 0
#endif

Though this is not entirely correct because we may want to rely on runtime
value of the _size fields and still use AT_HWCAP3 if it's available. So,
we'll be better off without even including <sys/ifunc.h> and define struct
ourselves.

So, to sum up, we aren't really solving much with this array version of the
struct and it's better to just add new fields to it. Existing code continues
to compile and any new code will have to jump through the hoops anyway.


We could still introduce that helper function though, but again, you won't
be able to use it if you want you code to compile against older Glibc headers...


Kind regards,
Yury