Re: [PATCH] x86: conditionalize workaround for build issue with GNU ld 2.37

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2021 15:37, Roger Pau Monné wrote:
> On Thu, Sep 09, 2021 at 04:35:49PM +0200, Jan Beulich wrote:
>> I haven't been able to find an environment where I could actually try
>> with lld (ld.lld); all testing was with GNU ld (ld.bfd).
> 
> Thanks for fixing this. I've been able to test with LLVM ld and the
> workaround is fine.

Oh, good, thanks for trying this out.

>> --- a/xen/arch/x86/Makefile
>> +++ b/xen/arch/x86/Makefile
>> @@ -92,10 +92,16 @@ efi-$(CONFIG_PV_SHIM_EXCLUSIVE) :=
>>  
>>  ifneq ($(build_id_linker),)
>>  notes_phdrs = --notes
>> +# Determine whether to engage a workaround for GNU ld 2.37.
>> +build-id-ld-good = $(shell echo 'void test(void) {}' \
>> +                           | $(CC) $(XEN_CFLAGS) -o .check.o -c -x c - 
>> 2>.check.err \
>> +                           && $(LD) -T check.lds -o .check.elf .check.o 
>> 2>>.check.err \
>> +                           && echo y)
> 
> Do we want to make this a Kconfig option (ie: LD_UNQUOTED_DASH) and
> then use is here?
> 
> We already have compiler and assembler checks in x86/Kconfig, so it
> would seem more natural to place it there.

The question of whether to record tool chain capabilities in .config
is still pending. I'm not convinced this is a good idea, Andrew keeps
shouting me out for that, and an actual discussion doesn't really
happen. Yet unlike back at the time when I first raised my concern,
Anthony meanwhile supports me in at least the question (to Andrew) of
when such a discussion would have happened: Neither of us is aware,
yet Andrew claims it did happen, but so far didn't point out where
one could read about what was discussed and decided there.

For the few uses we've accumulated I gave (if at all) an ack for
things happening under some sort of pressure, with the request that
aformentioned discussion would happen subsequently (and, depending on
outcome, these would be converted to another approach if need be). I
have meanwhile realized that it was a mistake to allow such things in
on this basis - the more of them we gain, the more I'm hearing "we've
already got some".

>>  else
>>  ifeq ($(CONFIG_PVH_GUEST),y)
>>  notes_phdrs = --notes
>>  endif
>> +build-id-ld-good := y
>>  endif
> 
> I also wonder whether we need to make the quoting tied to the usage of
> build-id. I guess we don't add sections with dashes and instead
> use underscores, but it might be prudent to always quote to be on the
> safe side if dashes are not supported.

If quoting was uniformly supported, I might have considered that. But
it not being uniformly supported is the reason for this change in the
first place. Hence I'd prefer to generalize this only if really needed.

>> --- /dev/null
>> +++ b/xen/arch/x86/check.lds
> 
> I would maybe name this check-dash.lds, in case we need to add more ld
> build tests.

I sincerely hope it was a one-off that a binutils release got cut with
this sort of a supposedly prominent bug. Considering that the dash is
merely what we're after in this specific case, but breakage was wider
(presumably about any printable char that's not alnum or underscore),
I'd consider check-dash too specific a name. You only say "maybe"; if
you were sufficiently convinced, this is an adjustment I'd be willing
to make. Yet even better would be if I / we could just be done with
this.

Jan