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

[Mathieu Desnoyers]

On 9/26/23 08:20, Justin Stitt wrote:
> Ping.
>
> Looking to get this patch and [1] slated for 6.7 which fixes some
> kselftest builds on older kernels.
>
> On Wed, Sep 13, 2023 at 6:03 AM Justin Stitt <justinstitt@xxxxxxxxxx> wrote:
> > When building with Clang, I am getting many warnings from the selftests/rseq tree.
> >
> > 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)
> > |        |                             ^                 ~~~~~~
> >
> > Use compiler builtins `__atomic_load_n()` and `__atomic_store_n()` with
> > accompanying __ATOMIC_ACQUIRE and __ATOMIC_RELEASE, respectively. This
> > will fix the warnings because the compiler builtins do not expect their
> > arguments to have _Atomic type. This should also make TSAN happier.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1698
> > Link: https://github.com/ClangBuiltLinux/continuous-integration2/issues/61
> > Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> > Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx>

Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>

Peter, should this go through tip ?

Shuah, should to go through selftests ?

Thanks,

Mathieu

> > ---
> > Note: Previous RFC https://lore.kernel.org/r/20230908-kselftest-param_test-c-v1-1-e35bd9052d61@xxxxxxxxxx
> > ---
> >   tools/testing/selftests/rseq/param_test.c | 20 ++++++++++----------
> >   1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/testing/selftests/rseq/param_test.c b/tools/testing/selftests/rseq/param_test.c
> > index bf951a490bb4..20403d58345c 100644
> > --- a/tools/testing/selftests/rseq/param_test.c
> > +++ b/tools/testing/selftests/rseq/param_test.c
> > @@ -1231,7 +1231,7 @@ void *test_membarrier_worker_thread(void *arg)
> >          }
> >
> >          /* Wait for initialization. */
> > -       while (!atomic_load(&args->percpu_list_ptr)) {}
> > +       while (!__atomic_load_n(&args->percpu_list_ptr, __ATOMIC_ACQUIRE)) {}
> >
> >          for (i = 0; i < iters; ++i) {
> >                  int ret;
> > @@ -1299,22 +1299,22 @@ void *test_membarrier_manager_thread(void *arg)
> >          test_membarrier_init_percpu_list(&list_a);
> >          test_membarrier_init_percpu_list(&list_b);
> >
> > -       atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
> > +       __atomic_store_n(&args->percpu_list_ptr, (intptr_t)&list_a, __ATOMIC_RELEASE);
> >
> > -       while (!atomic_load(&args->stop)) {
> > +       while (!__atomic_load_n(&args->stop, __ATOMIC_ACQUIRE)) {
> >                  /* list_a is "active". */
> >                  cpu_a = rand() % CPU_SETSIZE;
> >                  /*
> >                   * As list_b is "inactive", we should never see changes
> >                   * to list_b.
> >                   */
> > -               if (expect_b != atomic_load(&list_b.c[cpu_b].head->data)) {
> > +               if (expect_b != __atomic_load_n(&list_b.c[cpu_b].head->data, __ATOMIC_ACQUIRE)) {
> >                          fprintf(stderr, "Membarrier test failed\n");
> >                          abort();
> >                  }
> >
> >                  /* Make list_b "active". */
> > -               atomic_store(&args->percpu_list_ptr, (intptr_t)&list_b);
> > +               __atomic_store_n(&args->percpu_list_ptr, (intptr_t)&list_b, __ATOMIC_RELEASE);
> >                  if (rseq_membarrier_expedited(cpu_a) &&
> >                                  errno != ENXIO /* missing CPU */) {
> >                          perror("sys_membarrier");
> > @@ -1324,27 +1324,27 @@ void *test_membarrier_manager_thread(void *arg)
> >                   * Cpu A should now only modify list_b, so the values
> >                   * in list_a should be stable.
> >                   */
> > -               expect_a = atomic_load(&list_a.c[cpu_a].head->data);
> > +               expect_a = __atomic_load_n(&list_a.c[cpu_a].head->data, __ATOMIC_ACQUIRE);
> >
> >                  cpu_b = rand() % CPU_SETSIZE;
> >                  /*
> >                   * As list_a is "inactive", we should never see changes
> >                   * to list_a.
> >                   */
> > -               if (expect_a != atomic_load(&list_a.c[cpu_a].head->data)) {
> > +               if (expect_a != __atomic_load_n(&list_a.c[cpu_a].head->data, __ATOMIC_ACQUIRE)) {
> >                          fprintf(stderr, "Membarrier test failed\n");
> >                          abort();
> >                  }
> >
> >                  /* Make list_a "active". */
> > -               atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
> > +               __atomic_store_n(&args->percpu_list_ptr, (intptr_t)&list_a, __ATOMIC_RELEASE);
> >                  if (rseq_membarrier_expedited(cpu_b) &&
> >                                  errno != ENXIO /* missing CPU*/) {
> >                          perror("sys_membarrier");
> >                          abort();
> >                  }
> >                  /* Remember a value from list_b. */
> > -               expect_b = atomic_load(&list_b.c[cpu_b].head->data);
> > +               expect_b = __atomic_load_n(&list_b.c[cpu_b].head->data, __ATOMIC_ACQUIRE);
> >          }
> >
> >          test_membarrier_free_percpu_list(&list_a);
> > @@ -1401,7 +1401,7 @@ void test_membarrier(void)
> >                  }
> >          }
> >
> > -       atomic_store(&thread_args.stop, 1);
> > +       __atomic_store_n(&thread_args.stop, 1, __ATOMIC_RELEASE);
> >          ret = pthread_join(manager_thread, NULL);
> >          if (ret) {
> >                  errno = ret;
> >
> > ---
> > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > change-id: 20230908-kselftest-param_test-c-1763b62e762f
> >
> > Best regards,
> > --
> > Justin Stitt <justinstitt@xxxxxxxxxx>
>
> [1]: https://lore.kernel.org/all/20230908-kselftest-09-08-v2-0-0def978a4c1b@xxxxxxxxxx/
>
> Thanks
> Justin

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