[v9,5/6] riscv: Add ifunc helper method to hwprobe.h

  Add a little helper method so it's easier to fetch a single value from
the hwprobe function when used within an ifunc selector.

Signed-off-by: Evan Green <evan@rivosinc.com>

---

Changes in v9:
 - Use __inline rather than inline so c89 compiles (build-many-glibcs)

Changes in v7:
 - Introduced static inline helper (Richard)

 sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h | 25 +++++++++++++++++++++
 1 file changed, 25 insertions(+)
  

On 30/11/23 15:32, Evan Green wrote:
> Add a little helper method so it's easier to fetch a single value from
> the hwprobe function when used within an ifunc selector.
> 
> Signed-off-by: Evan Green <evan@rivosinc.com>
> 
> ---
> 
> Changes in v9:
>  - Use __inline rather than inline so c89 compiles (build-many-glibcs)
> 
> Changes in v7:
>  - Introduced static inline helper (Richard)
> 
>  sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h | 25 +++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> index fd3be5a411..ee7eed3960 100644
> --- a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> +++ b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> @@ -22,6 +22,7 @@
>  
>  #include <features.h>
>  #include <stddef.h>
> +#include <errno.h>
>  #ifdef __has_include
>  # if __has_include (<asm/hwprobe.h>)
>  #  include <asm/hwprobe.h>
> @@ -79,4 +80,28 @@ typedef int (*__riscv_hwprobe_t) (struct riscv_hwprobe *__pairs, size_t __pair_c
>  
>  __END_DECLS
>  
> +/* Helper function usable from ifunc selectors that probes a single key. */
> +static __inline int
> +__riscv_hwprobe_one(__riscv_hwprobe_t hwprobe_func,
> +                    signed long long int key,
> +                    unsigned long long int *value)
> +{
> +  struct riscv_hwprobe pair;
> +  int rc;
> +
> +  if (!hwprobe_func)
> +    return ENOSYS;

The convention is no implicit checks:

  if (hwprobe_func == NULL)
    return ENOSYS;

> +
> +  pair.key = key;
> +  rc = hwprobe_func(&pair, 1, 0, NULL, 0);

Missing space after function name.

> +  if (rc)
> +    return rc;

Same as before for function that return 'int':

  if (rc != 0)
    return rc;

> +
> +  if (pair.key < 0)
> +    return ENOENT;
> +
> +  *value = pair.value;
> +  return 0;
> +}
> +
>  #endif /* sys/hwprobe.h */
  

fwiw, this is the one part of the ifunc proposal i haven't aimed for
source compatibility with in bionic --- i'm not _against_ it exactly,
i'm just unconvinced it's much use. and it's trivial to add after the
fact if it does ship in glibc and does actually turn out to be useful.

specifically, my anecdata from Android [both open-source libraries and
apps] is that basically no-one uses ifuncs beyond libc and
compiler-generated fmv ifuncs [though for riscv64 that's still "future
work"], so there's no market for helpers for hand-written ifuncs?

i think i also worry that people aren't going to pay much attention,
and will prefer this even when they have multiple keys to query
because it's a nicer api, and i'm not sure i want to encourage that
kind of thing :-)

On Thu, Nov 30, 2023 at 10:33 AM Evan Green <evan@rivosinc.com> wrote:
>
> Add a little helper method so it's easier to fetch a single value from
> the hwprobe function when used within an ifunc selector.
>
> Signed-off-by: Evan Green <evan@rivosinc.com>
>
> ---
>
> Changes in v9:
>  - Use __inline rather than inline so c89 compiles (build-many-glibcs)
>
> Changes in v7:
>  - Introduced static inline helper (Richard)
>
>  sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h | 25 +++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> index fd3be5a411..ee7eed3960 100644
> --- a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> +++ b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> @@ -22,6 +22,7 @@
>
>  #include <features.h>
>  #include <stddef.h>
> +#include <errno.h>
>  #ifdef __has_include
>  # if __has_include (<asm/hwprobe.h>)
>  #  include <asm/hwprobe.h>
> @@ -79,4 +80,28 @@ typedef int (*__riscv_hwprobe_t) (struct riscv_hwprobe *__pairs, size_t __pair_c
>
>  __END_DECLS
>
> +/* Helper function usable from ifunc selectors that probes a single key. */
> +static __inline int
> +__riscv_hwprobe_one(__riscv_hwprobe_t hwprobe_func,
> +                    signed long long int key,
> +                    unsigned long long int *value)
> +{
> +  struct riscv_hwprobe pair;
> +  int rc;
> +
> +  if (!hwprobe_func)
> +    return ENOSYS;
> +
> +  pair.key = key;
> +  rc = hwprobe_func(&pair, 1, 0, NULL, 0);
> +  if (rc)
> +    return rc;
> +
> +  if (pair.key < 0)
> +    return ENOENT;
> +
> +  *value = pair.value;
> +  return 0;
> +}
> +
>  #endif /* sys/hwprobe.h */
> --
> 2.34.1
>
  

On Wed, 06 Dec 2023 10:17:15 PST (-0800), enh@google.com wrote:
> fwiw, this is the one part of the ifunc proposal i haven't aimed for
> source compatibility with in bionic --- i'm not _against_ it exactly,
> i'm just unconvinced it's much use. and it's trivial to add after the
> fact if it does ship in glibc and does actually turn out to be useful.
>
> specifically, my anecdata from Android [both open-source libraries and
> apps] is that basically no-one uses ifuncs beyond libc and
> compiler-generated fmv ifuncs [though for riscv64 that's still "future
> work"], so there's no market for helpers for hand-written ifuncs?

After seeing how complicated the IFUNC rules are, I'm not super 
surprised that we've only got core toolchain types using them -- even 
getting this done has been a multi-month saga of trying to figure out 
how things work, so I couldn't imagine what a regular user would do.

> i think i also worry that people aren't going to pay much attention,
> and will prefer this even when they have multiple keys to query
> because it's a nicer api, and i'm not sure i want to encourage that
> kind of thing :-)

Ya, makes sense, I could see this one going either way.  I don't really 
have a strong feeling here, so I'm going to defer to Evan on the glibc 
side as he's been poking the hwprobe stuff more than I have lately.

If anyone else has a stronger opinion then now's the time to speak up, 
though ;)

> On Thu, Nov 30, 2023 at 10:33 AM Evan Green <evan@rivosinc.com> wrote:
>>
>> Add a little helper method so it's easier to fetch a single value from
>> the hwprobe function when used within an ifunc selector.
>>
>> Signed-off-by: Evan Green <evan@rivosinc.com>
>>
>> ---
>>
>> Changes in v9:
>>  - Use __inline rather than inline so c89 compiles (build-many-glibcs)
>>
>> Changes in v7:
>>  - Introduced static inline helper (Richard)
>>
>>  sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h | 25 +++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
>> index fd3be5a411..ee7eed3960 100644
>> --- a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
>> +++ b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
>> @@ -22,6 +22,7 @@
>>
>>  #include <features.h>
>>  #include <stddef.h>
>> +#include <errno.h>
>>  #ifdef __has_include
>>  # if __has_include (<asm/hwprobe.h>)
>>  #  include <asm/hwprobe.h>
>> @@ -79,4 +80,28 @@ typedef int (*__riscv_hwprobe_t) (struct riscv_hwprobe *__pairs, size_t __pair_c
>>
>>  __END_DECLS
>>
>> +/* Helper function usable from ifunc selectors that probes a single key. */
>> +static __inline int
>> +__riscv_hwprobe_one(__riscv_hwprobe_t hwprobe_func,
>> +                    signed long long int key,
>> +                    unsigned long long int *value)
>> +{
>> +  struct riscv_hwprobe pair;
>> +  int rc;
>> +
>> +  if (!hwprobe_func)
>> +    return ENOSYS;
>> +
>> +  pair.key = key;
>> +  rc = hwprobe_func(&pair, 1, 0, NULL, 0);
>> +  if (rc)
>> +    return rc;
>> +
>> +  if (pair.key < 0)
>> +    return ENOENT;
>> +
>> +  *value = pair.value;
>> +  return 0;
>> +}
>> +
>>  #endif /* sys/hwprobe.h */
>> --
>> 2.34.1
>>
  

> specifically, my anecdata from Android [both open-source libraries and
> apps] is that basically no-one uses ifuncs beyond libc and
> compiler-generated fmv ifuncs [though for riscv64 that's still "future
> work"], so there's no market for helpers for hand-written ifuncs?

It used to be more widely used on GNU/Linux, for example:

  implemented enabling sized-delete support at runtime

  Under gcc 4.5 or greater we're using ifunc function attribute to
  resolve sized delete operator to either plain delete implementation
  (default) or to sized delete (if enabled via environment variable
  TCMALLOC_ENABLE_SIZED_DELETE).
   
  <https://github.com/gperftools/gperftools/commit/6fdfc5a7f40ebcff3fdaada1a2994ff54be2f9c7>

After distributions turned on BIND_NOW, using IFUNCs for purposes like
that became much more difficult.  We are getting closer to re-enabling
getenv for use in IFUNC resolvers if the ELF dependencies express a
usable relocatoion order, but until my delayed IFUNC resolution patch is
accepted, IFUNC resolvers won't interoperate will with symbol
interposition or underlinking, so compatibility problems remain.

Outside GCC and glibc, I only see an IFUNC resolver in zlib (which is
known to be problematic because it's a frequently interposed library,
and it will go away with the switch to zlib-ng), and nauty
<https://pallini.di.uniroma1.it/>.  I don't know how that was produced.
I do not evidence of other uses for global symbols, neither from
function multi-versioning nor from manually written clones.  (There
might be some additional use in the form of IRELATIVE relocations.)

Thanks,
Florian
  

On Thu, Dec 7, 2023 at 1:46 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> > specifically, my anecdata from Android [both open-source libraries and
> > apps] is that basically no-one uses ifuncs beyond libc and
> > compiler-generated fmv ifuncs [though for riscv64 that's still "future
> > work"], so there's no market for helpers for hand-written ifuncs?
>
> It used to be more widely used on GNU/Linux, for example:
>
>   implemented enabling sized-delete support at runtime
>
>   Under gcc 4.5 or greater we're using ifunc function attribute to
>   resolve sized delete operator to either plain delete implementation
>   (default) or to sized delete (if enabled via environment variable
>   TCMALLOC_ENABLE_SIZED_DELETE).
>
>   <https://github.com/gperftools/gperftools/commit/6fdfc5a7f40ebcff3fdaada1a2994ff54be2f9c7>
>
> After distributions turned on BIND_NOW, using IFUNCs for purposes like
> that became much more difficult.  We are getting closer to re-enabling
> getenv for use in IFUNC resolvers if the ELF dependencies express a
> usable relocatoion order, but until my delayed IFUNC resolution patch is
> accepted, IFUNC resolvers won't interoperate will with symbol
> interposition or underlinking, so compatibility problems remain.
>
> Outside GCC and glibc, I only see an IFUNC resolver in zlib (which is
> known to be problematic because it's a frequently interposed library,
> and it will go away with the switch to zlib-ng),

interesting... which zlib was that? i didn't see an ifunc in any of
the zlib forks i looked at, most (all?) of which exist because madler
zlib won't accept architecture-specific optimizations :-)

because macOS/iOS doesn't have ifuncs (and presumably Windows doesn't
either?), the zlib forks i'm aware of "roll their own" with
pthread_once() and function pointers or equivalents, since that works
everywhere.

> and nauty
> <https://pallini.di.uniroma1.it/>.  I don't know how that was produced.
> I do not evidence of other uses for global symbols, neither from
> function multi-versioning nor from manually written clones.  (There
> might be some additional use in the form of IRELATIVE relocations.)
>
> Thanks,
> Florian
>
  

> On Thu, Dec 7, 2023 at 1:46 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> > specifically, my anecdata from Android [both open-source libraries and
>> > apps] is that basically no-one uses ifuncs beyond libc and
>> > compiler-generated fmv ifuncs [though for riscv64 that's still "future
>> > work"], so there's no market for helpers for hand-written ifuncs?
>>
>> It used to be more widely used on GNU/Linux, for example:
>>
>>   implemented enabling sized-delete support at runtime
>>
>>   Under gcc 4.5 or greater we're using ifunc function attribute to
>>   resolve sized delete operator to either plain delete implementation
>>   (default) or to sized delete (if enabled via environment variable
>>   TCMALLOC_ENABLE_SIZED_DELETE).
>>
>>   <https://github.com/gperftools/gperftools/commit/6fdfc5a7f40ebcff3fdaada1a2994ff54be2f9c7>
>>
>> After distributions turned on BIND_NOW, using IFUNCs for purposes like
>> that became much more difficult.  We are getting closer to re-enabling
>> getenv for use in IFUNC resolvers if the ELF dependencies express a
>> usable relocatoion order, but until my delayed IFUNC resolution patch is
>> accepted, IFUNC resolvers won't interoperate will with symbol
>> interposition or underlinking, so compatibility problems remain.
>>
>> Outside GCC and glibc, I only see an IFUNC resolver in zlib (which is
>> known to be problematic because it's a frequently interposed library,
>> and it will go away with the switch to zlib-ng),
>
> interesting... which zlib was that? i didn't see an ifunc in any of
> the zlib forks i looked at, most (all?) of which exist because madler
> zlib won't accept architecture-specific optimizations :-)

It's our own fork I guess.

  <https://gitlab.com/redhat/centos-stream/rpms/zlib/-/blob/cec3445f7b544f6562f9ce52be6de97af7dc142b/zlib-1.2.11-Add-Power8-optimized-crc32.patch>

It's not even necessary for us because on ppc64le (as defined by
upstream/OpenPOWER), we always have at least POWER8 CPUs with support
for this instruction. 8-/

Thanks,
Florian
  

On Thu, Dec 7, 2023 at 8:47 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> > On Thu, Dec 7, 2023 at 1:46 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> > specifically, my anecdata from Android [both open-source libraries and
> >> > apps] is that basically no-one uses ifuncs beyond libc and
> >> > compiler-generated fmv ifuncs [though for riscv64 that's still "future
> >> > work"], so there's no market for helpers for hand-written ifuncs?
> >>
> >> It used to be more widely used on GNU/Linux, for example:
> >>
> >>   implemented enabling sized-delete support at runtime
> >>
> >>   Under gcc 4.5 or greater we're using ifunc function attribute to
> >>   resolve sized delete operator to either plain delete implementation
> >>   (default) or to sized delete (if enabled via environment variable
> >>   TCMALLOC_ENABLE_SIZED_DELETE).
> >>
> >>   <https://github.com/gperftools/gperftools/commit/6fdfc5a7f40ebcff3fdaada1a2994ff54be2f9c7>
> >>
> >> After distributions turned on BIND_NOW, using IFUNCs for purposes like
> >> that became much more difficult.  We are getting closer to re-enabling
> >> getenv for use in IFUNC resolvers if the ELF dependencies express a
> >> usable relocatoion order, but until my delayed IFUNC resolution patch is
> >> accepted, IFUNC resolvers won't interoperate will with symbol
> >> interposition or underlinking, so compatibility problems remain.
> >>
> >> Outside GCC and glibc, I only see an IFUNC resolver in zlib (which is
> >> known to be problematic because it's a frequently interposed library,
> >> and it will go away with the switch to zlib-ng),
> >
> > interesting... which zlib was that? i didn't see an ifunc in any of
> > the zlib forks i looked at, most (all?) of which exist because madler
> > zlib won't accept architecture-specific optimizations :-)
>
> It's our own fork I guess.

lol... yeah, i should have guessed that all distros probably hit this
problem before the "usual" forks even existed!

(/me really wishes the consensus around zlib-ng had formed before i
moved Android from madler to chromium-zlib a few years ago...)

>   <https://gitlab.com/redhat/centos-stream/rpms/zlib/-/blob/cec3445f7b544f6562f9ce52be6de97af7dc142b/zlib-1.2.11-Add-Power8-optimized-crc32.patch>
>
> It's not even necessary for us because on ppc64le (as defined by
> upstream/OpenPOWER), we always have at least POWER8 CPUs with support
> for this instruction. 8-/
>
> Thanks,
> Florian
>