[PATCH v3] riscv: Correct __riscv_hwprobe function prototype [BZ #32932]
Mark Harris
mark.hsj@gmail.com
Thu Jun 12 15:15:39 GMT 2025
On Thu, Jun 12, 2025 at 7:39 AM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 12/06/25 11:24, Mark Harris wrote:
> > 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.
>
> I will install it.
Thanks!
- Mark
More information about the Libc-alpha
mailing list