On Sun 07 Aug 2022 09:48:09 +0900 Masahiro Yamada wrote:
> When receiving some signal, GNU Make automatically deletes the target if
> it has already been changed by the interrupted recipe.
>
> If the target is possibly incomplete due to interruption, it must be
> deleted so that it will be remade from scratch on the next run of make.
> Otherwise, the target would remain corrupted permanently because its
> timestamp had already been updated.
>
> Thanks to this behavior of Make, you can stop the build any time by
> pressing Ctrl-C, and just run 'make' to resume it.
>
> Kbuild also relies on this feature, but it is equivalently important
> for any build systems that make decisions based on timestamps (if you
> want to support stop/resume reliably).
>
> However, this does not always work as claimed; Make immediately dies
> with Ctrl-C if its stderr goes into a pipe.
>
> [Test Makefile]
>
> foo:
> echo hello > $@
> sleep 3
> echo world >> $@
>
> [Test Result]
>
> $ make # hit Ctrl-C
> echo hello > foo
> sleep 3
> ^Cmake: *** Deleting file 'foo'
> make: *** [Makefile:3: foo] Interrupt
>
> $ make 2>&1 | cat # hit Ctrl-C
> echo hello > foo
> sleep 3
> ^C$ # 'foo' is often left-over
>
> The reason is because SIGINT is sent to the entire process group.
> In this example, SIGINT kills 'cat', and 'make' writes the message to
> the closed pipe, then dies with SIGPIPE.
>
> A typical bad scenario (as reported by [1], [2]) is to save build log
> by using the 'tee' command:
>
> $ make 2>&1 | tee log
>
> Again, this can be problematic for any build systems based on Make, so
> I hope it will be fixed in GNU Make. The maintainer of GNU Make stated
> this is a long-standing issue and difficult to fix [3]. It has not been
> fixed yet as of writing.
>
> So, we cannot rely on Make cleaning the target. We can do it by
> ourselves, in signal traps.
>
> As far as I understand, Make takes care of SIGHUP, SIGINT, SIGQUIT, and
> SITERM for the target removal. I added the traps for them, and also for
> SIGPIPE just in case cmd_* rule prints something to stdout or stderr
> (but I did not observe an actual case where SIGPIPE was triggered).
>
> [Note 1]
>
> The trap handler might be worth explaining.
>
> rm -f $@; trap - $(sig); kill -s $(sig) $$
>
> This lets the shell kill itself by the signal it caught, so the parent
> process can tell the child has exited on the signal. Generally, this is
> a proper manner for handling signals, in case the calling program (like
> Bash) may monitor WIFSIGNALED() and WTERMSIG() for WCE (Wait and
> Cooperative Exit) [4] although this may not be a big deal here because
> GNU Make handles SIGHUP, SIGINT, SIGQUIT in WUE (Wait and Unconditional
> Exit) and SIGTERM in IUE (Immediate Unconditional Exit).
>
> [Note 2]
>
> Reverting 392885ee82d3 ("kbuild: let fixdep directly write to .*.cmd
> files") would directly address [1], but it only saves if_changed_dep.
> As reported in [2], all commands that use redirection can potentially
> leave an empty (i.e. broken) target.
>
> [Note 3]
>
> Another (even safer) approach might be to always write to a temporary
> file, and rename it to $@ at the end of the recipe.
>
> <command> > $(tmp-target)
> mv $(tmp-target) $@
>
> It would require a lot of Makefile changes, and result in ugly code,
> so I did not take it.
>
> [Note 4]
>
> A little more thoughts about a pattern rule with multiple targets (or
> a grouped target).
>
> %.x %.y: %.z
> <recipe>
>
> When interrupted, GNU Make deletes both %.x and %.y, while this solution
> only deletes $@. Probably, this is not a big deal. The next run of make
> will execute the rule again to create $@ along with the other files.
>
> [1]: https://lore.kernel.org/all/YLeot94yAaM4xbMY@xxxxxxxxx/
> [2]: https://lore.kernel.org/all/20220510221333.2770571-1-robh@xxxxxxxxxx/
> [3]: https://lists.gnu.org/archive/html/help-make/2021-06/msg00001.html
> [4]: https://www.cons.org/cracauer/sigint.html
>
> Reported-by: Ingo Molnar <mingo@xxxxxxxxxx>
> Reported-by: Rob Herring <robh@xxxxxxxxxx>
> Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> ---
>
> If you are happy to help test this patch, that will be appreciated.
>
> Without applying this patch,
>
> $ make -j<nr-proc> 2>&1 | tee log
>
> Then, you will see an error reported in [1].
> You may need to repeat it dozen of times to reproduce it.
> The more CPU cores you have, the easier you will get the error.
>
> Apply this patch, and repeat the same.
> You will no longer see that error (hopefully).
>
>
> scripts/Kbuild.include | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> index ece44b735061..9432a7f33186 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -100,8 +100,29 @@ echo-cmd = $(if $($(quiet)cmd_$(1)),\
> quiet_redirect :=
> silent_redirect := exec >/dev/null;
>
> +# Delete the target on interruption
> +#
> +# GNU Make automatically deletes the target if it has already been changed by
> +# the interrupted recipe. So, you can safely stop the build by Ctrl-C (Make
> +# will delete incomplete targets), and resume it later.
> +#
> +# However, this does not work when the stderr is piped to another program, like
> +# $ make >&2 | tee log
> +# Make dies with SIGPIPE before cleaning the targets.
> +#
> +# To address it, we cleans the target in signal traps.
s/cleans/clean/
> +#
> +# Make deletes the target when it catches SIGHUP, SIGINT, SIGQUIT, SIGTERM.
> +# So, we cover them, and also SIGPIPE just in case.
> +#
> +# Of course, this is unneeded for phony targets.
> +delete-on-interrupt = \
> + $(if $(filter-out $(PHONY), $@), \
> + $(foreach sig, HUP INT QUIT TERM PIPE, \
> + trap 'rm -f $@; trap - $(sig); kill -s $(sig) $$$$' $(sig);))
> +
> # printing commands
> -cmd = @set -e; $(echo-cmd) $($(quiet)redirect) $(cmd_$(1))
> +cmd = @set -e; $(echo-cmd) $($(quiet)redirect) $(delete-on-interrupt) $(cmd_$(1))
>
> ###
> # if_changed - execute command if any prerequisite is newer than
> --
> 2.34.1
Thanks for the patch and the verbose reasoning. I would like to see
stable@k.o added if you think it is appropriate (patch applies cleanly
to 5.4, 5.15).
Reviewed-by: Nicolas Schier <nicolas@xxxxxxxxx>
Kind regards,
Nicolas
