[PATCH v2 0/3] selftests/futex: clang-inspired fixes

[PATCH v2 0/3] selftests/futex: clang-inspired fixes

[John Hubbard]

Hi,

Here's a few fixes that are part of my effort to get all selftests
building cleanly under clang. Plus one that I noticed by inspection.

Changes since the first version:

1) Rebased onto Linux 6.10-rc1
2) Added Reviewed-by's.

...and it turns out that all three patches are still required, on -rc1,
in order to get a clean clang build.

Enjoy!

thanks,
John Hubbard

John Hubbard (3):
  selftests/futex: don't redefine .PHONY targets (all, clean)
  selftests/futex: don't pass a const char* to asprintf(3)
  selftests/futex: pass _GNU_SOURCE without a value to the compiler

 tools/testing/selftests/futex/Makefile                      | 2 --
 tools/testing/selftests/futex/functional/Makefile           | 2 +-
 tools/testing/selftests/futex/functional/futex_requeue_pi.c | 2 +-
 3 files changed, 2 insertions(+), 4 deletions(-)


base-commit: e0cce98fe279b64f4a7d81b7f5c3a23d80b92fbc
-- 
2.45.1

[PATCH v2 1/3] selftests/futex: don't redefine .PHONY targets (all, clean)

[John Hubbard]

The .PHONY targets "all" and "clean"  are both defined in the file that
is included in the very next line: ../lib.mk.

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/futex/Makefile | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/testing/selftests/futex/Makefile b/tools/testing/selftests/futex/Makefile
index 11e157d7533b..78ab2cd111f6 100644
--- a/tools/testing/selftests/futex/Makefile
+++ b/tools/testing/selftests/futex/Makefile
@@ -3,8 +3,6 @@ SUBDIRS := functional
 
 TEST_PROGS := run.sh
 
-.PHONY: all clean
-
 include ../lib.mk
 
 all:
-- 
2.45.1

Re: [PATCH v2 1/3] selftests/futex: don't redefine .PHONY targets (all, clean)

[Shuah Khan]

On 5/28/24 20:29, John Hubbard wrote:
> The .PHONY targets "all" and "clean"  are both defined in the file that
> is included in the very next line: ../lib.mk.
> 

What problems are you seeing without this patch?
If I recall correctly, futex needs these defined.

Please provide information on why this change is
needed.

> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>   tools/testing/selftests/futex/Makefile | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/futex/Makefile b/tools/testing/selftests/futex/Makefile
> index 11e157d7533b..78ab2cd111f6 100644
> --- a/tools/testing/selftests/futex/Makefile
> +++ b/tools/testing/selftests/futex/Makefile
> @@ -3,8 +3,6 @@ SUBDIRS := functional
>   
>   TEST_PROGS := run.sh
>   
> -.PHONY: all clean
> -
>   include ../lib.mk
>   
>   all:

thanks,
-- Shuah

Re: [PATCH v2 1/3] selftests/futex: don't redefine .PHONY targets (all, clean)

[John Hubbard]

On 5/30/24 12:03 PM, Shuah Khan wrote:
> On 5/28/24 20:29, John Hubbard wrote:
>> The .PHONY targets "all" and "clean"  are both defined in the file that
>> is included in the very next line: ../lib.mk.
>>
> 
> What problems are you seeing without this patch?

Code duplication. It's a sin. :)

> If I recall correctly, futex needs these defined.

And so they are defined, in the very next line:

     include ../lib.mk

...which has this:

.PHONY: run_tests all clean install emit_tests gen_mods_dir clean_mods_dir

> 
> Please provide information on why this change is
> needed.

This patch is a valid cleanup, and doesn't introduce any problems
that I'm aware of. If there *are* problems that show up, those would
be deeper bugs, and I'll be happy to look into them and post solutions
if it comes up.

We don't just let latent bugs rest in peace. We fix them.

thanks,
-- 
John Hubbard
NVIDIA

> 
>> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>> ---
>>   tools/testing/selftests/futex/Makefile | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/futex/Makefile 
>> b/tools/testing/selftests/futex/Makefile
>> index 11e157d7533b..78ab2cd111f6 100644
>> --- a/tools/testing/selftests/futex/Makefile
>> +++ b/tools/testing/selftests/futex/Makefile
>> @@ -3,8 +3,6 @@ SUBDIRS := functional
>>   TEST_PROGS := run.sh
>> -.PHONY: all clean
>> -
>>   include ../lib.mk
>>   all:
> 
> thanks,
> -- Shuah

Re: [PATCH v2 1/3] selftests/futex: don't redefine .PHONY targets (all, clean)

[Shuah Khan]

On 5/30/24 13:13, John Hubbard wrote:
> On 5/30/24 12:03 PM, Shuah Khan wrote:
>> On 5/28/24 20:29, John Hubbard wrote:
>>> The .PHONY targets "all" and "clean"  are both defined in the file that
>>> is included in the very next line: ../lib.mk.
>>>
>>
>> What problems are you seeing without this patch?
> 
> Code duplication. It's a sin. :)

Please mention that you are removing duplicate code.

futex Makefile overrides CLEAN - just making sure it
does the cleanup correctly.

thanks,
-- Shuah

Re: [PATCH v2 1/3] selftests/futex: don't redefine .PHONY targets (all, clean)

[John Hubbard]

On 5/30/24 2:12 PM, Shuah Khan wrote:
> On 5/30/24 13:13, John Hubbard wrote:
>> On 5/30/24 12:03 PM, Shuah Khan wrote:
>>> On 5/28/24 20:29, John Hubbard wrote:
>>>> The .PHONY targets "all" and "clean"  are both defined in the file that
>>>> is included in the very next line: ../lib.mk.
>>>>
>>>
>>> What problems are you seeing without this patch?
>>
>> Code duplication. It's a sin. :)
> 
> Please mention that you are removing duplicate code.
> 
> futex Makefile overrides CLEAN - just making sure it

(Overrides of Makefile things are per-item, so to speak, not per
file, just to be clear.)

> does the cleanup correctly.
> 

OK sure, I'll update the commit message and post a v3, coming soon.


thanks,
-- 
John Hubbard
NVIDIA

[PATCH v2 2/3] selftests/futex: don't pass a const char* to asprintf(3)

[John Hubbard]

When building with clang, via:

    make LLVM=1 -C tools/testing/selftests

...clang issues a warning, because test_name is passed into asprintf(3),
which then changes it.

Fix this by simply removing the const qualifier. This is a local
automatic variable in a very short function, so there is not much need
to use the compiler to enforce const-ness at this scope.

[1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c49f@valentinobst.de/

Fixes: f17d8a87ecb5 ("selftests: fuxex: Report a unique test name per run of futex_requeue_pi")
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/futex/functional/futex_requeue_pi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/futex/functional/futex_requeue_pi.c b/tools/testing/selftests/futex/functional/futex_requeue_pi.c
index 7f3ca5c78df1..215c6cb539b4 100644
--- a/tools/testing/selftests/futex/functional/futex_requeue_pi.c
+++ b/tools/testing/selftests/futex/functional/futex_requeue_pi.c
@@ -360,7 +360,7 @@ int unit_test(int broadcast, long lock, int third_party_owner, long timeout_ns)
 
 int main(int argc, char *argv[])
 {
-	const char *test_name;
+	char *test_name;
 	int c, ret;
 
 	while ((c = getopt(argc, argv, "bchlot:v:")) != -1) {
-- 
2.45.1

Re: [PATCH v2 2/3] selftests/futex: don't pass a const char* to asprintf(3)

[Shuah Khan]

On 5/28/24 20:29, John Hubbard wrote:
> When building with clang, via:
> 
>      make LLVM=1 -C tools/testing/selftests
> 
> ...clang issues a warning, because test_name is passed into asprintf(3),
> which then changes it.

Please include the warning in the commit log.

> 
> Fix this by simply removing the const qualifier. This is a local
> automatic variable in a very short function, so there is not much need
> to use the compiler to enforce const-ness at this scope.
> 
> [1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c49f@valentinobst.de/
> 
> Fixes: f17d8a87ecb5 ("selftests: fuxex: Report a unique test name per run of futex_requeue_pi")
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>   tools/testing/selftests/futex/functional/futex_requeue_pi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/futex/functional/futex_requeue_pi.c b/tools/testing/selftests/futex/functional/futex_requeue_pi.c
> index 7f3ca5c78df1..215c6cb539b4 100644
> --- a/tools/testing/selftests/futex/functional/futex_requeue_pi.c
> +++ b/tools/testing/selftests/futex/functional/futex_requeue_pi.c
> @@ -360,7 +360,7 @@ int unit_test(int broadcast, long lock, int third_party_owner, long timeout_ns)
>   
>   int main(int argc, char *argv[])
>   {
> -	const char *test_name;
> +	char *test_name;
>   	int c, ret;
>   
>   	while ((c = getopt(argc, argv, "bchlot:v:")) != -1) {

thanks,
-- Shuah

Re: [PATCH v2 2/3] selftests/futex: don't pass a const char* to asprintf(3)

[John Hubbard]

On 5/30/24 12:04 PM, Shuah Khan wrote:
> On 5/28/24 20:29, John Hubbard wrote:
>> When building with clang, via:
>>
>>      make LLVM=1 -C tools/testing/selftests
>>
>> ...clang issues a warning, because test_name is passed into asprintf(3),
>> which then changes it.
> 
> Please include the warning in the commit log.

Ah, OK, the warning is:

futex_requeue_pi.c:403:17: warning: passing 'const char **' to parameter
of type 'char **' discards qualifiers in nested pointer types
[-Wincompatible-pointer-types-discards-qualifiers]


Please let me know if you'd prefer a v3, or if you'd rather fix it
up, whatever seems easiest for you.


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH v2 2/3] selftests/futex: don't pass a const char* to asprintf(3)

[Shuah Khan]

On 5/30/24 13:16, John Hubbard wrote:
> On 5/30/24 12:04 PM, Shuah Khan wrote:
>> On 5/28/24 20:29, John Hubbard wrote:
>>> When building with clang, via:
>>>
>>>      make LLVM=1 -C tools/testing/selftests
>>>
>>> ...clang issues a warning, because test_name is passed into asprintf(3),
>>> which then changes it.
>>
>> Please include the warning in the commit log.
> 
> Ah, OK, the warning is:
> 
> futex_requeue_pi.c:403:17: warning: passing 'const char **' to parameter
> of type 'char **' discards qualifiers in nested pointer types
> [-Wincompatible-pointer-types-discards-qualifiers]
> 
> 
> Please let me know if you'd prefer a v3, or if you'd rather fix it
> up, whatever seems easiest for you.
> 

Yes please send me v3.

thanks,
-- Shuah

Re: [PATCH v2 2/3] selftests/futex: don't pass a const char* to asprintf(3)

[John Hubbard]

On 5/30/24 2:13 PM, Shuah Khan wrote:
> On 5/30/24 13:16, John Hubbard wrote:
>> On 5/30/24 12:04 PM, Shuah Khan wrote:
>>> On 5/28/24 20:29, John Hubbard wrote:
>>>> When building with clang, via:
>>>>
>>>>      make LLVM=1 -C tools/testing/selftests
>>>>
>>>> ...clang issues a warning, because test_name is passed into 
>>>> asprintf(3),
>>>> which then changes it.
>>>
>>> Please include the warning in the commit log.
>>
>> Ah, OK, the warning is:
>>
>> futex_requeue_pi.c:403:17: warning: passing 'const char **' to parameter
>> of type 'char **' discards qualifiers in nested pointer types
>> [-Wincompatible-pointer-types-discards-qualifiers]
>>
>>
>> Please let me know if you'd prefer a v3, or if you'd rather fix it
>> up, whatever seems easiest for you.
>>
> 
> Yes please send me v3.
> 

OK, coming soon, hopefully a bit later today, in fact.

thanks,
-- 
John Hubbard
NVIDIA

[PATCH v2 3/3] selftests/futex: pass _GNU_SOURCE without a value to the compiler

[John Hubbard]

It's slightly better to set _GNU_SOURCE in the source code, but if one
must do it via the compiler invocation, then the best way to do so is
this:

    $(CC) -D_GNU_SOURCE=

...because otherwise, if this form is used:

    $(CC) -D_GNU_SOURCE

...then that leads the compiler to set a value, as if you had passed in:

    $(CC) -D_GNU_SOURCE=1

That, in turn, leads to warnings under both gcc and clang, like this:

    futex_requeue_pi.c:20: warning: "_GNU_SOURCE" redefined

Fix this by using the "-D_GNU_SOURCE=" form.

Reviewed-by: Edward Liaw <edliaw@google.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/futex/functional/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/futex/functional/Makefile b/tools/testing/selftests/futex/functional/Makefile
index a392d0917b4e..994fa3468f17 100644
--- a/tools/testing/selftests/futex/functional/Makefile
+++ b/tools/testing/selftests/futex/functional/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 INCLUDES := -I../include -I../../ $(KHDR_INCLUDES)
-CFLAGS := $(CFLAGS) -g -O2 -Wall -D_GNU_SOURCE -pthread $(INCLUDES) $(KHDR_INCLUDES)
+CFLAGS := $(CFLAGS) -g -O2 -Wall -D_GNU_SOURCE= -pthread $(INCLUDES) $(KHDR_INCLUDES)
 LDLIBS := -lpthread -lrt
 
 LOCAL_HDRS := \
-- 
2.45.1

Re: [PATCH v2 3/3] selftests/futex: pass _GNU_SOURCE without a value to the compiler

[Shuah Khan]

On 5/28/24 20:29, John Hubbard wrote:
> It's slightly better to set _GNU_SOURCE in the source code, but if one
> must do it via the compiler invocation, then the best way to do so is
> this:
> 
>      $(CC) -D_GNU_SOURCE=
> 
> ...because otherwise, if this form is used:
> 
>      $(CC) -D_GNU_SOURCE
> 
> ...then that leads the compiler to set a value, as if you had passed in:
> 
>      $(CC) -D_GNU_SOURCE=1
> 
> That, in turn, leads to warnings under both gcc and clang, like this:
> 
>      futex_requeue_pi.c:20: warning: "_GNU_SOURCE" redefined
> 
> Fix this by using the "-D_GNU_SOURCE=" form.
> 
> Reviewed-by: Edward Liaw <edliaw@google.com>
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>   tools/testing/selftests/futex/functional/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/futex/functional/Makefile b/tools/testing/selftests/futex/functional/Makefile
> index a392d0917b4e..994fa3468f17 100644
> --- a/tools/testing/selftests/futex/functional/Makefile
> +++ b/tools/testing/selftests/futex/functional/Makefile
> @@ -1,6 +1,6 @@
>   # SPDX-License-Identifier: GPL-2.0
>   INCLUDES := -I../include -I../../ $(KHDR_INCLUDES)
> -CFLAGS := $(CFLAGS) -g -O2 -Wall -D_GNU_SOURCE -pthread $(INCLUDES) $(KHDR_INCLUDES)
> +CFLAGS := $(CFLAGS) -g -O2 -Wall -D_GNU_SOURCE= -pthread $(INCLUDES) $(KHDR_INCLUDES)
>   LDLIBS := -lpthread -lrt
>   
>   LOCAL_HDRS := \

This looks fine. Applied now to linux-kselftest fixes branch

thanks,
-- Shuah

Re: [PATCH v2 0/3] selftests/futex: clang-inspired fixes

[Shuah Khan]

On 5/28/24 20:29, John Hubbard wrote:
> Hi,
> 
> Here's a few fixes that are part of my effort to get all selftests
> building cleanly under clang. Plus one that I noticed by inspection.
> 
> Changes since the first version:
> 
> 1) Rebased onto Linux 6.10-rc1
> 2) Added Reviewed-by's.
> 
> ...and it turns out that all three patches are still required, on -rc1,
> in order to get a clean clang build.

Thank you. I will apply these for the next rc.

thanks,
-- Shuah

Re: [PATCH v2 0/3] selftests/futex: clang-inspired fixes

[Shuah Khan]

On 5/30/24 08:23, Shuah Khan wrote:
> On 5/28/24 20:29, John Hubbard wrote:
>> Hi,
>>
>> Here's a few fixes that are part of my effort to get all selftests
>> building cleanly under clang. Plus one that I noticed by inspection.
>>
>> Changes since the first version:
>>
>> 1) Rebased onto Linux 6.10-rc1
>> 2) Added Reviewed-by's.
>>
>> ...and it turns out that all three patches are still required, on -rc1,
>> in order to get a clean clang build.
> 
> Thank you. I will apply these for the next rc.


John Hubbard (3):
   selftests/futex: don't redefine .PHONY targets (all, clean)
   selftests/futex: don't pass a const char* to asprintf(3)

Patch 1/3 and 2/3 please see changes requested.

Applied to linux-kselftest fixes branch
   selftests/futex: pass _GNU_SOURCE without a value to the compiler

thanks,
-- Shuah