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

[enh]

On Wed, Apr 23, 2025 at 11:18 AM Yury Khrustalev
<yury.khrustalev@arm.com> wrote:
>
> 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.

did you read fweimer's suggestion? what was so clever about it was
that it solves this problem...

> 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
>