[PATCH v3] aarch64: add support for hwcap3,4
enh
enh@google.com
Tue Apr 22 15:58:59 GMT 2025
On Tue, Apr 22, 2025 at 11:52 AM enh <enh@google.com> wrote:
>
> On Tue, Apr 22, 2025 at 8:43 AM Yury Khrustalev <yury.khrustalev@arm.com> wrote:
> >
> > Add basic support for hwcap3 and hwcap4 in dynamic loader and
> > ifunc resolvers.
> >
> > This patch tries to support both runtime an compile-time checks
> > for compatibility.
> >
> > At runtime, resolver functions must check the value of the _size
> > field to determine which hwcap fields are available.
> >
> > Existing resolver functions that rely on the second argument
> > passed to the resolver function will use the _hwcap and _hwcap2
> > fields. It is recommended in this patch that new resolvers use
> > the _hwcap_array field instead when they are available at compile
> > time.
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.
(the counterargument of "but that would mean you'd need a new sysroot
to be able to use AT_HWCAP5 when that's added" doesn't seem valid
because you'd also need the new size constant _and_ the HWCAP5_*
macros you're testing for too, or you have to "backport" all that to
your own code, in which case "one more #define isn't going to hurt".)
> > When compiling code for resolvers, old or new, the Glibc headers
> > available at the compilation time maybe older or newer compared
> > to the Glibc version that is available at runtime. To allow old
> > resolvers to compile, we introduce union of a legacy struct with
> > the _hwcap and _hwcap2 fields and an array _hwcap_array with the
> > length of _IFUNC_HWCAP_MAX elements (currently 4). First 2 hwcap
> > fields can therefore be accessed in two ways.
> >
> > Resolvers may use new macro defined in sys/ifunc.h:
> >
> > _IFUNC_ARG_SIZE
> >
> > for compile-time checks, for example:
> >
> > #ifndef _IFUNC_ARG_SIZE
> > // use _hwcap and _hwcap2 fields
> > #else
> > // use _hwcap_array array field
> > #endif
>
> is this actually useful? this seems likely to encourage error-prone
> code, and _IFUNC_HWCAP_MAX is already available if you want to know
> what version your header is. (though tbh _that_ seems more
> troublesome, because it encourages folks to forget that it's the
> _runtime_ size that actually matters. for bionic i think i'll just
> inline the 4 in the array declaration instead.)
>
> so maybe just having the new _IFUNC_ARG_SIZE_VER0/_IFUNC_ARG_SIZE_VER1
> -- which is also genuinely useful at run time, and makes it a bit
> clearer that callers need to think about _both_ cases, not either/or
> -- is what someone wanting to be "clever" at compile time should use?
>
> > This patch also adds static asserts to ensure that when Glibc is
> > compiled, legacy fields _hwcap{,2} are aligned precisely with the
> > first two elements of _hwcap_array.
> > ---
> > Corresponding ABI spec update: https://github.com/ARM-software/abi-aa/pull/320
> >
> > Regression tested on AArch64 and no regressions have been found.
> >
> > OK for trunk?
> >
> > base commit: 39183b953c
> >
> > Changes in v3:
> > - Instead of adding new fields to __ifunc_arg_t introduce a union
> > to allow further seemless extension and at the same time letting
> > existing code compile without change.
> > Link to v2:
> > https://inbox.sourceware.org/libc-alpha/20250404141511.1767584-1-yury.khrustalev@arm.com/
> >
> > Changes in v2:
> > - Expanded comment in <sys/ifunc.h> to explain how to use the _size
> > field to check the version of the contents of the __ifunc_arg_t
> > struct.
> > Link to v1:
> > https://inbox.sourceware.org/libc-alpha/20250401141851.129625-1-yury.khrustalev@arm.com/
> > ---
> > sysdeps/aarch64/dl-irel.h | 32 ++++++++++++++++++++++----
> > sysdeps/aarch64/sys/ifunc.h | 38 +++++++++++++++++++++++++++----
> > sysdeps/aarch64/tst-ifunc-arg-1.c | 4 ++++
> > sysdeps/aarch64/tst-ifunc-arg-2.c | 4 ++++
> > 4 files changed, 69 insertions(+), 9 deletions(-)
> >
> > diff --git a/sysdeps/aarch64/dl-irel.h b/sysdeps/aarch64/dl-irel.h
> > index ae402bc367..76719edae4 100644
> > --- a/sysdeps/aarch64/dl-irel.h
> > +++ b/sysdeps/aarch64/dl-irel.h
> > @@ -21,11 +21,33 @@
> > #define _DL_IREL_H
> >
> > #include <stdio.h>
> > -#include <unistd.h>
> > #include <ldsodefs.h>
> > -#include <sysdep.h>
> > #include <sys/ifunc.h>
> >
> > +#define sizeof_field(TYPE, MEMBER) sizeof ((((TYPE *)0)->MEMBER))
> > +#define offsetofend(TYPE, MEMBER) \
> > + (offsetof (TYPE, MEMBER) + sizeof_field (TYPE, MEMBER))
> > +
> > +_Static_assert (sizeof (__ifunc_arg_t) == _IFUNC_ARG_SIZE,
> > + "sizeof (__ifunc_arg_t) != _IFUNC_ARG_SIZE");
> > +
> > +_Static_assert (offsetof (__ifunc_arg_t, _hwcap)
> > + == offsetof (__ifunc_arg_t, _hwcap_array[0]),
> > + "__ifunc_arg_t incorrect offset of _hwcap");
> > +_Static_assert (offsetofend (__ifunc_arg_t, _hwcap)
> > + == offsetofend (__ifunc_arg_t, _hwcap_array[0]),
> > + "__ifunc_arg_t incorrect offset of _hwcap end");
> > +
> > +_Static_assert (offsetof (__ifunc_arg_t, _hwcap2)
> > + == offsetof (__ifunc_arg_t, _hwcap_array[1]),
> > + "__ifunc_arg_t incorrect offset of _hwcap2");
> > +_Static_assert (offsetofend (__ifunc_arg_t, _hwcap2)
> > + == offsetofend (__ifunc_arg_t, _hwcap_array[1]),
> > + "__ifunc_arg_t incorrect offset of _hwcap2 end");
> > +
> > +#undef offsetofend
> > +#undef sizeof_field
> > +
> > #define ELF_MACHINE_IRELA 1
> >
> > static inline ElfW(Addr)
> > @@ -35,8 +57,10 @@ elf_ifunc_invoke (ElfW(Addr) addr)
> > __ifunc_arg_t arg;
> >
> > arg._size = sizeof (arg);
> > - arg._hwcap = GLRO(dl_hwcap);
> > - arg._hwcap2 = GLRO(dl_hwcap2);
> > + arg._hwcap_array[0] = GLRO(dl_hwcap);
> > + arg._hwcap_array[1] = GLRO(dl_hwcap2);
> > + arg._hwcap_array[2] = GLRO(dl_hwcap3);
> > + arg._hwcap_array[3] = GLRO(dl_hwcap4);
> > return ((ElfW(Addr) (*) (uint64_t, const __ifunc_arg_t *)) (addr))
> > (GLRO(dl_hwcap) | _IFUNC_ARG_HWCAP, &arg);
> > }
> > diff --git a/sysdeps/aarch64/sys/ifunc.h b/sysdeps/aarch64/sys/ifunc.h
> > index 7781b37a29..88adc0ddb2 100644
> > --- a/sysdeps/aarch64/sys/ifunc.h
> > +++ b/sysdeps/aarch64/sys/ifunc.h
> > @@ -21,22 +21,50 @@
> >
> > /* A second argument is passed to the ifunc resolver. */
> > #define _IFUNC_ARG_HWCAP (1ULL << 62)
> > +#define _IFUNC_HWCAP_MAX 4
> >
> > /* The prototype of a gnu indirect function resolver on AArch64 is
> >
> > ElfW(Addr) ifunc_resolver (uint64_t, const __ifunc_arg_t *);
> >
> > - the first argument should have the _IFUNC_ARG_HWCAP bit set and
> > - the remaining bits should match the AT_HWCAP settings. */
> > + The first argument might have the _IFUNC_ARG_HWCAP bit set and
> > + the remaining bits should match the AT_HWCAP settings.
> > +
> > + If the _IFUNC_ARG_HWCAP bit is set in the first argument, then
> > + the second argument is passed to the resolver function. In this
> > + case, this second argument is a pointer to this struct and the
> > + _size field is set to size of the struct and the remaining fields
> > + are set to the respective values of AT_HWCAP{,2,3,4,...} settings.
> > +
> > + When a resolver function needs to access second argument, it must
> > + check the value of the size field first to make sure it then uses
> > + only available hwcap fields.
> > +
> > + Legacy resolver functions may access the _hwcap and _hwcap2 fields
> > + however they are deprecated and all new implementations must use
> > + elements of the _hwcap_array field when it is available.
> > +
> > + The following macros defined below can be used for compile-time
> > + compatibility checks: _IFUNC_ARG_SIZE, _IFUNC_ARG_SIZE_VER0,
> > + _IFUNC_ARG_SIZE_VER1. If _IFUNC_ARG_SIZE is not defined, then the
> > + _hwcap_array field is not available yet. */
> >
> > -/* Second argument to an ifunc resolver. */
> > struct __ifunc_arg_t
> > {
> > unsigned long _size; /* Size of the struct, so it can grow. */
> > - unsigned long _hwcap;
> > - unsigned long _hwcap2;
> > + union {
> > + struct {
> > + unsigned long _hwcap;
> > + unsigned long _hwcap2;
> > + };
> > + unsigned long _hwcap_array[_IFUNC_HWCAP_MAX];
> > + };
> > };
> >
> > typedef struct __ifunc_arg_t __ifunc_arg_t;
> >
> > +#define _IFUNC_ARG_SIZE_VER0 24 /* sizeof 1st published struct */
> > +#define _IFUNC_ARG_SIZE_VER1 40 /* sizeof 2nd published struct */
> > +#define _IFUNC_ARG_SIZE _IFUNC_ARG_SIZE_VER1
> > +
> > #endif
> > diff --git a/sysdeps/aarch64/tst-ifunc-arg-1.c b/sysdeps/aarch64/tst-ifunc-arg-1.c
> > index b90c836000..121320b3a2 100644
> > --- a/sysdeps/aarch64/tst-ifunc-arg-1.c
> > +++ b/sysdeps/aarch64/tst-ifunc-arg-1.c
> > @@ -57,6 +57,10 @@ do_test (void)
> > TEST_COMPARE (saved_arg2._size, sizeof (__ifunc_arg_t));
> > TEST_COMPARE (saved_arg2._hwcap, getauxval (AT_HWCAP));
> > TEST_COMPARE (saved_arg2._hwcap2, getauxval (AT_HWCAP2));
> > + TEST_COMPARE (saved_arg2._hwcap_array[0], getauxval (AT_HWCAP));
> > + TEST_COMPARE (saved_arg2._hwcap_array[1], getauxval (AT_HWCAP2));
> > + TEST_COMPARE (saved_arg2._hwcap_array[2], getauxval (AT_HWCAP3));
> > + TEST_COMPARE (saved_arg2._hwcap_array[3], getauxval (AT_HWCAP4));
> > return 0;
> > }
> >
> > diff --git a/sysdeps/aarch64/tst-ifunc-arg-2.c b/sysdeps/aarch64/tst-ifunc-arg-2.c
> > index dac144d937..5028787b05 100644
> > --- a/sysdeps/aarch64/tst-ifunc-arg-2.c
> > +++ b/sysdeps/aarch64/tst-ifunc-arg-2.c
> > @@ -60,6 +60,10 @@ do_test (void)
> > TEST_COMPARE (saved_arg2._size, sizeof (__ifunc_arg_t));
> > TEST_COMPARE (saved_arg2._hwcap, getauxval (AT_HWCAP));
> > TEST_COMPARE (saved_arg2._hwcap2, getauxval (AT_HWCAP2));
> > + TEST_COMPARE (saved_arg2._hwcap_array[0], getauxval (AT_HWCAP));
> > + TEST_COMPARE (saved_arg2._hwcap_array[1], getauxval (AT_HWCAP2));
> > + TEST_COMPARE (saved_arg2._hwcap_array[2], getauxval (AT_HWCAP3));
> > + TEST_COMPARE (saved_arg2._hwcap_array[3], getauxval (AT_HWCAP4));
> > return 0;
> > }
> >
> > --
> > 2.39.5
> >
More information about the Libc-alpha
mailing list