[PATCH 1/2] selftests/openat2: fix clang build failures: -static-libasan, LOCAL_HDRS

[PATCH 1/2] selftests/openat2: fix clang build failures: -static-libasan, LOCAL_HDRS

[John Hubbard]

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 <ryan.roberts@arm.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 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

[PATCH 2/2] selftests/fchmodat2: fix clang build failure due to -static-libasan

[John Hubbard]

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 <ryan.roberts@arm.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 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

Re: [PATCH 1/2] selftests/openat2: fix clang build failures: -static-libasan, LOCAL_HDRS

[Ryan Roberts]

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 <ryan.roberts@arm.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>

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/20240417160740.2019530-1-ryan.roberts@arm.com/

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

Re: [PATCH 1/2] selftests/openat2: fix clang build failures: -static-libasan, LOCAL_HDRS

[John Hubbard]

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.

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/20240503030214.86681-2-jhubbard@nvidia.com/

thanks,
John Hubbard

> 
> [1]
> https://lore.kernel.org/linux-kselftest/20240417160740.2019530-1-ryan.roberts@arm.com/
> 
> 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,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/2] selftests/openat2: fix clang build failures: -static-libasan, LOCAL_HDRS

[Ryan Roberts]

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 <ryan.roberts@arm.com>

> 
> 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/20240503030214.86681-2-jhubbard@nvidia.com/
> 
> thanks,
> John Hubbard
> 
>>
>> [1]
>> https://lore.kernel.org/linux-kselftest/20240417160740.2019530-1-ryan.roberts@arm.com/
>>
>> 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,

Re: [PATCH 1/2] selftests/openat2: fix clang build failures: -static-libasan, LOCAL_HDRS

[John Hubbard]

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 <ryan.roberts@arm.com>
> 

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/20240503030214.86681-2-jhubbard@nvidia.com/
>>
>> thanks,
>> John Hubbard
>>
>>>
>>> [1]
>>> https://lore.kernel.org/linux-kselftest/20240417160740.2019530-1-ryan.roberts@arm.com/
>>>
>>> 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,
> 

Re: [PATCH 1/2] selftests/openat2: fix clang build failures: -static-libasan, LOCAL_HDRS

[Ryan Roberts]

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 <ryan.roberts@arm.com>
>>
> 
> Thanks for the review!
> 
> thanks,

Re: [PATCH 1/2] selftests/openat2: fix clang build failures: -static-libasan, LOCAL_HDRS

[Ryan Roberts]

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 <ryan.roberts@arm.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  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/202404141807.LgsqXPY5-lkp@intel.com/
[2]
https://lore.kernel.org/linux-kselftest/20240417160740.2019530-1-ryan.roberts@arm.com/


> +
> +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

Re: [PATCH 1/2] selftests/openat2: fix clang build failures: -static-libasan, LOCAL_HDRS

[John Hubbard]

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):

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/202404141807.LgsqXPY5-lkp@intel.com/
> [2]
> https://lore.kernel.org/linux-kselftest/20240417160740.2019530-1-ryan.roberts@arm.com/
> 
> 
>> +
>> +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,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/2] selftests/openat2: fix clang build failures: -static-libasan, LOCAL_HDRS

[John Hubbard]

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/202404141807.LgsqXPY5-lkp@intel.com/
>> [2]
>> https://lore.kernel.org/linux-kselftest/20240417160740.2019530-1-ryan.roberts@arm.com/
>>
>>
>>> +
>>> +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,

Re: [PATCH 1/2] selftests/openat2: fix clang build failures: -static-libasan, LOCAL_HDRS

[Ryan Roberts]

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,

Re: [PATCH 1/2] selftests/openat2: fix clang build failures: -static-libasan, LOCAL_HDRS

[Muhammad Usama Anjum]

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/202404141807.LgsqXPY5-lkp@intel.com/
>> [2]
>> https://lore.kernel.org/linux-kselftest/20240417160740.2019530-1-ryan.roberts@arm.com/
>>
>>
>>> +
>>> +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

Re: [PATCH 1/2] selftests/openat2: fix clang build failures: -static-libasan, LOCAL_HDRS

[John Hubbard]

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/20240529020842.127275-1-jhubbard@nvidia.com

thanks,
-- 
John Hubbard
NVIDIA