[PATCH v2] riscv: Correct __riscv_hwprobe function prototype [BZ #32932]
Mark Harris
mark.hsj@gmail.com
Sat May 24 22:02:34 GMT 2025
On Wed, May 14, 2025 at 1:28 PM enh <enh@google.com> wrote:
> On Wed, May 14, 2025 at 2:53 PM Mark Harris <mark.hsj@gmail.com> wrote:
> > On Tue, May 13, 2025 at 2:18 PM enh <enh@google.com> wrote:
> > > On Wed, May 7, 2025 at 3:19 PM Mark Harris <mark.hsj@gmail.com> wrote:
> > > >
> > > > The third argument to __riscv_hwprobe is the size in bytes of the
> > > > cpu bitmask pointed to by the fourth argument, however in the access
> > > > attribute (read_only, 4, 3) it is used as an element count (i.e., the
> > > > number of unsigned longs that make up the bitmask), resulting in a
> > > > false compiler warning:
> > > >
> > > > $ gcc -c hwprobe1.c
> > > > hwprobe1.c: In function 'main':
> > > > hwprobe1.c:15:11: warning: '__riscv_hwprobe' reading 1024 bytes from a region of size 128 [-Wstringop-overread]
> > > > 15 | ret = __riscv_hwprobe (pairs, 1, sizeof(cpus), cpus, 0);
> > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > hwprobe1.c:9:23: note: source object 'cpus' of size 128
> > > > 9 | unsigned long int cpus[16];
> > > > | ^~~~
> > > > In file included from hwprobe1.c:1:
> > > > /usr/include/riscv64-linux-gnu/sys/hwprobe.h:66:12: note: in a call to function '__riscv_hwprobe' declared with attribute 'access (read_only, 4, 3)'
> > > > 66 | extern int __riscv_hwprobe (struct riscv_hwprobe *__pairs, size_t __pair_count,
> > > > | ^~~~~~~~~~~~~~~
> > > > $
> > > >
> > > > The documentation (https://docs.kernel.org/arch/riscv/hwprobe.html)
> > > > claims that the cpu bitmask has the type cpu_set_t *, which would be
> > > > consistent with other functions that take a cpu bitmask such as
> > > > sched_setaffinity and sched_getaffinity. It also uses the name
> > > > cpusetsize for the third argument, which is much more accurate than
> > > > cpu_count since it is a size in bytes and not a cpu count. The
> > > > (read_only, 4, 3) access attribute in the glibc prototype claims
> > > > that the cpu bitmask is only read, however when flags is
> > > > RISCV_HWPROBE_WHICH_CPUS it is both read and written.
> > > >
> > > > Therefore, in the glibc prototype the type of the fourth argument is
> > > > changed to cpu_set_t * to match the documentation, the name of the
> > > > third argument is changed to cpusetsize as in the documention, and the
> > > > incorrect access attribute that applies to these arguments is removed.
> > > > Almost all existing callers pass a null pointer for the fourth
> > > > argument, however a transparent union is introduced for compatibility
> > > > with callers that cast a pointer to the old argument type, and a
> > > > macro is introduced allowing callers the ability to distinguish
> > > > between the old and new prototype when needed.
> > > >
> > > > The access attributes are being specified with __fortified_attr_access,
> > > > however this macro is for fortified functions; the regular
> > > > __attr_access macro is for non-fortified functions such as this one.
> > > > Using the incorrect macro results in no access checks at fortify level
> > > > 3, because it is assumed that the fortified function will be doing the
> > > > checking. It is changed to use the correct macro so that the access
> > > > checks will work regardless of fortify level.
> > > >
> > > > Also because __riscv_hwprobe is not a cancellation point, __THROW
> > > > is added, consistent with similar functions. (However, it is omitted
> > > > from the typedef because GCC does not accept it there.)
> > > >
> > > > The __wur (warn_unused_result) attribute is helpful for functions that
> > > > cannot be used safely without checking the result, however there is
> > > > no such issue with __riscv_hwprobe; code such as the following should
> > > > not produce a warning:
> > > > struct riscv_hwprobe pair = { RISCV_HWPROBE_KEY_IMA_EXT_0, 0 };
> > > > __riscv_hwprobe (&pair, 1, 0, NULL, 0);
> > > > if (pair.value & RISCV_HWPROBE_EXT_ZBB) ...
> > > > Therefore this attribute is omitted.
> > >
> > > the similar
> > >
> > > struct riscv_hwprobe pair;
> > > pair.key = RISCV_HWPROBE_KEY_IMA_EXT_0;
> > >
> > > would be wrong though?
> >
> > warn_unused_result could potentially catch a bug if added to any
> > non-void function, but when the simplest safe and correct usage does
> > not require the result, and ignoring the result does not result in any
> > resource leaks or other problems, the attribute is more likely to be
> > an annoyance.
>
> ignoring the result _can_ result in problems if you haven't
> zero-initialized the array, no?
I will soften the wording of this sentence in the commit message.
>
> > Especially in this case I would prefer to promote the
> > usage where the entire structure is initialized and so the result does
> > not require checking.
>
> me too, but removing __wur doesn't really do anything to _promote_ the
> other idiom.
>
> btw, as evidence i don't care strongly about this, bionic never did
> have __wur on __riscv_hwprobe() --- i'm not really arguing with the
> idea so much as the exact wording of the comment. (which matters less
> for bionic because Android has stack zero-initialization on by
> default.)
Ok, the wording is easy to change.
>
> > Not only is it simpler, but all current uses
> > that I've seen will use syscall() directly if the libc function is not
> > available, which leads to an additional potential source of bugs if
> > the return value needs to be checked because those two interfaces use
> > a different convention for returning errors.
> >
> > In fact that exact bug is present in the third-party cpuinfo code that
> > you referenced below; it checks for errors using "if (ret < 0)" but
> > __riscv_hwprobe returns a positive errno value on error. (And even
> > for syscall(), which returns -1 on error, the code is arguably wrong
> > since it logs the value of ret (-1) on error instead of getting the
> > error from errno where syscall() stores it.)
>
> ugh, we really ought to fix ifuncs so we can stop adding crap api to
> work around potential problems from ifuncs not being sorted last :-(
Fixing ifuncs would be great.
>
> > > (actually, now you've made me look, i've found that one of the callers
> > > i have indexed is doing exactly this. though the fact that they assign
> > > to a "ret" that they then ignore probably means __wur wouldn't have
> > > kept them out of mischief either.)
> > >
> > > > The comment claiming that the second argument to the ifunc selector
> > > > is a pointer to the vDSO function is corrected. It is a pointer to
> > > > the regular glibc function (which returns errors as positive values),
> > > > not the vDSO function (which returns errors as negative values).
> > > >
> > > > Fixes commit 426d0e1aa8f17426d13707594111df712d2b8911 ("riscv: Add
> > > > Linux hwprobe syscall support").
> > > >
> > > > Fixes: BZ #32932
> > > > Signed-off-by: Mark Harris <mark.hsj@gmail.com>
> > > > ---
> > > > Changes in v2:
> > > > - Type of __riscv_hwprobe fourth argument changed (suggested by enh).
> > > > - Compatibility for C callers using the old argument type and new
> > > > macro to allow detection of old vs. new argument type.
> > > > - Include <sched.h> from <sys/hwprobe.h> for cpu_set_t.
> > > > - Remove access attribute for fourth argument because it is valid to
> > > > pass a pointer obtained from CPU_ALLOC that points to fewer bytes
> > > > than the fixed-size cpu_set_t type.
> > > >
> > > > ---
> > > > sysdeps/unix/sysv/linux/riscv/hwprobe.c | 4 +--
> > > > sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h | 40 +++++++++++++++------
> > > > 2 files changed, 31 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/sysdeps/unix/sysv/linux/riscv/hwprobe.c b/sysdeps/unix/sysv/linux/riscv/hwprobe.c
> > > > index e0cbd22cbe..bc7f6f38e1 100644
> > > > --- a/sysdeps/unix/sysv/linux/riscv/hwprobe.c
> > > > +++ b/sysdeps/unix/sysv/linux/riscv/hwprobe.c
> > > > @@ -23,13 +23,13 @@
> > > > #include <sysdep-vdso.h>
> > > >
> > > > int __riscv_hwprobe (struct riscv_hwprobe *pairs, size_t pair_count,
> > > > - size_t cpu_count, unsigned long int *cpus,
> > > > + size_t cpusetsize, __RISCV_HWPROBE_CPUS_TYPE cpus,
> > > > unsigned int flags)
> > > > {
> > > > int r;
> > > >
> > > > r = INTERNAL_VSYSCALL (riscv_hwprobe, 5, pairs, pair_count,
> > > > - cpu_count, cpus, flags);
> > > > + cpusetsize, cpus.__ul, flags);
> > > >
> > > > /* Negate negative errno values to match pthreads API. */
> > > > return -r;
> > > > diff --git a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> > > > index bebad6cf70..40415aae80 100644
> > > > --- a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> > > > +++ b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> > > > @@ -21,6 +21,7 @@
> > > > #define _SYS_HWPROBE_H 1
> > > >
> > > > #include <features.h>
> > > > +#include <sched.h>
> > > > #include <stddef.h>
> > > > #include <errno.h>
> > > > #ifdef __has_include
> > > > @@ -63,22 +64,39 @@ struct riscv_hwprobe {
> > > >
> > > > __BEGIN_DECLS
> > > >
> > > > -extern int __riscv_hwprobe (struct riscv_hwprobe *__pairs, size_t __pair_count,
> > > > - size_t __cpu_count, unsigned long int *__cpus,
> > > > +#if defined __cplusplus || !__GNUC_PREREQ (2, 7)
> > > > +# define __RISCV_HWPROBE_CPUS_TYPE cpu_set_t *
> > > > +#else
> > > > +/* The fourth argument to __riscv_hwprobe should be a null pointer or a
> > > > + pointer to a cpu_set_t (either the fixed-size type or allocated with
> > > > + CPU_ALLOC). However, early versions of this header file used the
> > > > + argument type unsigned long int *. The transparent union allows
> > > > + the argument to be either cpu_set_t * or unsigned long int * for
> > > > + compatibility. The older header file requiring unsigned long int *
> > > > + can be identified by the lack of the __RISCV_HWPROBE_CPUS_TYPE macro.
> > > > + In C++ and with compilers that do not support transparent unions, the
> > > > + argument type must be cpu_set_t *. */
> > > > +typedef union {
> > > > + cpu_set_t *__cs;
> > > > + unsigned long int *__ul;
> > > > +} __RISCV_HWPROBE_CPUS_TYPE __attribute__ ((__transparent_union__));
> > > > +# define __RISCV_HWPROBE_CPUS_TYPE __RISCV_HWPROBE_CPUS_TYPE
> > >
> > > (fwiw, for bionic i've gone with just changing to cpu_set_t* for
> > > everyone, on the basis of "the sooner we break it, the sooner it gets
> > > fixed". given that i've only found one caller. i've sent them a
> > > preparatory patch removing the cast they _never_ needed
> > > [https://github.com/pytorch/cpuinfo/pull/292] but no response to that
> > > so far :-( )
> >
> > That PR only removes the cast from the syscall(), not from
> > __riscv_hwprobe(), so if bionic __riscv_hwprobe() now requires
> > cpu_set_t * then won't the PR fail to build with the new bionic?
>
> indeed, but neither will their current code.
>
> job number 1 is getting them to pay attention to _any_ patch. (and
> having that patch in also means it's trivial for anyone to just use
> the other path --- at the moment, both paths have the incorrect cast.)
>
> but i need to know what version of the ndk the new signature will be
> in before i can send them the ideal patch for the other side of the
> #if. given how long it seems to take for them to look at patches, let
> me know if you'd like me to add the glibc version check too (and let
> me know what version of glibc that will be!).
Of course this change is not in any glibc release yet, however the
macro __RISCV_HWPROBE_CPUS_TYPE will be defined when building with
this updated header file.
>
> > > > +#endif
> > > > +
> > > > +extern int __riscv_hwprobe (struct riscv_hwprobe *__pairs,
> > > > + size_t __pair_count, size_t __cpusetsize,
> > > > + __RISCV_HWPROBE_CPUS_TYPE __cpus,
> > > > unsigned int __flags)
> > > > - __nonnull ((1)) __wur
> > > > - __fortified_attr_access (__read_write__, 1, 2)
> > > > - __fortified_attr_access (__read_only__, 4, 3);
> > > > + __THROW __nonnull ((1)) __attr_access ((__read_write__, 1, 2));
> > > >
> > > > -/* A pointer to the __riscv_hwprobe vDSO function is passed as the second
> > > > +/* A pointer to the __riscv_hwprobe function is passed as the second
> > > > argument to ifunc selector routines. Include a function pointer type for
> > > > convenience in calling the function in those settings. */
> > > > -typedef int (*__riscv_hwprobe_t) (struct riscv_hwprobe *__pairs, size_t __pair_count,
> > > > - size_t __cpu_count, unsigned long int *__cpus,
> > > > +typedef int (*__riscv_hwprobe_t) (struct riscv_hwprobe *__pairs,
> > > > + size_t __pair_count, size_t __cpusetsize,
> > > > + __RISCV_HWPROBE_CPUS_TYPE __cpus,
> > > > unsigned int __flags)
> > > > - __nonnull ((1)) __wur
> > > > - __fortified_attr_access (__read_write__, 1, 2)
> > > > - __fortified_attr_access (__read_only__, 4, 3);
> > > > + __nonnull ((1)) __attr_access ((__read_write__, 1, 2));
> > > >
> > > > /* Helper function usable from ifunc selectors that probes a single key. */
> > > > static __inline int
> > > > --
> > > > 2.49.0
> > > >
> >
> > - Mark
- Mark
More information about the Libc-alpha
mailing list