[PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o

Jan Beulich posted 9 patches 5 years, 4 months ago
Maintainers: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>, Julien Grall <julien@xen.org>, Andrew Cooper <andrew.cooper3@citrix.com>, Jan Beulich <jbeulich@suse.com>, Wei Liu <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>, George Dunlap <george.dunlap@citrix.com>, Stefano Stabellini <sstabellini@kernel.org>, Ian Jackson <iwj@xenproject.org>
There is a newer version of this series
[PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o
Posted by Jan Beulich 5 years, 4 months ago
Switch to $(call if_changed,ld) where possible; presumably not doing so
in e321576f4047 ("xen/build: start using if_changed") right away was an
oversight, as it did for Arm in (just) one case. It failed to add
prelink.o to $(targets), though, causing - judging from the observed
behavior on x86 - undue rebuilds of the final binary (because of
prelink.o getting rebuild for $(cmd_prelink.o) being empty, in turn
because of .prelink.o.cmd not getting read) during "make install-xen".

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/arm/Makefile |  4 +++-
 xen/arch/x86/Makefile | 18 ++++++++++--------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 51173d97127e..296c5e68bbc3 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -95,12 +95,14 @@ prelink_lto.o: $(ALL_OBJS)
 
 # Link it with all the binary objects
 prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
-	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
+	$(call if_changed,ld)
 else
 prelink.o: $(ALL_OBJS) FORCE
 	$(call if_changed,ld)
 endif
 
+targets += prelink.o
+
 $(TARGET)-syms: prelink.o xen.lds
 	$(LD) $(XEN_LDFLAGS) -T xen.lds -N prelink.o \
 	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 74152f2a0dad..9b368632fb43 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -136,19 +136,21 @@ prelink_lto.o: $(ALL_OBJS)
 	$(LD_LTO) -r -o $@ $^
 
 # Link it with all the binary objects
-prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(EFI_OBJS-y)
-	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
+prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(EFI_OBJS-y) FORCE
+	$(call if_changed,ld)
 
-prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
-	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
+prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o FORCE
+	$(call if_changed,ld)
 else
-prelink.o: $(ALL_OBJS) $(EFI_OBJS-y)
-	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
+prelink.o: $(ALL_OBJS) $(EFI_OBJS-y) FORCE
+	$(call if_changed,ld)
 
-prelink-efi.o: $(ALL_OBJS)
-	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
+prelink-efi.o: $(ALL_OBJS) FORCE
+	$(call if_changed,ld)
 endif
 
+targets += prelink.o prelink-efi.o
+
 $(TARGET)-syms: prelink.o xen.lds
 	$(LD) $(XEN_LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
 	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
-- 
2.22.0



Ping: [PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o
Posted by Jan Beulich 5 years, 4 months ago
On 14.09.2020 12:15, Jan Beulich wrote:
> Switch to $(call if_changed,ld) where possible; presumably not doing so
> in e321576f4047 ("xen/build: start using if_changed") right away was an
> oversight, as it did for Arm in (just) one case. It failed to add
> prelink.o to $(targets), though, causing - judging from the observed
> behavior on x86 - undue rebuilds of the final binary (because of
> prelink.o getting rebuild for $(cmd_prelink.o) being empty, in turn
> because of .prelink.o.cmd not getting read) during "make install-xen".
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/arch/arm/Makefile |  4 +++-
>  xen/arch/x86/Makefile | 18 ++++++++++--------
>  2 files changed, 13 insertions(+), 9 deletions(-)

May I ask for an Arm-side ack (or otherwise) here, please?

Jan

> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 51173d97127e..296c5e68bbc3 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -95,12 +95,14 @@ prelink_lto.o: $(ALL_OBJS)
>  
>  # Link it with all the binary objects
>  prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
> -	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
> +	$(call if_changed,ld)
>  else
>  prelink.o: $(ALL_OBJS) FORCE
>  	$(call if_changed,ld)
>  endif
>  
> +targets += prelink.o
> +
>  $(TARGET)-syms: prelink.o xen.lds
>  	$(LD) $(XEN_LDFLAGS) -T xen.lds -N prelink.o \
>  	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 74152f2a0dad..9b368632fb43 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -136,19 +136,21 @@ prelink_lto.o: $(ALL_OBJS)
>  	$(LD_LTO) -r -o $@ $^
>  
>  # Link it with all the binary objects
> -prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(EFI_OBJS-y)
> -	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
> +prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(EFI_OBJS-y) FORCE
> +	$(call if_changed,ld)
>  
> -prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
> -	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
> +prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o FORCE
> +	$(call if_changed,ld)
>  else
> -prelink.o: $(ALL_OBJS) $(EFI_OBJS-y)
> -	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
> +prelink.o: $(ALL_OBJS) $(EFI_OBJS-y) FORCE
> +	$(call if_changed,ld)
>  
> -prelink-efi.o: $(ALL_OBJS)
> -	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
> +prelink-efi.o: $(ALL_OBJS) FORCE
> +	$(call if_changed,ld)
>  endif
>  
> +targets += prelink.o prelink-efi.o
> +
>  $(TARGET)-syms: prelink.o xen.lds
>  	$(LD) $(XEN_LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
>  	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
> 


Re: Ping: [PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o
Posted by Julien Grall 5 years, 4 months ago
Hi Jan,

On 21/09/2020 11:17, Jan Beulich wrote:
> On 14.09.2020 12:15, Jan Beulich wrote:
>> Switch to $(call if_changed,ld) where possible; presumably not doing so
>> in e321576f4047 ("xen/build: start using if_changed") right away was an
>> oversight, as it did for Arm in (just) one case. It failed to add
>> prelink.o to $(targets), though, causing - judging from the observed
>> behavior on x86 - undue rebuilds of the final binary (because of
>> prelink.o getting rebuild for $(cmd_prelink.o) being empty, in turn
>> because of .prelink.o.cmd not getting read) during "make install-xen".
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>>   xen/arch/arm/Makefile |  4 +++-
>>   xen/arch/x86/Makefile | 18 ++++++++++--------
>>   2 files changed, 13 insertions(+), 9 deletions(-)
> 
> May I ask for an Arm-side ack (or otherwise) here, please?

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

> 
> Jan
> 
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 51173d97127e..296c5e68bbc3 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -95,12 +95,14 @@ prelink_lto.o: $(ALL_OBJS)
>>   
>>   # Link it with all the binary objects
>>   prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
>> -	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
>> +	$(call if_changed,ld)
>>   else
>>   prelink.o: $(ALL_OBJS) FORCE
>>   	$(call if_changed,ld)
>>   endif
>>   
>> +targets += prelink.o
>> +
>>   $(TARGET)-syms: prelink.o xen.lds
>>   	$(LD) $(XEN_LDFLAGS) -T xen.lds -N prelink.o \
>>   	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
>> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
>> index 74152f2a0dad..9b368632fb43 100644
>> --- a/xen/arch/x86/Makefile
>> +++ b/xen/arch/x86/Makefile
>> @@ -136,19 +136,21 @@ prelink_lto.o: $(ALL_OBJS)
>>   	$(LD_LTO) -r -o $@ $^
>>   
>>   # Link it with all the binary objects
>> -prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(EFI_OBJS-y)
>> -	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
>> +prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(EFI_OBJS-y) FORCE
>> +	$(call if_changed,ld)
>>   
>> -prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
>> -	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
>> +prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o FORCE
>> +	$(call if_changed,ld)
>>   else
>> -prelink.o: $(ALL_OBJS) $(EFI_OBJS-y)
>> -	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
>> +prelink.o: $(ALL_OBJS) $(EFI_OBJS-y) FORCE
>> +	$(call if_changed,ld)
>>   
>> -prelink-efi.o: $(ALL_OBJS)
>> -	$(LD) $(XEN_LDFLAGS) -r -o $@ $^
>> +prelink-efi.o: $(ALL_OBJS) FORCE
>> +	$(call if_changed,ld)
>>   endif
>>   
>> +targets += prelink.o prelink-efi.o
>> +
>>   $(TARGET)-syms: prelink.o xen.lds
>>   	$(LD) $(XEN_LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
>>   	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
>>
> 

-- 
Julien Grall

Re: Ping: [PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o
Posted by Jan Beulich 5 years, 4 months ago
On 21.09.2020 13:39, Julien Grall wrote:
> On 21/09/2020 11:17, Jan Beulich wrote:
>> On 14.09.2020 12:15, Jan Beulich wrote:
>>> Switch to $(call if_changed,ld) where possible; presumably not doing so
>>> in e321576f4047 ("xen/build: start using if_changed") right away was an
>>> oversight, as it did for Arm in (just) one case. It failed to add
>>> prelink.o to $(targets), though, causing - judging from the observed
>>> behavior on x86 - undue rebuilds of the final binary (because of
>>> prelink.o getting rebuild for $(cmd_prelink.o) being empty, in turn
>>> because of .prelink.o.cmd not getting read) during "make install-xen".
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>>   xen/arch/arm/Makefile |  4 +++-
>>>   xen/arch/x86/Makefile | 18 ++++++++++--------
>>>   2 files changed, 13 insertions(+), 9 deletions(-)
>>
>> May I ask for an Arm-side ack (or otherwise) here, please?
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

Thanks. On the Arm side this is actually addressing a (minor) bug,
so I wonder whether I should queue this up for backporting. Do you
have an opinion either way?

Jan

Re: Ping: [PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o
Posted by Julien Grall 5 years, 4 months ago
Hi Jan,

On 22/09/2020 09:28, Jan Beulich wrote:
> On 21.09.2020 13:39, Julien Grall wrote:
>> On 21/09/2020 11:17, Jan Beulich wrote:
>>> On 14.09.2020 12:15, Jan Beulich wrote:
>>>> Switch to $(call if_changed,ld) where possible; presumably not doing so
>>>> in e321576f4047 ("xen/build: start using if_changed") right away was an
>>>> oversight, as it did for Arm in (just) one case. It failed to add
>>>> prelink.o to $(targets), though, causing - judging from the observed
>>>> behavior on x86 - undue rebuilds of the final binary (because of
>>>> prelink.o getting rebuild for $(cmd_prelink.o) being empty, in turn
>>>> because of .prelink.o.cmd not getting read) during "make install-xen".
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>>    xen/arch/arm/Makefile |  4 +++-
>>>>    xen/arch/x86/Makefile | 18 ++++++++++--------
>>>>    2 files changed, 13 insertions(+), 9 deletions(-)
>>>
>>> May I ask for an Arm-side ack (or otherwise) here, please?
>>
>> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Thanks. On the Arm side this is actually addressing a (minor) bug,

Just to confirm, the bug is: Xen will be rebuilt when it is not 
necessary, right?

Cheers,

-- 
Julien Grall

Re: Ping: [PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o
Posted by Jan Beulich 5 years, 4 months ago
On 22.09.2020 11:24, Julien Grall wrote:
> On 22/09/2020 09:28, Jan Beulich wrote:
>> On 21.09.2020 13:39, Julien Grall wrote:
>>> On 21/09/2020 11:17, Jan Beulich wrote:
>>>> On 14.09.2020 12:15, Jan Beulich wrote:
>>>>> Switch to $(call if_changed,ld) where possible; presumably not doing so
>>>>> in e321576f4047 ("xen/build: start using if_changed") right away was an
>>>>> oversight, as it did for Arm in (just) one case. It failed to add
>>>>> prelink.o to $(targets), though, causing - judging from the observed
>>>>> behavior on x86 - undue rebuilds of the final binary (because of
>>>>> prelink.o getting rebuild for $(cmd_prelink.o) being empty, in turn
>>>>> because of .prelink.o.cmd not getting read) during "make install-xen".
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> ---
>>>>>    xen/arch/arm/Makefile |  4 +++-
>>>>>    xen/arch/x86/Makefile | 18 ++++++++++--------
>>>>>    2 files changed, 13 insertions(+), 9 deletions(-)
>>>>
>>>> May I ask for an Arm-side ack (or otherwise) here, please?
>>>
>>> Acked-by: Julien Grall <jgrall@amazon.com>
>>
>> Thanks. On the Arm side this is actually addressing a (minor) bug,
> 
> Just to confirm, the bug is: Xen will be rebuilt when it is not 
> necessary, right?

Yes. When building as non-root but installing as root, this would
typically involve an owner change of some of the involved files.
That's how I did notice the issue on x86 (after switching to
if_changed) in the first place.

Jan

Re: Ping: [PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o
Posted by Julien Grall 5 years, 4 months ago
Hi Jan,

On 22/09/2020 11:55, Jan Beulich wrote:
> On 22.09.2020 11:24, Julien Grall wrote:
>> On 22/09/2020 09:28, Jan Beulich wrote:
>>> On 21.09.2020 13:39, Julien Grall wrote:
>>>> On 21/09/2020 11:17, Jan Beulich wrote:
>>>>> On 14.09.2020 12:15, Jan Beulich wrote:
>>>>>> Switch to $(call if_changed,ld) where possible; presumably not doing so
>>>>>> in e321576f4047 ("xen/build: start using if_changed") right away was an
>>>>>> oversight, as it did for Arm in (just) one case. It failed to add
>>>>>> prelink.o to $(targets), though, causing - judging from the observed
>>>>>> behavior on x86 - undue rebuilds of the final binary (because of
>>>>>> prelink.o getting rebuild for $(cmd_prelink.o) being empty, in turn
>>>>>> because of .prelink.o.cmd not getting read) during "make install-xen".
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>> ---
>>>>>>     xen/arch/arm/Makefile |  4 +++-
>>>>>>     xen/arch/x86/Makefile | 18 ++++++++++--------
>>>>>>     2 files changed, 13 insertions(+), 9 deletions(-)
>>>>>
>>>>> May I ask for an Arm-side ack (or otherwise) here, please?
>>>>
>>>> Acked-by: Julien Grall <jgrall@amazon.com>
>>>
>>> Thanks. On the Arm side this is actually addressing a (minor) bug,
>>
>> Just to confirm, the bug is: Xen will be rebuilt when it is not
>> necessary, right?
> 
> Yes. When building as non-root but installing as root, this would
> typically involve an owner change of some of the involved files.
> That's how I did notice the issue on x86 (after switching to
> if_changed) in the first place.

Thanks for the explanation. I think it would be fine to backport.
@Stefano, what do you think?

Cheers,

-- 
Julien Grall

Re: Ping: [PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o
Posted by Stefano Stabellini 5 years, 4 months ago
On Tue, 22 Sep 2020, Julien Grall wrote:
> Hi Jan,
> 
> On 22/09/2020 11:55, Jan Beulich wrote:
> > On 22.09.2020 11:24, Julien Grall wrote:
> > > On 22/09/2020 09:28, Jan Beulich wrote:
> > > > On 21.09.2020 13:39, Julien Grall wrote:
> > > > > On 21/09/2020 11:17, Jan Beulich wrote:
> > > > > > On 14.09.2020 12:15, Jan Beulich wrote:
> > > > > > > Switch to $(call if_changed,ld) where possible; presumably not
> > > > > > > doing so
> > > > > > > in e321576f4047 ("xen/build: start using if_changed") right away
> > > > > > > was an
> > > > > > > oversight, as it did for Arm in (just) one case. It failed to add
> > > > > > > prelink.o to $(targets), though, causing - judging from the
> > > > > > > observed
> > > > > > > behavior on x86 - undue rebuilds of the final binary (because of
> > > > > > > prelink.o getting rebuild for $(cmd_prelink.o) being empty, in
> > > > > > > turn
> > > > > > > because of .prelink.o.cmd not getting read) during "make
> > > > > > > install-xen".
> > > > > > > 
> > > > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > > > > > > ---
> > > > > > >     xen/arch/arm/Makefile |  4 +++-
> > > > > > >     xen/arch/x86/Makefile | 18 ++++++++++--------
> > > > > > >     2 files changed, 13 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > May I ask for an Arm-side ack (or otherwise) here, please?
> > > > > 
> > > > > Acked-by: Julien Grall <jgrall@amazon.com>
> > > > 
> > > > Thanks. On the Arm side this is actually addressing a (minor) bug,
> > > 
> > > Just to confirm, the bug is: Xen will be rebuilt when it is not
> > > necessary, right?
> > 
> > Yes. When building as non-root but installing as root, this would
> > typically involve an owner change of some of the involved files.
> > That's how I did notice the issue on x86 (after switching to
> > if_changed) in the first place.
> 
> Thanks for the explanation. I think it would be fine to backport.
> @Stefano, what do you think?

I am OK with that