[PATCH 1/2] kbuild: rename cmd_$@ to savedcmd_$@ in *.cmd files

[PATCH 1/2] kbuild: rename cmd_$@ to savedcmd_$@ in *.cmd files

[Masahiro Yamada]

The cmd-check macro compares $(cmd_$@) and $(cmd_$1), but a pitfall is
that you cannot use cmd_<target> as the variable name for the command.

For example, the following code will not work in the top Makefile
or ./Kbuild.

    quiet_cmd_foo = GEN     $@
          cmd_foo = touch $@

    targets += foo
    foo: FORCE
            $(call if_changed,foo)

In this case, both $@ and $1 are expanded to 'foo', so $(cmd_check)
is always empty.

We do not need to use the same prefix for cmd_$@ and cmd_$1.
Rename the former to savedcmd_$@.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/Kbuild.include                      | 8 ++++----
 scripts/Makefile.modfinal                   | 2 +-
 scripts/basic/fixdep.c                      | 4 ++--
 scripts/clang-tools/gen_compile_commands.py | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 10cf8d2d82ef..1a7514f49089 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -170,10 +170,10 @@ cmd = @$(if $(cmd_$(1)),set -e; $($(quiet)log_print) $(delete-on-interrupt) $(cm
 ifneq ($(KBUILD_NOCMDDEP),1)
 # Check if both commands are the same including their order. Result is empty
 # string if equal. User may override this check using make KBUILD_NOCMDDEP=1
-cmd-check = $(filter-out $(subst $(space),$(space_escape),$(strip $(cmd_$@))), \
+cmd-check = $(filter-out $(subst $(space),$(space_escape),$(strip $(savedcmd_$@))), \
                          $(subst $(space),$(space_escape),$(strip $(cmd_$1))))
 else
-cmd-check = $(if $(strip $(cmd_$@)),,1)
+cmd-check = $(if $(strip $(savedcmd_$@)),,1)
 endif
 
 # Replace >$< with >$$< to preserve $ when reloading the .cmd file
@@ -199,7 +199,7 @@ if_changed = $(if $(if-changed-cond),$(cmd_and_savecmd),@:)
 
 cmd_and_savecmd =                                                            \
 	$(cmd);                                                              \
-	printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd
+	printf '%s\n' 'savedcmd_$@ := $(make-cmd)' > $(dot-target).cmd
 
 # Execute the command and also postprocess generated .d dependencies file.
 if_changed_dep = $(if $(if-changed-cond),$(cmd_and_fixdep),@:)
@@ -239,7 +239,7 @@ _why =                                                                        \
         $(if $(wildcard $@),                                                 \
             $(if $(newer-prereqs),- due to: $(newer-prereqs),                \
                 $(if $(cmd-check),                                           \
-                    $(if $(cmd_$@),- due to command line change,             \
+                    $(if $(savedcmd_$@),- due to command line change,        \
                         $(if $(filter $@, $(targets)),                       \
                             - due to missing .cmd file,                      \
                             - due to $(notdir $@) not in $$(targets)         \
diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal
index a30d5b08eee9..4703f652c009 100644
--- a/scripts/Makefile.modfinal
+++ b/scripts/Makefile.modfinal
@@ -54,7 +54,7 @@ newer_prereqs_except = $(filter-out $(PHONY) $(1),$?)
 # Same as if_changed, but allows to exclude specified extra dependencies
 if_changed_except = $(if $(call newer_prereqs_except,$(2))$(cmd-check),      \
 	$(cmd);                                                              \
-	printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd, @:)
+	printf '%s\n' 'savedcmd_$@ := $(make-cmd)' > $(dot-target).cmd, @:)
 
 # Re-generate module BTFs if either module's .ko or vmlinux changed
 %.ko: %.o %.mod.o scripts/module.lds $(and $(CONFIG_DEBUG_INFO_BTF_MODULES),$(KBUILD_BUILTIN),vmlinux) FORCE
diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index 2328f9a641da..7a408bb97478 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -70,7 +70,7 @@
  *
  * It first generates a line
  *
- *   cmd_<target> = <cmdline>
+ *   savedcmd_<target> = <cmdline>
  *
  * and then basically copies the .<target>.d file to stdout, in the
  * process filtering out the dependency on autoconf.h and adding
@@ -344,7 +344,7 @@ int main(int argc, char *argv[])
 	target = argv[2];
 	cmdline = argv[3];
 
-	printf("cmd_%s := %s\n\n", target, cmdline);
+	printf("savedcmd_%s := %s\n\n", target, cmdline);
 
 	buf = read_file(depfile);
 	parse_dep_file(buf, target);
diff --git a/scripts/clang-tools/gen_compile_commands.py b/scripts/clang-tools/gen_compile_commands.py
index 0227522959a4..15ba56527acd 100755
--- a/scripts/clang-tools/gen_compile_commands.py
+++ b/scripts/clang-tools/gen_compile_commands.py
@@ -19,7 +19,7 @@ _DEFAULT_OUTPUT = 'compile_commands.json'
 _DEFAULT_LOG_LEVEL = 'WARNING'
 
 _FILENAME_PATTERN = r'^\..*\.cmd$'
-_LINE_PATTERN = r'^cmd_[^ ]*\.o := (.* )([^ ]*\.c) *(;|$)'
+_LINE_PATTERN = r'^savedcmd_[^ ]*\.o := (.* )([^ ]*\.c) *(;|$)'
 _VALID_LOG_LEVELS = ['DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL']
 # The tools/ directory adopts a different build system, and produces .cmd
 # files in a different format. Do not support it.
-- 
2.34.1

[PATCH 2/2] kbuild: add more comments for KBUILD_NOCMDDEP=1

[Masahiro Yamada]

The cmd-check for KBUILD_NOCMDDEP=1 may not be clear until you see
commit c4d5ee13984f ("kbuild: make KBUILD_NOCMDDEP=1 handle empty
built-in.o").

When a phony target (i.e. FORCE) is the only prerequisite, Kbuild
uses a tricky way to detect that the target does not exist.

Add more comments.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/Kbuild.include | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 1a7514f49089..4648ab8f11d4 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -170,9 +170,13 @@ cmd = @$(if $(cmd_$(1)),set -e; $($(quiet)log_print) $(delete-on-interrupt) $(cm
 ifneq ($(KBUILD_NOCMDDEP),1)
 # Check if both commands are the same including their order. Result is empty
 # string if equal. User may override this check using make KBUILD_NOCMDDEP=1
+# If the target does not exist, the *.cmd file should not be included so
+# $(savedcmd_$@) gets empty. Then, target will be built even if $(newer-prereqs)
+# happens to become empty.
 cmd-check = $(filter-out $(subst $(space),$(space_escape),$(strip $(savedcmd_$@))), \
                          $(subst $(space),$(space_escape),$(strip $(cmd_$1))))
 else
+# We still need to detect missing targets.
 cmd-check = $(if $(strip $(savedcmd_$@)),,1)
 endif
 
@@ -186,6 +190,8 @@ make-cmd = $(call escsq,$(subst $(pound),$$(pound),$(subst $$,$$$$,$(cmd_$(1))))
 
 # Find any prerequisites that are newer than target or that do not exist.
 # PHONY targets skipped in both cases.
+# If there is no prerequisite other than phony targets, $(newer-prereqs) becomes
+# empty even if the target does not exist. cmd-check saves this corner case.
 newer-prereqs = $(filter-out $(PHONY),$?)
 
 # It is a typical mistake to forget the FORCE prerequisite. Check it here so
-- 
2.34.1

Re: [PATCH 1/2] kbuild: rename cmd_$@ to savedcmd_$@ in *.cmd files

[Nicolas Schier]

[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]

On Thu, Dec 29, 2022 at 06:15:00PM +0900 Masahiro Yamada wrote:
> The cmd-check macro compares $(cmd_$@) and $(cmd_$1), but a pitfall is
> that you cannot use cmd_<target> as the variable name for the command.
> 
> For example, the following code will not work in the top Makefile
> or ./Kbuild.
> 
>     quiet_cmd_foo = GEN     $@
>           cmd_foo = touch $@
> 
>     targets += foo
>     foo: FORCE
>             $(call if_changed,foo)
> 
> In this case, both $@ and $1 are expanded to 'foo', so $(cmd_check)
> is always empty.
> 
> We do not need to use the same prefix for cmd_$@ and cmd_$1.
> Rename the former to savedcmd_$@.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  scripts/Kbuild.include                      | 8 ++++----
>  scripts/Makefile.modfinal                   | 2 +-
>  scripts/basic/fixdep.c                      | 4 ++--
>  scripts/clang-tools/gen_compile_commands.py | 2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 

Thanks for fixing this, I wasn't aware of that at all.

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>

-- 
epost|xmpp: nicolas@fjasle.eu          irc://oftc.net/nsc
↳ gpg: 18ed 52db e34f 860e e9fb  c82b 7d97 0932 55a0 ce7f
     -- frykten for herren er opphav til kunnskap --

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] kbuild: add more comments for KBUILD_NOCMDDEP=1

[Nicolas Schier]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

On Thu, Dec 29, 2022 at 06:15:01PM +0900 Masahiro Yamada wrote:
> The cmd-check for KBUILD_NOCMDDEP=1 may not be clear until you see
> commit c4d5ee13984f ("kbuild: make KBUILD_NOCMDDEP=1 handle empty
> built-in.o").
> 
> When a phony target (i.e. FORCE) is the only prerequisite, Kbuild
> uses a tricky way to detect that the target does not exist.
> 
> Add more comments.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  scripts/Kbuild.include | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>


-- 
epost|xmpp: nicolas@fjasle.eu          irc://oftc.net/nsc
↳ gpg: 18ed 52db e34f 860e e9fb  c82b 7d97 0932 55a0 ce7f
     -- frykten for herren er opphav til kunnskap --

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]