linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] kbuild: fix if_change and friends to consider argument order
@ 2016-05-05  7:45 Masahiro Yamada
  2016-05-05  8:08 ` Woodhouse, David
  0 siblings, 1 reply; 5+ messages in thread
From: Masahiro Yamada @ 2016-05-05  7:45 UTC (permalink / raw)
  To: linux-kbuild
  Cc: Masahiro Yamada, Michal Marek, David Howells, David Woodhouse,
	linux-kernel

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] kbuild: fix if_change and friends to consider argument order
  2016-05-05  7:45 [PATCH] kbuild: fix if_change and friends to consider argument order Masahiro Yamada
@ 2016-05-05  8:08 ` Woodhouse, David
  2016-05-05 14:49   ` Masahiro Yamada
  2016-05-06  2:12   ` Masahiro Yamada
  0 siblings, 2 replies; 5+ messages in thread
From: Woodhouse, David @ 2016-05-05  8:08 UTC (permalink / raw)
  To: linux-kbuild, yamada.masahiro; +Cc: mmarek, linux-kernel, dhowells

[-- 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 --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] kbuild: fix if_change and friends to consider argument order
  2016-05-05  8:08 ` Woodhouse, David
@ 2016-05-05 14:49   ` Masahiro Yamada
  2016-05-05 18:00     ` Masahiro Yamada
  2016-05-06  2:12   ` Masahiro Yamada
  1 sibling, 1 reply; 5+ messages in thread
From: Masahiro Yamada @ 2016-05-05 14:49 UTC (permalink / raw)
  To: Woodhouse, David; +Cc: linux-kbuild, mmarek, linux-kernel, dhowells

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] kbuild: fix if_change and friends to consider argument order
  2016-05-05 14:49   ` Masahiro Yamada
@ 2016-05-05 18:00     ` Masahiro Yamada
  0 siblings, 0 replies; 5+ messages in thread
From: Masahiro Yamada @ 2016-05-05 18:00 UTC (permalink / raw)
  To: Woodhouse, David; +Cc: linux-kbuild, mmarek, linux-kernel, dhowells

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] kbuild: fix if_change and friends to consider argument order
  2016-05-05  8:08 ` Woodhouse, David
  2016-05-05 14:49   ` Masahiro Yamada
@ 2016-05-06  2:12   ` Masahiro Yamada
  1 sibling, 0 replies; 5+ messages in thread
From: Masahiro Yamada @ 2016-05-06  2:12 UTC (permalink / raw)
  To: Woodhouse, David; +Cc: linux-kbuild, mmarek, linux-kernel, dhowells

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-05-06  2:12 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-05  7:45 [PATCH] kbuild: fix if_change and friends to consider argument order Masahiro Yamada
2016-05-05  8:08 ` Woodhouse, David
2016-05-05 14:49   ` Masahiro Yamada
2016-05-05 18:00     ` Masahiro Yamada
2016-05-06  2:12   ` Masahiro Yamada

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).