Re: [XEN PATCH v4 12/18] xen/build: factorise generation of the linker scripts

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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