[PATCH v3] riscv: Correct __riscv_hwprobe function prototype [BZ #32932]
Mark Harris
mark.hsj@gmail.com
Thu Jun 12 14:24:47 GMT 2025
On Wed, Jun 11, 2025 at 9:12 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Sat, 24 May 2025 15:02:38 PDT (-0700), 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 documentation, 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 code such
> > as the following does not require the result to be checked and 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 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 v3:
> > - Update commit message to reflect comments from enh.
> > - Fix commit message typo.
> >
> > 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
> > +#endif
>
> Sorry for being slow here. I think maybe I saw an earlier version of
> this, or was just asleep or someththing -- I was worried changing these
> types would be some sort of compatibility issue, but this looks good to
> me.
>
> I suppose we could always add a second name for this with a different
> prototype and an alias, which would let us avoid the issues with
> compilers that don't have this transparent union trick. I'm fine with
> it as-is, though, as I doubt we have enough users to really matter.
>
> > +
> > +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
>
> Reviewed-by: Palmer Dabbelt <palmer@dabbelt.com>
> Acked-by: Palmer Dabbelt <palmer@dabbelt.com>
>
> Thanks!
Thanks for the review!
If there are no objections I'll need someone with commit access to push.
- Mark
More information about the Libc-alpha
mailing list