Re: [PATCH] x86/build: use --orphan-handling linker option if available

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.03.2022 10:19, Roger Pau Monné wrote:
> On Fri, Mar 04, 2022 at 09:02:08AM +0100, Jan Beulich wrote:
>> On 03.03.2022 16:09, Roger Pau Monné wrote:
>>> On Thu, Mar 03, 2022 at 01:17:03PM +0100, Jan Beulich wrote:
>>>> On 03.03.2022 12:19, Roger Pau Monné wrote:
>>>>> On Wed, Mar 02, 2022 at 03:19:35PM +0100, Jan Beulich wrote:
>>>>>> As was e.g. making necessary 4b7fd8153ddf ("x86: fold sections in final
>>>>>> binaries"), arbitrary sections appearing without our linker script
>>>>>> placing them explicitly can be a problem. Have the linker make us aware
>>>>>> of such sections, so we would know that the script needs adjusting.
>>>>>>
>>>>>> To deal with the resulting warnings:
>>>>>> - Retain .note.* explicitly for ELF, and discard all of them (except the
>>>>>>   earlier consumed .note.gnu.build-id) for PE/COFF.
>>>>>> - Have explicit statements for .got, .plt, and alike and add assertions
>>>>>>   that they're empty. No output sections will be created for these as
>>>>>>   long as they remain empty (or else the assertions would cause early
>>>>>>   failure anyway).
>>>>>> - Collect all .rela.* into a single section, with again an assertion
>>>>>>   added for the resulting section to be empty.
>>>>>> - Extend the enumerating of .debug_* to ELF. Note that for Clang adding
>>>>>>   of .debug_macinfo is necessary. Amend this by its Dwarf5 counterpart,
>>>>>>   .debug_macro, then as well (albeit more may need adding for full
>>>>>>   coverage).
>>>>>>
>>>>>> Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>> ---
>>>>>> I would have wanted to make this generic (by putting it in
>>>>>> xen/Makefile), but the option cannot be added to LDFLAGS, or else
>>>>>> there'll be a flood of warnings with $(LD) -r. (Besides, adding to
>>>>>> LDFLAGS would mean use of the option on every linker pass rather than
>>>>>> just the last one.)
>>>>>>
>>>>>> Retaining of .note in xen-syms is under question. Plus if we want to
>>>>>> retain all notes, the question is whether they wouldn't better go into
>>>>>> .init.rodata. But .note.gnu.build-id shouldn't move there, and when
>>>>>> notes are discontiguous all intermediate space will also be assigned to
>>>>>> the NOTE segment, thus making the contents useless for tools going just
>>>>>> by program headers.
>>>>>>
>>>>>> Newer Clang may require yet more .debug_* to be added. I've only played
>>>>>> with versions 5 and 7 so far.
>>>>>>
>>>>>> Unless we would finally drop all mentioning of Stabs sections, we may
>>>>>> want to extend to there what is done for Dwarf here (allowing the EFI
>>>>>> conditional around the section to also go away).
>>>>>>
>>>>>> See also 
>>>>>> https://sourceware.org/pipermail/binutils/2022-March/119922.html.
>>>>>
>>>>> LLD 13.0.0 also warns about:
>>>>>
>>>>> ld: warning: <internal>:(.symtab) is being placed in '.symtab'
>>>>> ld: warning: <internal>:(.shstrtab) is being placed in '.shstrtab'
>>>>> ld: warning: <internal>:(.strtab) is being placed in '.strtab'
>>>>>
>>>>> So seeing your mail where you mention GNU ld not needing those, I
>>>>> think we would need to add them anyway for LLVM ld.
>>>>
>>>> Hmm, that's ugly. How do I recognize LLVM ld? I can't simply use a
>>>> pre-processor conditional keying off of __clang__, as that used as the
>>>> compiler doesn't mean their ld is also in use (typically the case on
>>>> Linux).
>>>
>>> Hard to tell, `ld -v` for LLD will typically contain '^LLD' I think,
>>> but I don't really like matching on human readable output like this.
>>
>> Same here. But Linux'es ld-version.sh looks to be doing just that.
> 
> OK, so be it then. We can always improve afterwards, as I don't really
> have any better suggestion ATM.
> 
>>>> I also don't want to add these uniformly, for now knowing what
>>>> side effects their mentioning might have with GNU ld.
>>>
>>> Wouldn't it be fine to just place them at the end, just like it's
>>> done by default by ld?
>>>
>>> Are you worried about not getting the proper type if mentioned in the
>>> linker script?
>>
>> I'm worried of about any kind of anomaly that could be caused by
>> mentioning sections which a linker doesn't expect to be named in
>> a script. That's hardly something they would even test their
>> linkers against.
> 
> I've raised a bug with LLD:
> 
> https://github.com/llvm/llvm-project/issues/54194
> 
> To see whether this behavior is intended.
> 
>>>>>> --- a/xen/arch/x86/Makefile
>>>>>> +++ b/xen/arch/x86/Makefile
>>>>>> @@ -120,6 +120,8 @@ syms-warn-dup-y := --warn-dup
>>>>>>  syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
>>>>>>  syms-warn-dup-$(CONFIG_ENFORCE_UNIQUE_SYMBOLS) := --error-dup
>>>>>>  
>>>>>> +orphan-handling-$(call ld-option,--orphan-handling=warn) += 
>>>>>> --orphan-handling=warn
>>>>>
>>>>> Might be better to place in xen/Kconfig with the CC checks?
>>>>
>>>> Well. I've tried to stay away from complaining if people introduce
>>>> new tool chain capability checks in Kconfig. But I'm not going to
>>>> add any myself (unless things would become really inconsistent) up
>>>> and until we have actually properly discussed the upsides and
>>>> downsides of either model. Doing this via email (see the "Kconfig
>>>> vs tool chain capabilities" thread started in August 2020) has
>>>> proven to not lead anywhere. I'm really hoping that we can finally
>>>> sort this in Bukarest.
>>>>
>>>>> I'm also wondering whether we could add the flag here into XEN_LDFLAGS
>>>>> and EFI_LDFLAGS, as those options are only used together with the
>>>>> linker script in the targets on the Makefile.
>>>>
>>>> Not for XEN_LDFLAGS at least, and undesirable for EFI_LDFLAGS. See
>>>> the respective post-commit message remark.
>>>
>>> But the calls to LD in order to generate $(TARGET)-syms do not use -r,
>>> and are all using the linker script, so it should be fine to use
>>> --orphan-handling=warn there?
>>
>> But XEN_LDFLAGS is also used elsewhere together with -r. (Whether
>> that's actually correct is a different question.)
>>
>>> Could we do something like:
>>>
>>> $(TARGET)-syms: XEN_LDFLAGS += ...
>>>
>>> And similar for $(TARGET).efi?
>>
>> Yes, this ought to be possible, but would again lead to the option
>> being passed on all three linking stages instead of just the final
>> one. When there are many warnings (e.g. because of the same kind of
>> section appearing many times), it's not helpful to see the flood
>> three times (or likely even six times, once for xen-syms and once
>> for xen.efi).
> 
> OK, I think our build system is already quite chatty, so wouldn't
> really care about seeing repeated messages there. We can find a way to
> generalize passing options to the final linker step if/when we need to
> add more.
> 
> I'm fine with doing the LLD fixup as a separate patch, so:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks. However, something is wrong here. Unlike in my local builds, the
pre-push build test I did after committing this triggered a massive amount
(tens of thousands) of objdump warnings:

CU at offset ... contains corrupt or unsupported version number: 0
Invalid pointer size (0) in compunit header, using 4 instead

Helpfully it doesn't say whether that's xen-syms, xen-efi, or both. I'll
have to investigate and fix; I can only guess at this point that this
might be triggered by a difference in .config, or be hidden by some
other change I have in my local tree.

Jan