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@xxxxxxx> > > 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,
