Re: [PATCH RFC] selftests/rseq: fix kselftest Clang build warnings

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

On 9/11/23 12:53, Justin Stitt wrote:
> On Sat, Sep 9, 2023 at 5:37 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> On 9/8/23 19:03, Justin Stitt wrote:
>>> Hi,
>>>
>>> I am experiencing many warnings when trying to build tools/testing/selftests.
>>>
>>> Here's one such example from rseq tree:
>>> |  param_test.c:1234:10: error: address argument to atomic operation must be a pointer to _Atomic type ('intptr_t *' (aka 'long *') invalid)
>>> |   1234 |         while (!atomic_load(&args->percpu_list_ptr)) {}
>>> |        |                 ^           ~~~~~~~~~~~~~~~~~~~~~~
>>> |  /usr/local/google/home/justinstitt/repos/tc-build/build/llvm/final/lib/clang/18/include/stdatomic.h:140:29: note: expanded from macro 'atomic_load'
>>> |    140 | #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST)
>>> |        |                             ^                 ~~~~~~
>>>
>>> I added the _Atomic type in various locations to silence _all_ (10) of these
>>> warnings. I'm wondering, though, perhaps the absence of these _Atomic
>>> types in the first place is on purpose? Am I on the right track to fix
>>> these warnings without damaging the legitimacy of the tests at hand?
>>>
>>> I'd like some feedback about where to go from here and if others are
>>> experiencing the same issues. Thanks!
>>>
>>> FWIW here's my specific build incantation on Clang-18 (49d41de57896e935cd5726719c5208bce22694ae):
>>> $ make LLVM=1 -j128 ARCH=x86_64 mrproper headers defconfig kselftest-merge
>>> $ make LLVM=1 ARCH=x86_64 -C tools/testing/selftests
>>
>> I should have used the __atomic_load_n() compiler builtin rather than
>> atomic_load(), mainly because it does not require the _Atomic annotation
>> to each type it touches.
> 
> Would you like me to send a patch in with this change?

Yes, please, although I suspect we'd want to turn atomic_store() into 
builtins as well.

And use a release MO for stores, and acquire for loads. This should make 
validators like TSAN happier.

Thanks,

Mathieu

> 
>>
>> Thanks,
>>
>> Mathieu
>>
>>
>>>
>>> Link: https://github.com/ClangBuiltLinux/linux/issues/1698
>>> Link: https://github.com/ClangBuiltLinux/continuous-integration2/issues/61
>>> Signed-off-by: Justin Stitt <justinstitt@google.com>
>>> ---
>>>    tools/testing/selftests/rseq/param_test.c | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/rseq/param_test.c b/tools/testing/selftests/rseq/param_test.c
>>> index bf951a490bb4..94802aeed2c6 100644
>>> --- a/tools/testing/selftests/rseq/param_test.c
>>> +++ b/tools/testing/selftests/rseq/param_test.c
>>> @@ -356,7 +356,7 @@ struct inc_thread_test_data {
>>>    };
>>>
>>>    struct percpu_list_node {
>>> -     intptr_t data;
>>> +     _Atomic intptr_t data;
>>>        struct percpu_list_node *next;
>>>    };
>>>
>>> @@ -1212,8 +1212,8 @@ static int set_signal_handler(void)
>>>    /* Test MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU membarrier command. */
>>>    #ifdef TEST_MEMBARRIER
>>>    struct test_membarrier_thread_args {
>>> -     int stop;
>>> -     intptr_t percpu_list_ptr;
>>> +     _Atomic int stop;
>>> +     _Atomic intptr_t percpu_list_ptr;
>>>    };
>>>
>>>    /* Worker threads modify data in their "active" percpu lists. */
>>> @@ -1240,7 +1240,7 @@ void *test_membarrier_worker_thread(void *arg)
>>>                        int cpu = get_current_cpu_id();
>>>
>>>                        ret = rseq_offset_deref_addv(RSEQ_MO_RELAXED, RSEQ_PERCPU,
>>> -                             &args->percpu_list_ptr,
>>> +                             (intptr_t*)&args->percpu_list_ptr,
>>>                                sizeof(struct percpu_list_entry) * cpu, 1, cpu);
>>>                } while (rseq_unlikely(ret));
>>>        }
>>>
>>> ---
>>> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
>>> change-id: 20230908-kselftest-param_test-c-1763b62e762f
>>>
>>> Best regards,
>>> --
>>> Justin Stitt <justinstitt@google.com>
>>>
>>
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
>> https://www.efficios.com
>>

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com