| Message ID | 20230908-kselftest-param_test-c-v1-1-e35bd9052d61@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] selftests/rseq: fix kselftest Clang build warnings | expand |
On Fri, Sep 8, 2023 at 4:03 PM Justin Stitt <justinstitt@google.com> 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) > | | ^ ~~~~~~ ah, so __c11_atomic_load is a compiler built in (similar to cstdint.h) (see the path of the note diagnostic). Your changes below look correct to me (except for the last hunk. percpu_list_ptr is already an intptr_t, the cast seems redundant); does GCC error with your below changes? > > 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 > > 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> >
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. 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> >
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? > > 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 >
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 >>
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)); }
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 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(-) --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230908-kselftest-param_test-c-1763b62e762f Best regards, -- Justin Stitt <justinstitt@google.com>