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