* [PATCH] checkpatch: if_changed: check for multiple calls in targets @ 2018-07-16 12:39 Dirk Gouders 2018-07-16 15:23 ` Joe Perches 0 siblings, 1 reply; 8+ messages in thread From: Dirk Gouders @ 2018-07-16 12:39 UTC (permalink / raw) To: Andy Whitcroft, Joe Perches Cc: Dirk Gouders, Masahiro Yamada, linux-kbuild, linux-kernel Because the kbuild function if_changed writes the command line to a .cmd file for later tests, multiple calls of that function within a target would result in overwrites of previous values and effectively render the command line test meaningless, resulting in flip-flop behaviour. Produce an error for targets with multiple calls to if_changed. Three examples that would now be detected: 98f78525371b55ccd (x86/boot: Refuse to build with data relocations) 6a8dfe1cac5c591ae (microblaze: support U-BOOT image format) 684151a75bf25f5ae (sparc32: added U-Boot build target: uImage) Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com> Signed-off-by: Dirk Gouders <dirk@gouders.net> --- scripts/checkpatch.pl | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 447857ffaf6b..b0aadf23148e 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2911,6 +2911,14 @@ sub process { "Use of $flag is deprecated, please use \`$replacement->{$flag} instead.\n" . $herecurr) if ($replacement->{$flag}); } + # Check for multiple calls of if_changed within a target in Makefiles + if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) && + ($prevline =~ /^[ +]\t\$\(call if_changed,/) && + ($line =~ /^[ +]\t\$\(call if_changed,/)) { + ERROR("MULTIPLE_IF_CHANGED", + "Multiple calls of if_changed within a target.\n" . $herecurr); + } + # check for DT compatible documentation if (defined $root && (($realfile =~ /\.dtsi?$/ && $line =~ /^\+\s*compatible\s*=\s*\"/) || -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] checkpatch: if_changed: check for multiple calls in targets 2018-07-16 12:39 [PATCH] checkpatch: if_changed: check for multiple calls in targets Dirk Gouders @ 2018-07-16 15:23 ` Joe Perches 2018-07-17 9:32 ` Dirk Gouders 0 siblings, 1 reply; 8+ messages in thread From: Joe Perches @ 2018-07-16 15:23 UTC (permalink / raw) To: Dirk Gouders, Andy Whitcroft; +Cc: Masahiro Yamada, linux-kbuild, linux-kernel On Mon, 2018-07-16 at 14:39 +0200, Dirk Gouders wrote: > Because the kbuild function if_changed writes the command line to a > .cmd file for later tests, multiple calls of that function within a > target would result in overwrites of previous values and effectively > render the command line test meaningless, resulting in flip-flop > behaviour. > > Produce an error for targets with multiple calls to if_changed. Hi. Some questions: How is the existing use in arch/microblaze/boot/Makefile incorrect? $(obj)/simpleImage.%: vmlinux FORCE $(call if_changed,cp,.unstrip) $(call if_changed,objcopy) $(call if_changed,uimage) $(call if_changed,strip,.strip) @echo 'Kernel: $(UIMAGE_OUT) is ready' ' (#'`cat .version`')' > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > @@ -2911,6 +2911,14 @@ sub process { > "Use of $flag is deprecated, please use \`$replacement->{$flag} instead.\n" . $herecurr) if ($replacement->{$flag}); > } > i > + # Check for multiple calls of if_changed within a target in Makefiles > + if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) && Why is any Kbuild file check useful? > + ($prevline =~ /^[ +]\t\$\(call if_changed,/) && > + ($line =~ /^[ +]\t\$\(call if_changed,/)) { What about if_changed_dep and if_changed_rule? > + ERROR("MULTIPLE_IF_CHANGED", > + "Multiple calls of if_changed within a target.\n" . $herecurr); > + } And some more style things: There are instances with multiple tabs so probably these should use '\t*' or '\s*' and not '\t'. This should probably not require a single space after if_changed so likely: 'call\s+if_changed' ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] checkpatch: if_changed: check for multiple calls in targets 2018-07-16 15:23 ` Joe Perches @ 2018-07-17 9:32 ` Dirk Gouders 2018-07-20 7:48 ` [PATCH v2] checkpatch: kbuild: " Dirk Gouders 0 siblings, 1 reply; 8+ messages in thread From: Dirk Gouders @ 2018-07-17 9:32 UTC (permalink / raw) To: Joe Perches; +Cc: Andy Whitcroft, Masahiro Yamada, linux-kbuild, linux-kernel Joe Perches <joe@perches.com> writes: > On Mon, 2018-07-16 at 14:39 +0200, Dirk Gouders wrote: >> Because the kbuild function if_changed writes the command line to a >> .cmd file for later tests, multiple calls of that function within a >> target would result in overwrites of previous values and effectively >> render the command line test meaningless, resulting in flip-flop >> behaviour. >> >> Produce an error for targets with multiple calls to if_changed. > > Hi. Some questions: > > How is the existing use in arch/microblaze/boot/Makefile incorrect? > > $(obj)/simpleImage.%: vmlinux FORCE > $(call if_changed,cp,.unstrip) > $(call if_changed,objcopy) > $(call if_changed,uimage) > $(call if_changed,strip,.strip) > @echo 'Kernel: $(UIMAGE_OUT) is ready' ' (#'`cat .version`')' Hi Joe, thank you for the review. Perhaps I should have started the commit message with: "The kbuild function if_changed should not be called more than once for a target." and then the explanatory text. In case you are interested in the thread where Masahiro explained in detail the real issues with those Makefiles that made them use if_changed too often: https://marc.info/?l=linux-kernel&m=153149389303034&w=2 >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] >> @@ -2911,6 +2911,14 @@ sub process { >> "Use of $flag is deprecated, please use \`$replacement->{$flag} instead.\n" . $herecurr) if ($replacement->{$flag}); >> } >> i >> + # Check for multiple calls of if_changed within a target in Makefiles >> + if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) && > > Why is any Kbuild file check useful? Because Kbuild files are makefiles and if we want to check makefiles they are also meant. >> + ($prevline =~ /^[ +]\t\$\(call if_changed,/) && >> + ($line =~ /^[ +]\t\$\(call if_changed,/)) { > > What about if_changed_dep and if_changed_rule? This patch tries to avoid just one single issue that actually showed up and caused irritation. > >> + ERROR("MULTIPLE_IF_CHANGED", >> + "Multiple calls of if_changed within a target.\n" . $herecurr); >> + } > > And some more style things: > > There are instances with multiple tabs so probably > these should use '\t*' or '\s*' and not '\t'. Oh yes, I missed that. I guess, you mean e.g. arch/x86/purgatory/Makefile: $(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE $(call if_changed,ld) I did not check the log but it seems there was one commit that for some reason used \t\t for the recipes. Anyway, the first TAB is mandatory, because we want to check recipes, only. > This should probably not require a single space after > if_changed so likely: > > 'call\s+if_changed' Treewide, I did not find a single call of if_changed that uses other than a single space but you are right, me might see variations. So, according to your remarks, I would change the pattern to: /^[ +]\t\s*\$\(call\s+if_changed,/ Dirk ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] checkpatch: kbuild: if_changed: check for multiple calls in targets 2018-07-17 9:32 ` Dirk Gouders @ 2018-07-20 7:48 ` Dirk Gouders 2018-07-20 10:06 ` Joe Perches 0 siblings, 1 reply; 8+ messages in thread From: Dirk Gouders @ 2018-07-20 7:48 UTC (permalink / raw) To: Joe Perches Cc: Dirk Gouders, Andy Whitcroft, Masahiro Yamada, linux-kbuild, linux-kernel The kbuild function if_changed should not be called more than once for a target. Because that function writes the command line to a .cmd file for later tests, multiple calls of it within a target would result in overwrites of previous values and effectively render the command line test meaningless, resulting in flip-flop behaviour. Add a check for makefiles and kbuild files and produce an error for targets with multiple calls to if_changed. Three examples that now could be detected: 98f78525371b55ccd (x86/boot: Refuse to build with data relocations) 6a8dfe1cac5c591ae (microblaze: support U-BOOT image format) 684151a75bf25f5ae (sparc32: added U-Boot build target: uImage) Reviewed-by: Joe Perches <joe@perches.com> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com> Signed-off-by: Dirk Gouders <dirk@gouders.net> --- v2: rework commit message and regular expression --- scripts/checkpatch.pl | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 447857ffaf6b..437e98414f74 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2911,6 +2911,14 @@ sub process { "Use of $flag is deprecated, please use \`$replacement->{$flag} instead.\n" . $herecurr) if ($replacement->{$flag}); } + # Check for multiple calls of if_changed within a target in Makefiles + if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) && + ($prevline =~ /^[ +]\t\s*\$\(call\s+if_changed,/) && + ($line =~ /^[ +]\t\s*\$\(call\s+if_changed,/)) { + ERROR("MULTIPLE_IF_CHANGED", + "Multiple calls of if_changed within a target.\n" . $herecurr); + } + # check for DT compatible documentation if (defined $root && (($realfile =~ /\.dtsi?$/ && $line =~ /^\+\s*compatible\s*=\s*\"/) || -- 2.16.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] checkpatch: kbuild: if_changed: check for multiple calls in targets 2018-07-20 7:48 ` [PATCH v2] checkpatch: kbuild: " Dirk Gouders @ 2018-07-20 10:06 ` Joe Perches 2018-07-20 15:21 ` Segher Boessenkool 0 siblings, 1 reply; 8+ messages in thread From: Joe Perches @ 2018-07-20 10:06 UTC (permalink / raw) To: Dirk Gouders; +Cc: Andy Whitcroft, Masahiro Yamada, linux-kbuild, linux-kernel On Fri, 2018-07-20 at 09:48 +0200, Dirk Gouders wrote: > The kbuild function if_changed should not be called more than once for > a target. > > Because that function writes the command line to a .cmd file for later > tests, multiple calls of it within a target would result in overwrites > of previous values and effectively render the command line test > meaningless, resulting in flip-flop behaviour. > > Add a check for makefiles and kbuild files and produce an error for > targets with multiple calls to if_changed. > > Three examples that now could be detected: > > 98f78525371b55ccd (x86/boot: Refuse to build with data relocations) > 6a8dfe1cac5c591ae (microblaze: support U-BOOT image format) > 684151a75bf25f5ae (sparc32: added U-Boot build target: uImage) > > Reviewed-by: Joe Perches <joe@perches.com> I didn't review this. I gave you feedback but didn't add a signature. For anything other than "Suggested-by:", please don't add signatures to patches unless the person gives directly gives you one. > Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com> > Signed-off-by: Dirk Gouders <dirk@gouders.net> > --- > v2: rework commit message and regular expression > --- > scripts/checkpatch.pl | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 447857ffaf6b..437e98414f74 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -2911,6 +2911,14 @@ sub process { > "Use of $flag is deprecated, please use \`$replacement->{$flag} instead.\n" . $herecurr) if ($replacement->{$flag}); > } > > + # Check for multiple calls of if_changed within a target in Makefiles > + if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) && The uses of .* here are superfluous. > + ($prevline =~ /^[ +]\t\s*\$\(call\s+if_changed,/) && > + ($line =~ /^[ +]\t\s*\$\(call\s+if_changed,/)) { > + ERROR("MULTIPLE_IF_CHANGED", > + "Multiple calls of if_changed within a target.\n" . $herecurr); > + } > + > # check for DT compatible documentation > if (defined $root && > (($realfile =~ /\.dtsi?$/ && $line =~ /^\+\s*compatible\s*=\s*\"/) || ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] checkpatch: kbuild: if_changed: check for multiple calls in targets 2018-07-20 10:06 ` Joe Perches @ 2018-07-20 15:21 ` Segher Boessenkool 2018-07-20 15:33 ` Joe Perches 0 siblings, 1 reply; 8+ messages in thread From: Segher Boessenkool @ 2018-07-20 15:21 UTC (permalink / raw) To: Joe Perches Cc: Dirk Gouders, Andy Whitcroft, Masahiro Yamada, linux-kbuild, linux-kernel On Fri, Jul 20, 2018 at 03:06:37AM -0700, Joe Perches wrote: > On Fri, 2018-07-20 at 09:48 +0200, Dirk Gouders wrote: > > + # Check for multiple calls of if_changed within a target in Makefiles > > + if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) && > > The uses of .* here are superfluous. And it looks like you wanted to match this only at the beginning of the string, which would be /^Makefile/ etc. Segher ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] checkpatch: kbuild: if_changed: check for multiple calls in targets 2018-07-20 15:21 ` Segher Boessenkool @ 2018-07-20 15:33 ` Joe Perches 2018-07-20 15:44 ` Segher Boessenkool 0 siblings, 1 reply; 8+ messages in thread From: Joe Perches @ 2018-07-20 15:33 UTC (permalink / raw) To: Segher Boessenkool Cc: Dirk Gouders, Andy Whitcroft, Masahiro Yamada, linux-kbuild, linux-kernel On Fri, 2018-07-20 at 10:21 -0500, Segher Boessenkool wrote: > On Fri, Jul 20, 2018 at 03:06:37AM -0700, Joe Perches wrote: > > On Fri, 2018-07-20 at 09:48 +0200, Dirk Gouders wrote: > > > + # Check for multiple calls of if_changed within a target in Makefiles > > > + if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) && > > > > The uses of .* here are superfluous. > > And it looks like you wanted to match this only at the beginning of the > string, which would be /^Makefile/ etc. Nope. $realfile includes path and /^Makefile/ matches only the top level Makefile and none of the ones in subdirectories. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] checkpatch: kbuild: if_changed: check for multiple calls in targets 2018-07-20 15:33 ` Joe Perches @ 2018-07-20 15:44 ` Segher Boessenkool 0 siblings, 0 replies; 8+ messages in thread From: Segher Boessenkool @ 2018-07-20 15:44 UTC (permalink / raw) To: Joe Perches Cc: Dirk Gouders, Andy Whitcroft, Masahiro Yamada, linux-kbuild, linux-kernel On Fri, Jul 20, 2018 at 08:33:56AM -0700, Joe Perches wrote: > On Fri, 2018-07-20 at 10:21 -0500, Segher Boessenkool wrote: > > On Fri, Jul 20, 2018 at 03:06:37AM -0700, Joe Perches wrote: > > > On Fri, 2018-07-20 at 09:48 +0200, Dirk Gouders wrote: > > > > + # Check for multiple calls of if_changed within a target in Makefiles > > > > + if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) && > > > > > > The uses of .* here are superfluous. > > > > And it looks like you wanted to match this only at the beginning of the > > string, which would be /^Makefile/ etc. > > Nope. > > $realfile includes path and /^Makefile/ matches only the > top level Makefile and none of the ones in subdirectories. Then the .* is more mysterious :-) (Maybe the script should use File::Basename). Segher ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-07-20 15:45 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-16 12:39 [PATCH] checkpatch: if_changed: check for multiple calls in targets Dirk Gouders 2018-07-16 15:23 ` Joe Perches 2018-07-17 9:32 ` Dirk Gouders 2018-07-20 7:48 ` [PATCH v2] checkpatch: kbuild: " Dirk Gouders 2018-07-20 10:06 ` Joe Perches 2018-07-20 15:21 ` Segher Boessenkool 2018-07-20 15:33 ` Joe Perches 2018-07-20 15:44 ` Segher Boessenkool
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).