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
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:
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
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,
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
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,
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-2f9ed7d1c...
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) {
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-2f9ed7d1c...
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
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,
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
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,
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 := \
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=1That, in turn, leads to warnings under both gcc and clang, like this:
futex_requeue_pi.c:20: warning: "_GNU_SOURCE" redefinedFix 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
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:
- Rebased onto Linux 6.10-rc1
- 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
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:
- Rebased onto Linux 6.10-rc1
- 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
linux-kselftest-mirror@lists.linaro.org
-
John Hubbard -
Shuah Khan