Re: [PATCH] zap linking-only option from EMBEDDED_EXTRA_CFLAGS

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Sep 27, 2022 at 04:32:27PM +0200, Jan Beulich wrote:
> On 27.09.2022 16:14, Roger Pau Monné wrote:
> > On Fri, Sep 09, 2022 at 09:22:52AM +0200, Jan Beulich wrote:
> >> While I was suspicious of the compiler issuing a diagnostic about an
> >> unused linking-only option when not doing any linking, I did check this
> >> with a couple of gcc versions only, but not with Clang. (Oddly enough at
> >> least older Clang versions complain about the use of '-nopie' now that
> >> we actually use '-no-pie'.) Filter out the problematic option in all
> >> cases where the variable is consumed for compilation only (which right
> >> now is everywhere).
> >>
> >> Fixes: ecd6b9759919 ("Config.mk: correct PIE-related option(s) in 
> >> EMBEDDED_EXTRA_CFLAGS")
> >> Reported-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >> Arguably with all users of EMBEDDED_EXTRA_CFLAGS using these just for
> >> compiling, the option could be omitted from that variable right away.
> >> But if any compile-and-link-in-one-go use appeared, there would be an
> >> issue.
> > 
> > Is it feasible to have compile-and-link-in-one-go in one use feasible
> > with what we consider embedded (firmware or kernel like binaries).  I
> > would expect those to always require a linker script and a separate
> > linking step.
> 
> A separate linking step doesn't mean this needs doing via $(LD) - it
> could also be done via $(CC). There's also no connection between using
> a separate linking step and using a linker script - aiui the linker
> script could also be handed to $(CC) for it to pass on the option to
> the linker.

There's one thing that puzzles me, if we already pass -fno-pie for
code generation, do we also need the -no-pie linker option explicitly
added?  I would expect the compiler to be clever enough to
automatically pass -no-pie to the linker if -fno-pie is used,
otherwise the code won't be correctly linked?

I would rather prefer to remove the -no-pie option from
EMBEDDED_EXTRA_CFLAGS and just add a note that users wanting to
link-in-place need to review the set of options used.

> >> --- a/tools/tests/x86_emulator/testcase.mk
> >> +++ b/tools/tests/x86_emulator/testcase.mk
> >> @@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
> >>  CFLAGS :=
> >>  include $(XEN_ROOT)/tools/Rules.mk
> >>  
> >> -$(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
> >> +$(call cc-options-add,CFLAGS,CC,$(filter-out 
> >> -no-pie,$(EMBEDDED_EXTRA_CFLAGS)))
> > 
> > Is the x86 emulator harness correct in using EMBEDDED_EXTRA_CFLAGS?
> 
> Yes, I think it is (here): This is the script to build the blobs we
> then have the emulator process. Of course it wouldn't be right to
> use for building the actual harness executable.

Oh, OK, didn't gasp it was building blobs of code to pass to the
instruction emulator.

> > TBH I'm not sure the naming and usage of the variable is very
> > helpful, maybe it would better be STANDALONE_EXTRA_CFLAGS, and drop
> > it's usage from the x86 emulator test harness, open code the needed
> > flags for that use-case.
> 
> I agree the naming is, well, odd. I would be okay with the proposed
> alternative name, but I also don't view that as all-so-much-better.

Anyway, it's clearer for me, but I agree it's not much better.  Let's
leave the naming discussion for another time if you don't think it's
much better.

Thanks, Roger.