| Message ID | 20200331103102.1105674-13-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Build system improvements | expand |
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,
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,
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 $@ $<
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(-)