In Arm and X86 makefile, generating the linker script is the same, so
we can simply have both call the same macro.

We need to add *.lds files into extra-y so that Rules.mk can find the
.*.cmd dependency file and load it.

Change made to the command line:
- Use of $(CPP) instead of $(CC) -E
- Remove -Ui386.
  We don't compile Xen for i386 anymore, so that macro is never
  defined. Also it doesn't make sense on Arm.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Notes:
    v4:
    - fix rebuild by adding FORCE as dependency
    - Use $(CPP)
    - remove -Ui386
    - avoid using "define" for cmd_cc_lds_S, as adding '; \' on each line is
      still mandatory for if_changed (or cmd) macro to work.

 xen/Rules.mk          | 6 ++++++
 xen/arch/arm/Makefile | 8 ++++----
 xen/arch/x86/Makefile | 8 ++++----
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index e126e4972dec..616c6ae179d8 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -236,6 +236,12 @@ cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@
 %.s: %.S FORCE
 	$(call if_changed,cpp_s_S)
 
+# Linker scripts, .lds.S -> .lds
+quiet_cmd_cc_lds_S = LDS     $@
+cmd_cc_lds_S = $(CPP) -P $(filter-out -Wa$(comma)%,$(a_flags)) -o $@ $<; \
+    sed -e 's/.*\.lds\.o:/$(@F):/g' <$(dot-target).d >$(dot-target).d.new; \
+    mv -f $(dot-target).d.new $(dot-target).d
+
 # Add intermediate targets:
 # When building objects with specific suffix patterns, add intermediate
 # targets that the final targets are derived from.
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index b79ad55646bd..45484d6d11b2 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -64,6 +64,8 @@ obj-y += vpsci.o
 obj-y += vuart.o
 extra-y += $(TARGET_SUBARCH)/head.o
 
+extra-y += xen.lds
+
 #obj-bin-y += ....o
 
 ifdef CONFIG_DTB_FILE
@@ -122,10 +124,8 @@ $(TARGET)-syms: prelink.o xen.lds
 asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
 	$(CC) $(filter-out -flto,$(c_flags)) -S -o $@ $<
 
-xen.lds: xen.lds.S
-	$(CC) -P -E -Ui386 $(a_flags) -o $@ $<
-	sed -e 's/xen\.lds\.o:/xen\.lds:/g' <.xen.lds.d >.xen.lds.d.new
-	mv -f .xen.lds.d.new .xen.lds.d
+xen.lds: xen.lds.S FORCE
+	$(call if_changed,cc_lds_S)
 
 dtb.o: $(CONFIG_DTB_FILE)
 
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 44137d919b66..eb6f7a6aceca 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -72,6 +72,7 @@ obj-y += hpet.o
 obj-y += vm_event.o
 obj-y += xstate.o
 extra-y += asm-macros.i
+extra-y += xen.lds
 
 x86_emulate.o: x86_emulate/x86_emulate.c x86_emulate/x86_emulate.h
 
@@ -173,6 +174,7 @@ export XEN_BUILD_EFI := $(shell $(CC) $(XEN_CFLAGS) -c efi/check.c -o efi/check.
 # Check if the linker supports PE.
 XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(shell $(LD) -mi386pep --subsystem=10 -o efi/check.efi efi/check.o 2>/dev/null && echo y))
 CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI
+extra-$(XEN_BUILD_PE) += efi.lds
 
 $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p')
 $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
@@ -240,10 +242,8 @@ $(BASEDIR)/include/asm-x86/asm-macros.h: asm-macros.i Makefile
 	$(call move-if-changed,$@.new,$@)
 
 efi.lds: AFLAGS-y += -DEFI
-xen.lds efi.lds: xen.lds.S
-	$(CC) -P -E -Ui386 $(filter-out -Wa$(comma)%,$(a_flags)) -o $@ $<
-	sed -e 's/.*\.lds\.o:/$(@F):/g' <.$(@F).d >.$(@F).d.new
-	mv -f .$(@F).d.new .$(@F).d
+xen.lds efi.lds: xen.lds.S FORCE
+	$(call if_changed,cc_lds_S)
 
 boot/mkelf32: boot/mkelf32.c
 	$(HOSTCC) $(HOSTCFLAGS) -o $@ $<
-- 
Anthony PERARD

On 31.03.2020 12:30, Anthony PERARD wrote:
> In Arm and X86 makefile, generating the linker script is the same, so
> we can simply have both call the same macro.
> 
> We need to add *.lds files into extra-y so that Rules.mk can find the
> .*.cmd dependency file and load it.
> 
> Change made to the command line:
> - Use of $(CPP) instead of $(CC) -E
> - Remove -Ui386.
>   We don't compile Xen for i386 anymore, so that macro is never
>   defined. Also it doesn't make sense on Arm.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 
> Notes:
>     v4:
>     - fix rebuild by adding FORCE as dependency
>     - Use $(CPP)
>     - remove -Ui386
>     - avoid using "define" for cmd_cc_lds_S, as adding '; \' on each line is
>       still mandatory for if_changed (or cmd) macro to work.

I still don't believe in there being a need for "; \" there. This
actually breaks things, after all:

> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -236,6 +236,12 @@ cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@
>  %.s: %.S FORCE
>  	$(call if_changed,cpp_s_S)
>  
> +# Linker scripts, .lds.S -> .lds
> +quiet_cmd_cc_lds_S = LDS     $@
> +cmd_cc_lds_S = $(CPP) -P $(filter-out -Wa$(comma)%,$(a_flags)) -o $@ $<; \
> +    sed -e 's/.*\.lds\.o:/$(@F):/g' <$(dot-target).d >$(dot-target).d.new; \
> +    mv -f $(dot-target).d.new $(dot-target).d

if $(CPP) or sed fail, previously the whole rule would have failed,
which no longer is the case with your use of semicolons. There
ought to be a solution to this, ideally one better than adding
"set -e" as the first command ("define" would at least deal with
the multi-line make issue, but without it being clear to me why the
semicolons would be needed I don't think I can suggest anything
there at the moment).

Jan

On Wed, Apr 08, 2020 at 02:46:42PM +0200, Jan Beulich wrote:
> On 31.03.2020 12:30, Anthony PERARD wrote:
> >     - avoid using "define" for cmd_cc_lds_S, as adding '; \' on each line is
> >       still mandatory for if_changed (or cmd) macro to work.
> 
> I still don't believe in there being a need for "; \" there. This
> actually breaks things, after all:
> 
> > --- a/xen/Rules.mk
> > +++ b/xen/Rules.mk
> > @@ -236,6 +236,12 @@ cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@
> >  %.s: %.S FORCE
> >  	$(call if_changed,cpp_s_S)
> >  
> > +# Linker scripts, .lds.S -> .lds
> > +quiet_cmd_cc_lds_S = LDS     $@
> > +cmd_cc_lds_S = $(CPP) -P $(filter-out -Wa$(comma)%,$(a_flags)) -o $@ $<; \
> > +    sed -e 's/.*\.lds\.o:/$(@F):/g' <$(dot-target).d >$(dot-target).d.new; \
> > +    mv -f $(dot-target).d.new $(dot-target).d
> 
> if $(CPP) or sed fail, previously the whole rule would have failed,
> which no longer is the case with your use of semicolons. There
> ought to be a solution to this, ideally one better than adding
> "set -e" as the first command ("define" would at least deal with
> the multi-line make issue, but without it being clear to me why the
> semicolons would be needed I don't think I can suggest anything
> there at the moment).

The only macro that will consumes cmd_cc_lds_S (and other cmd_*) is
"cmd", it is defined as:
    cmd = @set -e; $(echo-cmd) $(cmd_$(1))
So, "set -e" is already there, and using semicolons in commands is
equivalent to using "&&".

With "cmd" alone, multi-line command would work as expected (unless
$(echo-cmd) is is trying to print the command line).

It's "if_changed" macro that doesn't work with multi-line commands.
It does:
    $(cmd); printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd
With a multiple line command, $(make-cmd) get's expanded to multiple
line, so the second argument of "printf" is going to be spread over
multiple line in make, and thus multiple shell. We run into this error:
    /bin/sh: -c: line 0: unexpected EOF while looking for matching `''
    /bin/sh: -c: line 1: syntax error: unexpected end of file

This is why we need to have commands on a single line.

I hope the explanation is clear enough.

Thanks,

-- 
Anthony PERARD

On 15.04.2020 18:58, Anthony PERARD wrote:
> On Wed, Apr 08, 2020 at 02:46:42PM +0200, Jan Beulich wrote:
>> On 31.03.2020 12:30, Anthony PERARD wrote:
>>>     - avoid using "define" for cmd_cc_lds_S, as adding '; \' on each line is
>>>       still mandatory for if_changed (or cmd) macro to work.
>>
>> I still don't believe in there being a need for "; \" there. This
>> actually breaks things, after all:
>>
>>> --- a/xen/Rules.mk
>>> +++ b/xen/Rules.mk
>>> @@ -236,6 +236,12 @@ cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@
>>>  %.s: %.S FORCE
>>>  	$(call if_changed,cpp_s_S)
>>>  
>>> +# Linker scripts, .lds.S -> .lds
>>> +quiet_cmd_cc_lds_S = LDS     $@
>>> +cmd_cc_lds_S = $(CPP) -P $(filter-out -Wa$(comma)%,$(a_flags)) -o $@ $<; \
>>> +    sed -e 's/.*\.lds\.o:/$(@F):/g' <$(dot-target).d >$(dot-target).d.new; \
>>> +    mv -f $(dot-target).d.new $(dot-target).d
>>
>> if $(CPP) or sed fail, previously the whole rule would have failed,
>> which no longer is the case with your use of semicolons. There
>> ought to be a solution to this, ideally one better than adding
>> "set -e" as the first command ("define" would at least deal with
>> the multi-line make issue, but without it being clear to me why the
>> semicolons would be needed I don't think I can suggest anything
>> there at the moment).
> 
> The only macro that will consumes cmd_cc_lds_S (and other cmd_*) is
> "cmd", it is defined as:
>     cmd = @set -e; $(echo-cmd) $(cmd_$(1))
> So, "set -e" is already there, and using semicolons in commands is
> equivalent to using "&&".
> 
> With "cmd" alone, multi-line command would work as expected (unless
> $(echo-cmd) is is trying to print the command line).
> 
> It's "if_changed" macro that doesn't work with multi-line commands.
> It does:
>     $(cmd); printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd
> With a multiple line command, $(make-cmd) get's expanded to multiple
> line, so the second argument of "printf" is going to be spread over
> multiple line in make, and thus multiple shell. We run into this error:
>     /bin/sh: -c: line 0: unexpected EOF while looking for matching `''
>     /bin/sh: -c: line 1: syntax error: unexpected end of file
> 
> This is why we need to have commands on a single line.
> 
> I hope the explanation is clear enough.

Yes, thanks. One question remains though: Why do we need multiple
commands here in the first place, when Linux gets away with one?

Two other remarks: For one the command's name, aiui, ought to be
cmd_cpp_lds_S (see Linux). And there ought to be cpp_flags, which
would then also be used by e.g. cmd_s_S (instead of both having
$(filter-out -Wa$(comma)%,$(a_flags)) open-coded).

Jan

On Thu, Apr 16, 2020 at 09:36:15AM +0200, Jan Beulich wrote:
> On 15.04.2020 18:58, Anthony PERARD wrote:
> > On Wed, Apr 08, 2020 at 02:46:42PM +0200, Jan Beulich wrote:
> >> On 31.03.2020 12:30, Anthony PERARD wrote:
> >>>     - avoid using "define" for cmd_cc_lds_S, as adding '; \' on each line is
> >>>       still mandatory for if_changed (or cmd) macro to work.
> >>
> >> I still don't believe in there being a need for "; \" there. This
> >> actually breaks things, after all:
> >>
> >>> --- a/xen/Rules.mk
> >>> +++ b/xen/Rules.mk
> >>> @@ -236,6 +236,12 @@ cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@
> >>>  %.s: %.S FORCE
> >>>  	$(call if_changed,cpp_s_S)
> >>>  
> >>> +# Linker scripts, .lds.S -> .lds
> >>> +quiet_cmd_cc_lds_S = LDS     $@
> >>> +cmd_cc_lds_S = $(CPP) -P $(filter-out -Wa$(comma)%,$(a_flags)) -o $@ $<; \
> >>> +    sed -e 's/.*\.lds\.o:/$(@F):/g' <$(dot-target).d >$(dot-target).d.new; \
> >>> +    mv -f $(dot-target).d.new $(dot-target).d
> >>
> >> if $(CPP) or sed fail, previously the whole rule would have failed,
> >> which no longer is the case with your use of semicolons. There
> >> ought to be a solution to this, ideally one better than adding
> >> "set -e" as the first command ("define" would at least deal with
> >> the multi-line make issue, but without it being clear to me why the
> >> semicolons would be needed I don't think I can suggest anything
> >> there at the moment).
> > 
> > The only macro that will consumes cmd_cc_lds_S (and other cmd_*) is
> > "cmd", it is defined as:
> >     cmd = @set -e; $(echo-cmd) $(cmd_$(1))
> > So, "set -e" is already there, and using semicolons in commands is
> > equivalent to using "&&".
> > 
> > With "cmd" alone, multi-line command would work as expected (unless
> > $(echo-cmd) is is trying to print the command line).
> > 
> > It's "if_changed" macro that doesn't work with multi-line commands.
> > It does:
> >     $(cmd); printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd
> > With a multiple line command, $(make-cmd) get's expanded to multiple
> > line, so the second argument of "printf" is going to be spread over
> > multiple line in make, and thus multiple shell. We run into this error:
> >     /bin/sh: -c: line 0: unexpected EOF while looking for matching `''
> >     /bin/sh: -c: line 1: syntax error: unexpected end of file
> > 
> > This is why we need to have commands on a single line.
> > 
> > I hope the explanation is clear enough.
> 
> Yes, thanks. One question remains though: Why do we need multiple
> commands here in the first place, when Linux gets away with one?

Actually, Linux also has multiple commands as well. After running CPP,
it runs ./fixdep (via if_change_dep) which does at least the same thing
as our sed command. We can't use fixdep yet, but I'm working toward it.

> Two other remarks: For one the command's name, aiui, ought to be
> cmd_cpp_lds_S (see Linux). And there ought to be cpp_flags, which
> would then also be used by e.g. cmd_s_S (instead of both having
> $(filter-out -Wa$(comma)%,$(a_flags)) open-coded).

When switching to use CPP instead of CC, I forgot to rename the command,
so I'll fix that.

I'll look at introducing cpp_flags.

Thanks,

-- 
Anthony PERARD