[XEN PATCH 03/15] build, x86: clean build log for boot/ targets

Anthony PERARD posted 15 patches 1 year, 6 months ago
[XEN PATCH 03/15] build, x86: clean build log for boot/ targets
Posted by Anthony PERARD 1 year, 6 months ago
We are adding %.lnk to .PRECIOUS or make treat them as intermediate
targets and remove them.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 xen/arch/x86/boot/Makefile | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 03d8ce3a9e..2693b938bd 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -5,6 +5,8 @@ head-bin-objs := cmdline.o reloc.o
 nocov-y   += $(head-bin-objs)
 noubsan-y += $(head-bin-objs)
 targets   += $(head-bin-objs)
+targets   += $(head-bin-objs:.o=.bin)
+targets   += $(head-bin-objs:.o=.lnk)
 
 head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))
 
@@ -26,10 +28,16 @@ $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
 LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
 LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
 
-%.bin: %.lnk
-	$(OBJCOPY) -j .text -O binary $< $@
+%.bin: OBJCOPYFLAGS := -j .text -O binary
+%.bin: %.lnk FORCE
+	$(call if_changed,objcopy)
 
-%.lnk: %.o $(src)/build32.lds
-	$(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) -o $@ $<
+quiet_cmd_ld_lnk_o = LD      $@
+cmd_ld_lnk_o = $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) -o $@ $<
+
+%.lnk: %.o $(src)/build32.lds FORCE
+	$(call if_changed,ld_lnk_o)
 
 clean-files := *.lnk *.bin
+
+.PRECIOUS: %.lnk
-- 
Anthony PERARD
Re: [XEN PATCH 03/15] build, x86: clean build log for boot/ targets
Posted by Jan Beulich 1 year, 6 months ago
On 23.05.2023 18:37, Anthony PERARD wrote:
> We are adding %.lnk to .PRECIOUS or make treat them as intermediate
> targets and remove them.

What's wrong with them getting removed? Note also that's no different from
today, so it's an orthogonal change in any event (unless I'm overlooking
something). Plus if such behavior was intended, shouldn't $(targets) be
made a prereq of .PRECIOUS in generic makefile logic?

> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  xen/arch/x86/boot/Makefile | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index 03d8ce3a9e..2693b938bd 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -5,6 +5,8 @@ head-bin-objs := cmdline.o reloc.o
>  nocov-y   += $(head-bin-objs)
>  noubsan-y += $(head-bin-objs)
>  targets   += $(head-bin-objs)
> +targets   += $(head-bin-objs:.o=.bin)
> +targets   += $(head-bin-objs:.o=.lnk)

Leaving aside the question of whether .lnk really should be part
of $(targets), don't these two lines also ...

> @@ -26,10 +28,16 @@ $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
>  LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
>  LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
>  
> -%.bin: %.lnk
> -	$(OBJCOPY) -j .text -O binary $< $@
> +%.bin: OBJCOPYFLAGS := -j .text -O binary
> +%.bin: %.lnk FORCE
> +	$(call if_changed,objcopy)
>  
> -%.lnk: %.o $(src)/build32.lds
> -	$(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) -o $@ $<
> +quiet_cmd_ld_lnk_o = LD      $@
> +cmd_ld_lnk_o = $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) -o $@ $<
> +
> +%.lnk: %.o $(src)/build32.lds FORCE
> +	$(call if_changed,ld_lnk_o)
>  
>  clean-files := *.lnk *.bin

... eliminate the need for this?

Jan

> +
> +.PRECIOUS: %.lnk
Re: [XEN PATCH 03/15] build, x86: clean build log for boot/ targets
Posted by Anthony PERARD 1 year, 6 months ago
On Wed, May 24, 2023 at 04:16:54PM +0200, Jan Beulich wrote:
> On 23.05.2023 18:37, Anthony PERARD wrote:
> > We are adding %.lnk to .PRECIOUS or make treat them as intermediate
> > targets and remove them.
> 
> What's wrong with them getting removed? Note also that's no different from
> today, so it's an orthogonal change in any event (unless I'm overlooking

Indeed, those targets are been removed today. That doesn't cause any
issue because make knows that they are just intermediate and it doesn't
have to rebuilt them if there are just missing.

But, the macro $(if_changed,) doesn't know about intermediates, and if
the target is missing, then it's going to be rebuilt. So with
$(if_changed,) the %.lnk targets are been rebuilt at every incremental
build which cause make to relink "xen" when there's otherwise nothing
to be done.
(I'm using a command like `XEN_BUILD_DATE=today XEN_BUILD_TIME=now make`
to avoid "compile.h" from been regenerated every time)

So, the change isn't orthogonal, but needs a better explanation in the
commit message.

> something). Plus if such behavior was intended, shouldn't $(targets) be
> made a prereq of .PRECIOUS in generic makefile logic?

I think I need to reevaluate what to do here. Maybe .PRECIOUS isn't the
right one to use. But yes, we probably want something generic to tell
make to never delete any $(targets) when they are intermediate.

Linux uses .SECONDARY or .NOTINTERMEDIATE, and .SECONDARY might be
better than .PRECIOUS. PRECIOUS also prevent make from delete a target
when make is interrupted or killed, which might not be desired.

> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> >  xen/arch/x86/boot/Makefile | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> > index 03d8ce3a9e..2693b938bd 100644
> > --- a/xen/arch/x86/boot/Makefile
> > +++ b/xen/arch/x86/boot/Makefile
> > @@ -5,6 +5,8 @@ head-bin-objs := cmdline.o reloc.o
> >  nocov-y   += $(head-bin-objs)
> >  noubsan-y += $(head-bin-objs)
> >  targets   += $(head-bin-objs)
> > +targets   += $(head-bin-objs:.o=.bin)
> > +targets   += $(head-bin-objs:.o=.lnk)
> 
> Leaving aside the question of whether .lnk really should be part
> of $(targets), don't these two lines also ...
> 
> > @@ -26,10 +28,16 @@ $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
> >  LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
> >  LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
> >  
> > -%.bin: %.lnk
> > -	$(OBJCOPY) -j .text -O binary $< $@
> > +%.bin: OBJCOPYFLAGS := -j .text -O binary
> > +%.bin: %.lnk FORCE
> > +	$(call if_changed,objcopy)
> >  
> > -%.lnk: %.o $(src)/build32.lds
> > -	$(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) -o $@ $<
> > +quiet_cmd_ld_lnk_o = LD      $@
> > +cmd_ld_lnk_o = $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) -o $@ $<
> > +
> > +%.lnk: %.o $(src)/build32.lds FORCE
> > +	$(call if_changed,ld_lnk_o)
> >  
> >  clean-files := *.lnk *.bin
> 
> ... eliminate the need for this?

Yes, but that doesn't seems to work here.

I think there's a "targets:=" missing in "Makefile.clean". So, new patch
to fix that, and then I can eliminate the "clean-files".

Cheers,

-- 
Anthony PERARD
Re: [XEN PATCH 03/15] build, x86: clean build log for boot/ targets
Posted by Andrew Cooper 1 year, 6 months ago
On 23/05/2023 5:37 pm, Anthony PERARD wrote:
> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index 03d8ce3a9e..2693b938bd 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -26,10 +28,16 @@ $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
>  LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
>  LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
>  
> -%.bin: %.lnk
> -	$(OBJCOPY) -j .text -O binary $< $@
> +%.bin: OBJCOPYFLAGS := -j .text -O binary
> +%.bin: %.lnk FORCE
> +	$(call if_changed,objcopy)
>  
> -%.lnk: %.o $(src)/build32.lds
> -	$(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) -o $@ $<
> +quiet_cmd_ld_lnk_o = LD      $@

I'd suggest that this be LD32 because it is different to most other LD's
in the log.

However, this is a definite improvement.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>

Happy to fix up on commit.