On Sat. 30 Apr 2022 at 02:11, Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote: > On Wed, Apr 27, 2022 at 12:52 AM Vincent Mailhol > <mailhol.vincent@xxxxxxxxxx> 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@xxxxxxxxxx> > > --- > > 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@xxxxxxxxx/ 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
