[v2] selftests/user_events: silence a clang warning: address of packed member

[PATCH v2] selftests/user_events: silence a clang warning: address of packed member

Posted by John Hubbard 1 year, 6 months ago

When building with clang, via:

    make LLVM=1 -C tools/testing/selftest

...clang warns about "taking address of packed member 'write_index' ".
This is not particularly helpful, because the test code really wants to
write to exactly this location, and if it ends up being unaligned, then
the test won't work (and may segfault, depending on the CPU type).

If that ever comes up, it will be obvious and can be fixed. But all it's
doing now is prevent a clean clang build, so disable the warning.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---

Changes since the first version:

1) Rebased onto Linux 6.10-rc1


thanks,
John Hubbard


 tools/testing/selftests/user_events/Makefile | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/testing/selftests/user_events/Makefile b/tools/testing/selftests/user_events/Makefile
index 10fcd0066203..617e94344711 100644
--- a/tools/testing/selftests/user_events/Makefile
+++ b/tools/testing/selftests/user_events/Makefile
@@ -1,5 +1,10 @@
 # SPDX-License-Identifier: GPL-2.0
 CFLAGS += -Wl,-no-as-needed -Wall $(KHDR_INCLUDES)
+
+ifneq ($(LLVM),)
+    CFLAGS += -Wno-address-of-packed-member
+endif
+
 LDLIBS += -lrt -lpthread -lm
 
 TEST_GEN_PROGS = ftrace_test dyn_test perf_test abi_test

base-commit: 2bfcfd584ff5ccc8bb7acde19b42570414bf880b
-- 
2.45.1

Re: [PATCH v2] selftests/user_events: silence a clang warning: address of packed member

Posted by Nathan Chancellor 1 year, 6 months ago

Hi John,

On Mon, May 27, 2024 at 02:47:04PM -0700, John Hubbard wrote:
> When building with clang, via:
> 
>     make LLVM=1 -C tools/testing/selftest
> 
> ...clang warns about "taking address of packed member 'write_index' ".
> This is not particularly helpful, because the test code really wants to
> write to exactly this location, and if it ends up being unaligned, then
> the test won't work (and may segfault, depending on the CPU type).
> 
> If that ever comes up, it will be obvious and can be fixed. But all it's
> doing now is prevent a clean clang build, so disable the warning.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
> 
> Changes since the first version:
> 
> 1) Rebased onto Linux 6.10-rc1
> 
> 
> thanks,
> John Hubbard
> 
> 
>  tools/testing/selftests/user_events/Makefile | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/testing/selftests/user_events/Makefile b/tools/testing/selftests/user_events/Makefile
> index 10fcd0066203..617e94344711 100644
> --- a/tools/testing/selftests/user_events/Makefile
> +++ b/tools/testing/selftests/user_events/Makefile
> @@ -1,5 +1,10 @@
>  # SPDX-License-Identifier: GPL-2.0
>  CFLAGS += -Wl,-no-as-needed -Wall $(KHDR_INCLUDES)
> +
> +ifneq ($(LLVM),)

Perhaps it would be better if this were

  ifeq ($(CC),clang)

as that would catch both CC=clang and LLVM=1 users? I haven't tested
this though.

Additionally, I think it would be good to mention that
-Wno-address-of-packed-member is GCC's default, whereas Clang enables
-Waddress-of-packed-member by default.

> +    CFLAGS += -Wno-address-of-packed-member
> +endif
> +
>  LDLIBS += -lrt -lpthread -lm
>  
>  TEST_GEN_PROGS = ftrace_test dyn_test perf_test abi_test
> 
> base-commit: 2bfcfd584ff5ccc8bb7acde19b42570414bf880b
> -- 
> 2.45.1
> 
>

Re: [PATCH v2] selftests/user_events: silence a clang warning: address of packed member

Posted by Nathan Chancellor 1 year, 6 months ago

On Tue, May 28, 2024 at 01:28:33PM -0700, Nathan Chancellor wrote:
> Hi John,
> 
> On Mon, May 27, 2024 at 02:47:04PM -0700, John Hubbard wrote:
> > When building with clang, via:
> > 
> >     make LLVM=1 -C tools/testing/selftest
> > 
> > ...clang warns about "taking address of packed member 'write_index' ".
> > This is not particularly helpful, because the test code really wants to
> > write to exactly this location, and if it ends up being unaligned, then
> > the test won't work (and may segfault, depending on the CPU type).
> > 
> > If that ever comes up, it will be obvious and can be fixed. But all it's
> > doing now is prevent a clean clang build, so disable the warning.
> > 
> > Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> > ---
> > 
> > Changes since the first version:
> > 
> > 1) Rebased onto Linux 6.10-rc1
> > 
> > 
> > thanks,
> > John Hubbard
> > 
> > 
> >  tools/testing/selftests/user_events/Makefile | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/user_events/Makefile b/tools/testing/selftests/user_events/Makefile
> > index 10fcd0066203..617e94344711 100644
> > --- a/tools/testing/selftests/user_events/Makefile
> > +++ b/tools/testing/selftests/user_events/Makefile
> > @@ -1,5 +1,10 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  CFLAGS += -Wl,-no-as-needed -Wall $(KHDR_INCLUDES)
> > +
> > +ifneq ($(LLVM),)
> 
> Perhaps it would be better if this were
> 
>   ifeq ($(CC),clang)
> 
> as that would catch both CC=clang and LLVM=1 users? I haven't tested
> this though.

Hmmm, now that I am actually looking at tools/testing/selftests/lib.mk,
it seems like CC is only set to clang when $(LLVM) is set, so keeping it
the way it is now is probably best. Sorry about that, I am not too
familiar with the tools build system.

> Additionally, I think it would be good to mention that
> -Wno-address-of-packed-member is GCC's default, whereas Clang enables
> -Waddress-of-packed-member by default.
> 
> > +    CFLAGS += -Wno-address-of-packed-member
> > +endif
> > +
> >  LDLIBS += -lrt -lpthread -lm
> >  
> >  TEST_GEN_PROGS = ftrace_test dyn_test perf_test abi_test
> > 
> > base-commit: 2bfcfd584ff5ccc8bb7acde19b42570414bf880b
> > -- 
> > 2.45.1
> > 
> > 
>

Re: [PATCH v2] selftests/user_events: silence a clang warning: address of packed member

Posted by John Hubbard 1 year, 6 months ago

On 5/28/24 1:35 PM, Nathan Chancellor wrote:
> On Tue, May 28, 2024 at 01:28:33PM -0700, Nathan Chancellor wrote:
...
>>> diff --git a/tools/testing/selftests/user_events/Makefile b/tools/testing/selftests/user_events/Makefile
>>> index 10fcd0066203..617e94344711 100644
>>> --- a/tools/testing/selftests/user_events/Makefile
>>> +++ b/tools/testing/selftests/user_events/Makefile
>>> @@ -1,5 +1,10 @@
>>>   # SPDX-License-Identifier: GPL-2.0
>>>   CFLAGS += -Wl,-no-as-needed -Wall $(KHDR_INCLUDES)
>>> +
>>> +ifneq ($(LLVM),)
>>
>> Perhaps it would be better if this were
>>
>>    ifeq ($(CC),clang)
>>
>> as that would catch both CC=clang and LLVM=1 users? I haven't tested
>> this though.

That exact fix wouldn't quite work for the LLVM=1 case, because there,
CC is set to a horrible long mess, like this:

     CC = clang --target=x86_64-pc-linux-gnu -fintegrated-as

...but I see what you mean. It's not covering the CC=clang case.

> 
> Hmmm, now that I am actually looking at tools/testing/selftests/lib.mk,
> it seems like CC is only set to clang when $(LLVM) is set, so keeping it
> the way it is now is probably best. Sorry about that, I am not too
> familiar with the tools build system.
> 

That's true, and the patch here was admittedly only attempting to fix
the LLVM=1 case. However, I recall someone is building these kselftests
via "make CC=clang", which caught me by surprise, so I suppose that
should be fixed too. Maybe all of these checks (there are a few other
patches, but none merged yet) should be effectively:

     "if LLVM==1 or CC==clang, then apply the clang fix(es)"
	

>> Additionally, I think it would be good to mention that
>> -Wno-address-of-packed-member is GCC's default, whereas Clang enables
>> -Waddress-of-packed-member by default.

OK, I'll add that to a short comment nearby.

>>
>>> +    CFLAGS += -Wno-address-of-packed-member
>>> +endif
>>> +
>>>   LDLIBS += -lrt -lpthread -lm
>>>   
>>>   TEST_GEN_PROGS = ftrace_test dyn_test perf_test abi_test
>>>
>>> base-commit: 2bfcfd584ff5ccc8bb7acde19b42570414bf880b
>>> -- 
>>> 2.45.1
>>>
>>>
>>

thanks,
-- 
John Hubbard
NVIDIA