[PATCH v3] riscv: Correct __riscv_hwprobe function prototype [BZ #32932]
Adhemerval Zanella Netto
adhemerval.zanella@linaro.org
Thu Jun 12 14:38:57 GMT 2025
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.
More information about the Libc-alpha
mailing list