[PATCH] kbuild: fix if_change and friends to consider argument order

[PATCH] kbuild: fix if_change and friends to consider argument order

[Masahiro Yamada]

Currently, arg-check is implemented as follows:

  arg-check = $(strip $(filter-out $(cmd_$(1)), $(cmd_$@)) \
                      $(filter-out $(cmd_$@),   $(cmd_$(1))) )

This does not care about the order of arguments that appear in
$(cmd_$(1)) and $(cmd_$@).  So, if_changed and friends never rebuild
the target if only the argument order is changed.  This is a problem
when the link order is changed.

Apparently,

  obj-y += foo.o
  obj-y += bar.o

and

  obj-y += bar.o
  obj-y += foo.o

should be distinguished because the link order determines the probe
order of drivers.  So, built-in.o should be rebuilt if the order of
objects is changed.

This commit fixes arg-check to compare two strings as a whole.
$(strip ...) is important because we want to ignore the difference
that comes from white-spaces.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 scripts/Kbuild.include | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index b2ab2a9..2d03480 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -228,8 +228,8 @@ objectify = $(foreach o,$(1),$(if $(filter /%,$(o)),$(o),$(obj)/$(o)))
 ifneq ($(KBUILD_NOCMDDEP),1)
 # Check if both arguments has same arguments. Result is empty string if equal.
 # User may override this check using make KBUILD_NOCMDDEP=1
-arg-check = $(strip $(filter-out $(cmd_$(1)), $(cmd_$@)) \
-                    $(filter-out $(cmd_$@),   $(cmd_$(1))) )
+arg-check = $(filter-out $(quote)$(strip $(cmd_$1))$(quote), \
+                         $(quote)$(strip $(cmd_$@))$(quote))
 else
 arg-check = $(if $(strip $(cmd_$@)),,1)
 endif
-- 
1.9.1

Re: [PATCH] kbuild: fix if_change and friends to consider argument order

[Woodhouse, David]

[-- Attachment #1: Type: text/plain, Size: 895 bytes --]

On Thu, 2016-05-05 at 16:45 +0900, Masahiro Yamada wrote:
> 
> This commit fixes arg-check to compare two strings as a whole.
> $(strip ...) is important because we want to ignore the difference
> that comes from white-spaces.

Do we?

I can construct a hypothetical situation in which whitespace differs
and we *do* want it to make a difference (for example I used to sign
with a key called 'My Signing Key.pem' and now I've changed to use
'My  Signing  Key.pem'. (OK, it's a *stupid* example but still...)

I couldn't come up with the converse — where whitespace does change for
some reason, but we really don't want to rebuild.

Should we err on the side of caution, and let whitespace changes
trigger a rebuild?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3437 bytes --]

Re: [PATCH] kbuild: fix if_change and friends to consider argument order

[Masahiro Yamada]

2016-05-05 17:08 GMT+09:00 Woodhouse, David <david.woodhouse@intel.com>:
> On Thu, 2016-05-05 at 16:45 +0900, Masahiro Yamada wrote:
>>
>> This commit fixes arg-check to compare two strings as a whole.
>> $(strip ...) is important because we want to ignore the difference
>> that comes from white-spaces.
>
> Do we?
>
> I can construct a hypothetical situation in which whitespace differs
> and we *do* want it to make a difference (for example I used to sign
> with a key called 'My Signing Key.pem' and now I've changed to use
> 'My  Signing  Key.pem'. (OK, it's a *stupid* example but still...)
>
> I couldn't come up with the converse — where whitespace does change for
> some reason, but we really don't want to rebuild.
>
> Should we err on the side of caution, and let whitespace changes
> trigger a rebuild?
>
> --
> David Woodhouse                            Open Source Technology Centre
> David.Woodhouse@intel.com                              Intel Corporation
>



Please hold on.

I noticed some side effect on this patch.

I need to test it more carefully.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: fix if_change and friends to consider argument order

[Masahiro Yamada]

2016-05-05 23:49 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> 2016-05-05 17:08 GMT+09:00 Woodhouse, David <david.woodhouse@intel.com>:
>> On Thu, 2016-05-05 at 16:45 +0900, Masahiro Yamada wrote:
>>>
>>> This commit fixes arg-check to compare two strings as a whole.
>>> $(strip ...) is important because we want to ignore the difference
>>> that comes from white-spaces.
>>
>> Do we?
>>
>> I can construct a hypothetical situation in which whitespace differs
>> and we *do* want it to make a difference (for example I used to sign
>> with a key called 'My Signing Key.pem' and now I've changed to use
>> 'My  Signing  Key.pem'. (OK, it's a *stupid* example but still...)
>>
>> I couldn't come up with the converse — where whitespace does change for
>> some reason, but we really don't want to rebuild.
>>
>> Should we err on the side of caution, and let whitespace changes
>> trigger a rebuild?
>>
>> --
>> David Woodhouse                            Open Source Technology Centre
>> David.Woodhouse@intel.com                              Intel Corporation
>>
>
>
>
> Please hold on.
>
> I noticed some side effect on this patch.
>
> I need to test it more carefully.


This patch is not working at all.
Please disregard it.


Probably, I will send v2 in a few days.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: fix if_change and friends to consider argument order

[Masahiro Yamada]

Hi David,


2016-05-05 17:08 GMT+09:00 Woodhouse, David <david.woodhouse@intel.com>:
> On Thu, 2016-05-05 at 16:45 +0900, Masahiro Yamada wrote:
>>
>> This commit fixes arg-check to compare two strings as a whole.
>> $(strip ...) is important because we want to ignore the difference
>> that comes from white-spaces.
>
> Do we?
>
> I can construct a hypothetical situation in which whitespace differs
> and we *do* want it to make a difference (for example I used to sign
> with a key called 'My Signing Key.pem' and now I've changed to use
> 'My  Signing  Key.pem'. (OK, it's a *stupid* example but still...)
>

Have you ever succeeded in passing such a string in Kbuild in the first place?

For example, I added the following line into init/Makefile

CFLAGS_main.o += -DHELLO_WORLD='"hello        world!"'


and

     printk("%s\n", HELLO_WORLD);

to start_kernel().



But, I got the console log

[     0.001639] hello world!




The root cause of this problem is the following line

_c_flags       = $(filter-out $(CFLAGS_REMOVE_$(basetarget).o), $(orig_c_flags))


$(filter-out  ) strips extra spaces, so Kbuild can not keep
white-spaces as they are.


Maybe, we can fix this problem.  (This is another problem, though)

Thank you for spotting this.



-- 
Best Regards
Masahiro Yamada