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

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

On Mon, Mar 07, 2022 at 09:18:42AM +0100, Jan Beulich wrote:
> 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.

Got a reply back from the LLD folks, and they consider the GNU ld
behavior quirky. Linux linker script does explicitly mention .symtab,
.strtab and shstrtab:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a840c4de56

So Xen should be safe to do the same.

> >>>>>> --- 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

That's weird, I wasn't aware we had any objdump calls after the final
image is linked.

> 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.

Hm, I didn't see any of those when doing my test build on FreeBSD, but
didn't check with gcc.

Thanks, Roger.