checkpatch: if_changed: check for multiple calls in targets
diff mbox

Message ID 20180716123902.5337-1-dirk@gouders.net
State New
Headers show

Commit Message

Dirk Gouders July 16, 2018, 12:39 p.m. UTC
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(+)

Comments

Joe Perches July 16, 2018, 3:23 p.m. UTC | #1
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'

--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dirk Gouders July 17, 2018, 9:32 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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*\"/) ||