When building with clang via:
make LLVM=1 -C tools/testing/selftests
two distinct failures occur:
1) gcc requires -static-libasan in order to ensure that Address
Sanitizer's library is the first one loaded. However, this leads to
build failures on clang, when building via:
make LLVM=1 -C tools/testing/selftests
However, clang already does the right thing by default: it statically
links the Address Sanitizer if -fsanitize is specified. Therefore, fix
this by simply omitting -static-libasan for clang builds. And leave
behind a comment, because the whole reason for static linking might not
be obvious.
2) clang won't accept invocations of this form, but gcc will:
$(CC) file1.c header2.h
Fix this by using selftests/lib.mk facilities for tracking local header
file dependencies: add them to LOCAL_HDRS, leaving only the .c files to
be passed to the compiler.
Cc: Ryan Roberts <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
---
tools/testing/selftests/openat2/Makefile | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile
index 254d676a2689..185dc76ebb5f 100644
--- a/tools/testing/selftests/openat2/Makefile
+++ b/tools/testing/selftests/openat2/Makefile
@@ -1,8 +1,18 @@
# SPDX-License-Identifier: GPL-2.0-or-later
-CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined -static-libasan
+CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
+# gcc requires -static-libasan in order to ensure that Address Sanitizer's
+# library is the first one loaded. However, clang already statically links the
+# Address Sanitizer if -fsanitize is specified. Therefore, simply omit
+# -static-libasan for clang builds.
+ifeq ($(LLVM),)
+ CFLAGS += -static-libasan
+endif
+
+LOCAL_HDRS += helpers.h
+
include ../lib.mk
-$(TEST_GEN_PROGS): helpers.c helpers.h
+$(TEST_GEN_PROGS): helpers.c
base-commit: ddb4c3f25b7b95df3d6932db0b379d768a6ebdf7
prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
--
2.45.0
gcc requires -static-libasan in order to ensure that Address Sanitizer's
library is the first one loaded. However, this leads to build failures
on clang, when building via:
make LLVM=1 -C tools/testing/selftests
However, clang already does the right thing by default: it statically
links the Address Sanitizer if -fsanitize is specified. Therefore,
simply omit -static-libasan for clang builds. And leave behind a
comment, because the whole reason for static linking might not be
obvious.
Cc: Ryan Roberts <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
---
tools/testing/selftests/fchmodat2/Makefile | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/fchmodat2/Makefile b/tools/testing/selftests/fchmodat2/Makefile
index 71ec34bf1501..4373cea79b79 100644
--- a/tools/testing/selftests/fchmodat2/Makefile
+++ b/tools/testing/selftests/fchmodat2/Makefile
@@ -1,6 +1,15 @@
# SPDX-License-Identifier: GPL-2.0-or-later
-CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined -static-libasan $(KHDR_INCLUDES)
+CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined $(KHDR_INCLUDES)
+
+# gcc requires -static-libasan in order to ensure that Address Sanitizer's
+# library is the first one loaded. However, clang already statically links the
+# Address Sanitizer if -fsanitize is specified. Therefore, simply omit
+# -static-libasan for clang builds.
+ifeq ($(LLVM),)
+ CFLAGS += -static-libasan
+endif
+
TEST_GEN_PROGS := fchmodat2_test
include ../lib.mk
--
2.45.0
On 04/05/2024 05:43, John Hubbard wrote:
> When building with clang via:
>
> make LLVM=1 -C tools/testing/selftests
>
> two distinct failures occur:
>
> 1) gcc requires -static-libasan in order to ensure that Address
> Sanitizer's library is the first one loaded. However, this leads to
> build failures on clang, when building via:
>
> make LLVM=1 -C tools/testing/selftests
>
> However, clang already does the right thing by default: it statically
> links the Address Sanitizer if -fsanitize is specified. Therefore, fix
> this by simply omitting -static-libasan for clang builds. And leave
> behind a comment, because the whole reason for static linking might not
> be obvious.
>
> 2) clang won't accept invocations of this form, but gcc will:
>
> $(CC) file1.c header2.h
>
> Fix this by using selftests/lib.mk facilities for tracking local header
> file dependencies: add them to LOCAL_HDRS, leaving only the .c files to
> be passed to the compiler.
>
> Cc: Ryan Roberts <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
Hi John,
I sent out a similar fix a couple of weeks ago, see [1]. I don't think it got
picked up though. It takes a slightly different approach, explicitly adding
-static-libsan (note no 'a') for clang, instead of relying on its default.
And it just drops helpers.h from the makefile altogether, on the assumption that
it was a mistake; its just a header and shouldn't be compiled directly. I'm not
exactly sure what the benefit of adding it to LOCAL_HDRS is?
[1]
https://lore.kernel.org/linux-kselftest/[email protected]/
Thanks,
Ryan
> ---
> tools/testing/selftests/openat2/Makefile | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile
> index 254d676a2689..185dc76ebb5f 100644
> --- a/tools/testing/selftests/openat2/Makefile
> +++ b/tools/testing/selftests/openat2/Makefile
> @@ -1,8 +1,18 @@
> # SPDX-License-Identifier: GPL-2.0-or-later
>
> -CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined -static-libasan
> +CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
> TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
>
> +# gcc requires -static-libasan in order to ensure that Address Sanitizer's
> +# library is the first one loaded. However, clang already statically links the
> +# Address Sanitizer if -fsanitize is specified. Therefore, simply omit
> +# -static-libasan for clang builds.
> +ifeq ($(LLVM),)
> + CFLAGS += -static-libasan
> +endif
> +
> +LOCAL_HDRS += helpers.h
> +
> include ../lib.mk
>
> -$(TEST_GEN_PROGS): helpers.c helpers.h
> +$(TEST_GEN_PROGS): helpers.c
>
> base-commit: ddb4c3f25b7b95df3d6932db0b379d768a6ebdf7
> prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
On 07/05/2024 17:19, John Hubbard wrote:
> On 5/7/24 12:45 AM, Ryan Roberts wrote:
>> On 04/05/2024 05:43, John Hubbard wrote:
> ...
>> Hi John,
>>
>> I sent out a similar fix a couple of weeks ago, see [1]. I don't think it got
>> picked up though. It takes a slightly different approach, explicitly adding
>> -static-libsan (note no 'a') for clang, instead of relying on its default.
>>
>> And it just drops helpers.h from the makefile altogether, on the assumption that
>> it was a mistake; its just a header and shouldn't be compiled directly. I'm not
>> exactly sure what the benefit of adding it to LOCAL_HDRS is?
>
> Ah no, you must not drop headers.h. That's a mistake itself, because
> LOCAL_HDRS adds a Make dependency; that's its purpose. If you touch
> helpers.h it should cause a rebuild, which won't happen if you remove it
> from LOCAL_HDRS.
Ahh. I was under the impression that the compiler was configured to output the
list of dependencies for make to track (something like -M, from memory ?). Since
helpers.h is included from helpers.c I assumed it would be tracked like this - I
guess its not that simple?
Anyway, on the basis that LOCAL_HDRS is the right way to do this, let's go with
your version and drop mine:
Reviewed-by: Ryan Roberts <[email protected]>
>
> The way it works is that lib.mk adds $(LOCAL_HDRS) to the dependencies list,
> but then filters precisely that same set *out* of the list that it provides
> to the compile invocation.
>
> The other way to implement this requirement of "some things need to be
> Make dependencies, and some need to be both dependencies and compilation
> inputs", is to add everything to the dependency list, but then use a
> separate list of files to pass to the compiler. For an example of that,
> see $(EXTRA_FILES) in patch 1/7 [1] of my selftests/x86 cleanup.
>
> [1] https://lore.kernel.org/all/[email protected]/
>
> thanks,
> John Hubbard
>
>>
>> [1]
>> https://lore.kernel.org/linux-kselftest/[email protected]/
>>
>> Thanks,
>> Ryan
>>
>>
>>> ---
>>> tools/testing/selftests/openat2/Makefile | 14 ++++++++++++--
>>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/openat2/Makefile
>>> b/tools/testing/selftests/openat2/Makefile
>>> index 254d676a2689..185dc76ebb5f 100644
>>> --- a/tools/testing/selftests/openat2/Makefile
>>> +++ b/tools/testing/selftests/openat2/Makefile
>>> @@ -1,8 +1,18 @@
>>> # SPDX-License-Identifier: GPL-2.0-or-later
>>> -CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
>>> -static-libasan
>>> +CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
>>> TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
>>> +# gcc requires -static-libasan in order to ensure that Address Sanitizer's
>>> +# library is the first one loaded. However, clang already statically links the
>>> +# Address Sanitizer if -fsanitize is specified. Therefore, simply omit
>>> +# -static-libasan for clang builds.
>>> +ifeq ($(LLVM),)
>>> + CFLAGS += -static-libasan
>>> +endif
>>> +
>>> +LOCAL_HDRS += helpers.h
>>> +
>>> include ../lib.mk
>>> -$(TEST_GEN_PROGS): helpers.c helpers.h
>>> +$(TEST_GEN_PROGS): helpers.c
>>>
>>> base-commit: ddb4c3f25b7b95df3d6932db0b379d768a6ebdf7
>>> prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
>>
>
> thanks,
On 5/7/24 9:34 AM, Ryan Roberts wrote:
> On 07/05/2024 17:19, John Hubbard wrote:
>> On 5/7/24 12:45 AM, Ryan Roberts wrote:
>>> On 04/05/2024 05:43, John Hubbard wrote:
>> ...
>>> Hi John,
>>>
>>> I sent out a similar fix a couple of weeks ago, see [1]. I don't think it got
>>> picked up though. It takes a slightly different approach, explicitly adding
>>> -static-libsan (note no 'a') for clang, instead of relying on its default.
>>>
>>> And it just drops helpers.h from the makefile altogether, on the assumption that
>>> it was a mistake; its just a header and shouldn't be compiled directly. I'm not
>>> exactly sure what the benefit of adding it to LOCAL_HDRS is?
>>
>> Ah no, you must not drop headers.h. That's a mistake itself, because
>> LOCAL_HDRS adds a Make dependency; that's its purpose. If you touch
>> helpers.h it should cause a rebuild, which won't happen if you remove it
>> from LOCAL_HDRS.
>
> Ahh. I was under the impression that the compiler was configured to output the
> list of dependencies for make to track (something like -M, from memory ?). Since
> helpers.h is included from helpers.c I assumed it would be tracked like this - I
> guess its not that simple?
This can be done, but it is not automatic with GNU Make. You have to
explicitly
run gcc -M, capture the output in a dependencies list, and track it.
Which the
Kbuild system does, but kselftest does not.
After just now sweeping through kselftest to fix up the clang build, I see a
lot of mistaken or partial use of the kselftest build's Make variables,
because
people naturally reason based on what they know about Kbuild, and it doesn't
always translate. And LOCAL_HDRS might need some more documentation too.
I'll keep thinking about how to clarify this, I have a couple early ideas.
>
> Anyway, on the basis that LOCAL_HDRS is the right way to do this, let's go with
> your version and drop mine:
>
> Reviewed-by: Ryan Roberts <[email protected]>
>
Thanks for the review!
thanks,
--
John Hubbard
NVIDIA
>>
>> The way it works is that lib.mk adds $(LOCAL_HDRS) to the dependencies list,
>> but then filters precisely that same set *out* of the list that it provides
>> to the compile invocation.
>>
>> The other way to implement this requirement of "some things need to be
>> Make dependencies, and some need to be both dependencies and compilation
>> inputs", is to add everything to the dependency list, but then use a
>> separate list of files to pass to the compiler. For an example of that,
>> see $(EXTRA_FILES) in patch 1/7 [1] of my selftests/x86 cleanup.
>>
>> [1] https://lore.kernel.org/all/[email protected]/
>>
>> thanks,
>> John Hubbard
>>
>>>
>>> [1]
>>> https://lore.kernel.org/linux-kselftest/[email protected]/
>>>
>>> Thanks,
>>> Ryan
>>>
>>>
>>>> ---
>>>> tools/testing/selftests/openat2/Makefile | 14 ++++++++++++--
>>>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/openat2/Makefile
>>>> b/tools/testing/selftests/openat2/Makefile
>>>> index 254d676a2689..185dc76ebb5f 100644
>>>> --- a/tools/testing/selftests/openat2/Makefile
>>>> +++ b/tools/testing/selftests/openat2/Makefile
>>>> @@ -1,8 +1,18 @@
>>>> # SPDX-License-Identifier: GPL-2.0-or-later
>>>> -CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
>>>> -static-libasan
>>>> +CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
>>>> TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
>>>> +# gcc requires -static-libasan in order to ensure that Address Sanitizer's
>>>> +# library is the first one loaded. However, clang already statically links the
>>>> +# Address Sanitizer if -fsanitize is specified. Therefore, simply omit
>>>> +# -static-libasan for clang builds.
>>>> +ifeq ($(LLVM),)
>>>> + CFLAGS += -static-libasan
>>>> +endif
>>>> +
>>>> +LOCAL_HDRS += helpers.h
>>>> +
>>>> include ../lib.mk
>>>> -$(TEST_GEN_PROGS): helpers.c helpers.h
>>>> +$(TEST_GEN_PROGS): helpers.c
>>>>
>>>> base-commit: ddb4c3f25b7b95df3d6932db0b379d768a6ebdf7
>>>> prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
>>>
>>
>> thanks,
>
On 07/05/2024 17:47, John Hubbard wrote:
> On 5/7/24 9:34 AM, Ryan Roberts wrote:
>> On 07/05/2024 17:19, John Hubbard wrote:
>>> On 5/7/24 12:45 AM, Ryan Roberts wrote:
>>>> On 04/05/2024 05:43, John Hubbard wrote:
>>> ...
>>>> Hi John,
>>>>
>>>> I sent out a similar fix a couple of weeks ago, see [1]. I don't think it got
>>>> picked up though. It takes a slightly different approach, explicitly adding
>>>> -static-libsan (note no 'a') for clang, instead of relying on its default.
>>>>
>>>> And it just drops helpers.h from the makefile altogether, on the assumption
>>>> that
>>>> it was a mistake; its just a header and shouldn't be compiled directly. I'm not
>>>> exactly sure what the benefit of adding it to LOCAL_HDRS is?
>>>
>>> Ah no, you must not drop headers.h. That's a mistake itself, because
>>> LOCAL_HDRS adds a Make dependency; that's its purpose. If you touch
>>> helpers.h it should cause a rebuild, which won't happen if you remove it
>>> from LOCAL_HDRS.
>>
>> Ahh. I was under the impression that the compiler was configured to output the
>> list of dependencies for make to track (something like -M, from memory ?). Since
>> helpers.h is included from helpers.c I assumed it would be tracked like this - I
>> guess its not that simple?
>
> This can be done, but it is not automatic with GNU Make. You have to explicitly
> run gcc -M, capture the output in a dependencies list, and track it. Which the
> Kbuild system does, but kselftest does not.
Understood - thanks for the lesson!
>
> After just now sweeping through kselftest to fix up the clang build, I see a
> lot of mistaken or partial use of the kselftest build's Make variables, because
> people naturally reason based on what they know about Kbuild, and it doesn't
> always translate. And LOCAL_HDRS might need some more documentation too.
> I'll keep thinking about how to clarify this, I have a couple early ideas.
>
>>
>> Anyway, on the basis that LOCAL_HDRS is the right way to do this, let's go with
>> your version and drop mine:
>>
>> Reviewed-by: Ryan Roberts <[email protected]>
>>
>
> Thanks for the review!
>
> thanks,
On 04/05/2024 05:43, John Hubbard wrote:
> When building with clang via:
>
> make LLVM=1 -C tools/testing/selftests
>
> two distinct failures occur:
>
> 1) gcc requires -static-libasan in order to ensure that Address
> Sanitizer's library is the first one loaded. However, this leads to
> build failures on clang, when building via:
>
> make LLVM=1 -C tools/testing/selftests
>
> However, clang already does the right thing by default: it statically
> links the Address Sanitizer if -fsanitize is specified. Therefore, fix
> this by simply omitting -static-libasan for clang builds. And leave
> behind a comment, because the whole reason for static linking might not
> be obvious.
>
> 2) clang won't accept invocations of this form, but gcc will:
>
> $(CC) file1.c header2.h
>
> Fix this by using selftests/lib.mk facilities for tracking local header
> file dependencies: add them to LOCAL_HDRS, leaving only the .c files to
> be passed to the compiler.
>
> Cc: Ryan Roberts <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
> ---
> tools/testing/selftests/openat2/Makefile | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile
> index 254d676a2689..185dc76ebb5f 100644
> --- a/tools/testing/selftests/openat2/Makefile
> +++ b/tools/testing/selftests/openat2/Makefile
> @@ -1,8 +1,18 @@
> # SPDX-License-Identifier: GPL-2.0-or-later
>
> -CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined -static-libasan
> +CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
> TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
>
> +# gcc requires -static-libasan in order to ensure that Address Sanitizer's
> +# library is the first one loaded. However, clang already statically links the
> +# Address Sanitizer if -fsanitize is specified. Therefore, simply omit
> +# -static-libasan for clang builds.
> +ifeq ($(LLVM),)
> + CFLAGS += -static-libasan
> +endif
It just occured to me that the bug report I was fixing with my attempt was
invoking make like this (see [1]):
# tools/testing/selftests/fchmodat2$ make CC=clang
# tools/testing/selftests/openat2$ make CC=clang
So LLVM is not set in this case. Perhaps my approach [2] (suggested by Arnd) of
using cc-option is more robust? (cc-option is alredy used by other selftests).
[1] https://lore.kernel.org/all/[email protected]/
[2]
https://lore.kernel.org/linux-kselftest/[email protected]/
> +
> +LOCAL_HDRS += helpers.h
> +
> include ../lib.mk
>
> -$(TEST_GEN_PROGS): helpers.c helpers.h
> +$(TEST_GEN_PROGS): helpers.c
>
> base-commit: ddb4c3f25b7b95df3d6932db0b379d768a6ebdf7
> prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
On 5/10/24 10:56 AM, John Hubbard wrote:
> On 5/10/24 4:52 AM, Ryan Roberts wrote:
>> On 04/05/2024 05:43, John Hubbard wrote:
> ...
>> It just occured to me that the bug report I was fixing with my attempt
>> was
>> invoking make like this (see [1]):
>>
>> # tools/testing/selftests/fchmodat2$ make CC=clang
>> # tools/testing/selftests/openat2$ make CC=clang
>>
>> So LLVM is not set in this case. Perhaps my approach [2] (suggested by
>> Arnd) of
>> using cc-option is more robust? (cc-option is alredy used by other
>> selftests).
>>
>
> Yes, I think that would better handle the two cases: setting LLVM,
> and/or setting CC (!).
>
> For that, some nits, but only worth fussing over if the patch hasn't
> gone in yet, or if you're changing it for some other reason:
>
I just remembered it needs the LOCAL_HDRS approach as well. Did your
patch already go in? Should I fix up this one here to use cc-option,
or go with yours? Either way is fine with me.
thanks,
--
John Hubbard
NVIDIA
> In Make, the arguments to functions include *all* spaces, so it's good
> practice to not add spaces in most function calls, unless they are
> definitely desired.
>
> Also, you only ever want one of those $(CC) options, so saying so is a
> nice touch. Neither of these is a functional issue in [2], but you could
> do this on top of the patch (I'm only showing the openat2 case):
>
> diff --git a/tools/testing/selftests/openat2/Makefile
> b/tools/testing/selftests/openat2/Makefile
> index 02af9b6ca5eb..c894778874a5 100644
> --- a/tools/testing/selftests/openat2/Makefile
> +++ b/tools/testing/selftests/openat2/Makefile
> @@ -3,7 +3,7 @@
> include ../../../build/Build.include
>
> CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
> -CFLAGS += $(call cc-option, -static-libasan) $(call cc-option,
> -static-libsan)
> +CFLAGS += $(call cc-option,-static-libasan,$(call
> cc-option,-static-libsan))
> TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
>
> include ../lib.mk
>
>
>>
>> [1] https://lore.kernel.org/all/[email protected]/
>> [2]
>> https://lore.kernel.org/linux-kselftest/[email protected]/
>>
>>
>>> +
>>> +LOCAL_HDRS += helpers.h
>>> +
>>> include ../lib.mk
>>> -$(TEST_GEN_PROGS): helpers.c helpers.h
>>> +$(TEST_GEN_PROGS): helpers.c
>>>
>>> base-commit: ddb4c3f25b7b95df3d6932db0b379d768a6ebdf7
>>> prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
>>
>
> thanks,
On 10/05/2024 19:22, John Hubbard wrote:
> On 5/10/24 10:56 AM, John Hubbard wrote:
>> On 5/10/24 4:52 AM, Ryan Roberts wrote:
>>> On 04/05/2024 05:43, John Hubbard wrote:
>> ...
>>> It just occured to me that the bug report I was fixing with my attempt was
>>> invoking make like this (see [1]):
>>>
>>> # tools/testing/selftests/fchmodat2$ make CC=clang
>>> # tools/testing/selftests/openat2$ make CC=clang
>>>
>>> So LLVM is not set in this case. Perhaps my approach [2] (suggested by Arnd) of
>>> using cc-option is more robust? (cc-option is alredy used by other selftests).
>>>
>>
>> Yes, I think that would better handle the two cases: setting LLVM,
>> and/or setting CC (!).
>>
>> For that, some nits, but only worth fussing over if the patch hasn't
>> gone in yet, or if you're changing it for some other reason:
>>
>
> I just remembered it needs the LOCAL_HDRS approach as well. Did your
> patch already go in? Should I fix up this one here to use cc-option,
> or go with yours? Either way is fine with me.
I don't think my patch has been taken into any branch - I didn't see a
notification anyway. So it would be great if you are happy to take ownership of
it? - I'm on Paternity leave for the next 3 weeks so wouldn't get it done until
I get back.
>
> thanks,
On 5/10/24 10:56 PM, John Hubbard wrote:
> On 5/10/24 4:52 AM, Ryan Roberts wrote:
>> On 04/05/2024 05:43, John Hubbard wrote:
> ...
>> It just occured to me that the bug report I was fixing with my attempt was
>> invoking make like this (see [1]):
>>
>> # tools/testing/selftests/fchmodat2$ make CC=clang
>> # tools/testing/selftests/openat2$ make CC=clang
>>
>> So LLVM is not set in this case. Perhaps my approach [2] (suggested by
>> Arnd) of
>> using cc-option is more robust? (cc-option is alredy used by other
>> selftests).
>>
>
> Yes, I think that would better handle the two cases: setting LLVM,
> and/or setting CC (!).
>
> For that, some nits, but only worth fussing over if the patch hasn't
> gone in yet, or if you're changing it for some other reason:
>
> In Make, the arguments to functions include *all* spaces, so it's good
> practice to not add spaces in most function calls, unless they are
> definitely desired.
>
> Also, you only ever want one of those $(CC) options, so saying so is a
> nice touch. Neither of these is a functional issue in [2], but you could
> do this on top of the patch (I'm only showing the openat2 case):
I was building with CC=clang and build was still failing. We should be able
to build by CC=clang and LLVM=1 both. It seems like patches still haven't
been accepted. Let's get a v2 out to fix this.
>
> diff --git a/tools/testing/selftests/openat2/Makefile
> b/tools/testing/selftests/openat2/Makefile
> index 02af9b6ca5eb..c894778874a5 100644
> --- a/tools/testing/selftests/openat2/Makefile
> +++ b/tools/testing/selftests/openat2/Makefile
> @@ -3,7 +3,7 @@
> include ../../../build/Build.include
>
> CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
> -CFLAGS += $(call cc-option, -static-libasan) $(call cc-option,
> -static-libsan)
> +CFLAGS += $(call cc-option,-static-libasan,$(call cc-option,-static-libsan))
> TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
>
> include ../lib.mk
>
>
>>
>> [1] https://lore.kernel.org/all/[email protected]/
>> [2]
>> https://lore.kernel.org/linux-kselftest/[email protected]/
>>
>>
>>> +
>>> +LOCAL_HDRS += helpers.h
>>> +
>>> include ../lib.mk
>>> -$(TEST_GEN_PROGS): helpers.c helpers.h
>>> +$(TEST_GEN_PROGS): helpers.c
>>>
>>> base-commit: ddb4c3f25b7b95df3d6932db0b379d768a6ebdf7
>>> prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
>>
>
> thanks,
--
BR,
Muhammad Usama Anjum
On 5/29/24 8:58 PM, Muhammad Usama Anjum wrote:
> On 5/10/24 10:56 PM, John Hubbard wrote:
>> On 5/10/24 4:52 AM, Ryan Roberts wrote:
>>> On 04/05/2024 05:43, John Hubbard wrote:
>> ...
>> In Make, the arguments to functions include *all* spaces, so it's good
>> practice to not add spaces in most function calls, unless they are
>> definitely desired.
>>
>> Also, you only ever want one of those $(CC) options, so saying so is a
>> nice touch. Neither of these is a functional issue in [2], but you could
>> do this on top of the patch (I'm only showing the openat2 case):
> I was building with CC=clang and build was still failing. We should be able
> to build by CC=clang and LLVM=1 both. It seems like patches still haven't
> been accepted. Let's get a v2 out to fix this.
>
Yes, these are about next on the list, and I expect to post a v2 or its moral
equivalent tomorrow or the next day at the latest.
For the CC=clang vs. LLVM=1 issue in particular, I've already posted a
fix, please see my "[PATCH 0/2] selftests/lib.mk: LLVM=1, CC=clang, and
warnings" [1] that went out just yesterday.
[1] https://lore.kernel.org/[email protected]
thanks,
--
John Hubbard
NVIDIA