Re: [RFC PATCH] kbuild: call checksyscalls.sh and check-atomics.sh only if prerequisites change

[Vincent MAILHOL <mailhol.vincent@wanadoo.fr>]

On Sat. 30 Apr 2022 at 02:11, Masahiro Yamada <masahiroy@kernel.org> wrote:
> On Wed, Apr 27, 2022 at 12:52 AM Vincent Mailhol
> <mailhol.vincent@wanadoo.fr> wrote:
> >
> > Currently, checksyscalls.sh and check-atomics.sh are executed
> > unconditionally. Most developers will not modify the files being
> > checked by those scripts and thus do not need to execute these again
> > for each iterative make. Change Kbuild target so that those two
> > scripts get executed only if the prerequisite are modified.
> >
> > In order to implement this we:
> >
> >   1. use the if_change macro instead of cmd. c.f. [1]
> >
> >   2. create two dot files: scripts/.checksyscalls and
> >   scripts/atomic/.check-atomics to keep track of whether the script
> >   were already executed or not. Otherwise, the prerequisite would
> >   always be considered as newer than the target (c.f. output "due to
> >   target missing" of make V=2).
> >
> >   3. modify the CLEAN_FILES target of the root Makefile to removed the
> >   two temporary dot files created in 2.
> >
> > We also added an additional dependency to include/linux/atomic/* for
> > check-atomics.sh to make sure that the script gets executed again if
> > the header are modified. check-atomics.sh already has a dependency
> > toward include/generated/asm-offsets.h and so no additional
> > dependencies were added.
> >
> > [1] https://www.kernel.org/doc/html/latest/kbuild/makefiles.html#command-change-detection
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> > Sending this as RFC because I am not an expert of Kbuild. The use of
> > the dot files was my best shot at tackling this issue. Maybe there is
> > a smarter way which I just missed?
> >
> > If I receive no comments for the next two weeks, I will resend this
> > patch without the RFC tag.
> > ---
> >  Kbuild   | 14 ++++++++------
> >  Makefile |  3 ++-
> >  2 files changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/Kbuild b/Kbuild
> > index fa441b98c9f6..d579f4971aa3 100644
> > --- a/Kbuild
> > +++ b/Kbuild
> > @@ -39,21 +39,23 @@ $(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s FORCE
> >  #####
> >  # Check for missing system calls
> >
> > -always-y += missing-syscalls
> > +always-y += scripts/.missing-syscalls
> >
> >  quiet_cmd_syscalls = CALL    $<
> >        cmd_syscalls = $(CONFIG_SHELL) $< $(CC) $(c_flags) $(missing_syscalls_flags)
> >
> > -missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE
> > -       $(call cmd,syscalls)
> > +scripts/.missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE
> > +       $(call if_changed,syscalls)
> > +       @touch $@
>
> I am not sure about this hunk.
>
> Avoiding the needless preprocess is good.
> But, it is better to display a warning somewhere (maybe 'all' target)
> until the required syscall is implemented.

The classic way to achieve is to raise an error, not a warning.

This should have been achievable through CONFIG_WERROR, however,
the below commit deactivated it with -Wno-error (rationale of why
this was an issue is missing).

commit 20fbb11fe4ea ("don't make the syscall checking produce errors
from warnings")

If we just warn, I am not sure how to emit a warning each time
until it gets fixed. The normal behaviour everywhere else is to
only warn during the first build and be quiet (unless
dependencies change).

> Also, you need to check the timestamp of syscall_32.tbl.
> When it is updated (i.e. when a new syscall is added),
> this check must be re-run.

ACK

Regardless of the above discussion, I think I will give up on
checksyscalls.sh, at least for the moment. The patch assumed that
checksyscalls.sh would only be called from ./Kbuild. It appears
that there is another user: arch/mips/Makefile c.f. kernel test
robot's report:
https://lore.kernel.org/llvm/202205030015.JCmg5yPS-lkp@intel.com/

Because of that, only creating a single empty file to check the
timestamp is insufficient and I could not find a smart way to
manage all this.

> >  #####
> >  # Check atomic headers are up-to-date
> >
> > -always-y += old-atomics
> > +always-y += scripts/atomic/.old-atomics
> >
> >  quiet_cmd_atomics = CALL    $<
> >        cmd_atomics = $(CONFIG_SHELL) $<
> >
> > -old-atomics: scripts/atomic/check-atomics.sh FORCE
> > -       $(call cmd,atomics)
> > +scripts/atomic/.old-atomics: scripts/atomic/check-atomics.sh $(wildcard include/linux/atomic/*) FORCE
> > +       $(call if_changed,atomics)
> > +       @touch $@
>
>
> Presumably, this is wrong.
> If the header is manually edited, Kbuild must stop the build.

Currently (i.e. before my patch), Kbuild does not stop the build either.
c.f. check-atomics.sh which returns 0 inconditionally:
https://elixir.bootlin.com/linux/v5.17/source/scripts/atomic/check-atomics.sh#L33

This can be easily remediated by making check-atomics.sh return
an error code if the check does not succeed.

> This change just lets it keep going, and
> what is worse, the warning is completely silenced
> second time.

Same comment as before, I expect warnings to only be raised once,
making this an error would solve the issue.

I will send a v2 but only for check-atomics.sh


Yours sincerely,
Vincent Mailhol