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

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

On Tue, Mar 08, 2022 at 12:15:04PM +0100, Jan Beulich wrote:
> On 08.03.2022 11:12, Roger Pau Monné wrote:
> > On Mon, Mar 07, 2022 at 02:53:32PM +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>
> > 
> > LGTM, just two questions.
> 
> Sure, just that ...
> 
> >> @@ -19,6 +26,8 @@ ENTRY(efi_start)
> >>  
> >>  #define FORMAT "elf64-x86-64"
> >>  #define DECL_SECTION(x) #x : AT(ADDR(#x) - __XEN_VIRT_START)
> >> +#define DECL_DEBUG(x, a) #x 0 : { *(x) }
> >> +#define DECL_DEBUG2(x, y, a) #x 0 : { *(x) *(y) }
> > 
> > Would it be helpful to place those in a 
> 
> ... you may have had a 3rd one?

Oh, no, I just forgot to remove this. I was going to ask whether we
could place those in something akin to a PT_NOLOAD program section,
but that doesn't exist AFAICT (and even if possible would require
adjustments to mkelf32).

> 
> >> @@ -179,6 +188,13 @@ SECTIONS
> >>  #endif
> >>  #endif
> >>  
> >> +#ifndef EFI
> >> +  /* Retain these just for the purpose of possible analysis tools. */
> >> +  DECL_SECTION(.note) {
> >> +       *(.note.*)
> >> +  } PHDR(note) PHDR(text)
> > 
> > Wouldn't it be enough to place it in the note program header?
> > 
> > The buildid note is already placed in .rodata, so any remaining notes
> > don't need to be in a LOAD section?
> 
> All the notes will be covered by the NOTE phdr. I had this much later
> in the script originally, but then the NOTE phdr covered large parts of
> .init.*. Clearly that yields invalid notes, which analysis (or simple
> dumping) tools wouldn't be happy about. We might be able to add 2nd
> NOTE phdr, but mkelf32 assumes exactly 2 phdrs if it finds more than
> one, so changes there would likely be needed then (which I'd like to
> avoid for the moment). I'm also not sure in how far tools can be
> expected to look for multiple NOTE phdrs ...

But if we are adding a .note section now we might as well merge it
with .note.gnu.build-id:

  DECL_SECTION(.note) {
       __note_gnu_build_id_start = .;
       *(.note.gnu.build-id)
       __note_gnu_build_id_end = .;
       *(.note.*)
  } PHDR(note) PHDR(text)

And drop the .note.Xen section?

> >> +#endif
> >> +
> >>    _erodata = .;
> >>  
> >>    . = ALIGN(SECTION_ALIGN);
> >> @@ -266,6 +282,32 @@ SECTIONS
> >>         __ctors_end = .;
> >>    } PHDR(text)
> >>  
> >> +#ifndef EFI
> >> +  /*
> >> +   * With --orphan-sections=warn (or =error) we need to handle certain 
> >> linker
> >> +   * generated sections. These are all expected to be empty; respective
> >> +   * ASSERT()s can be found towards the end of this file.
> >> +   */
> >> +  DECL_SECTION(.got) {
> >> +       *(.got)
> >> +  } PHDR(text)
> >> +  DECL_SECTION(.got.plt) {
> >> +       *(.got.plt)
> >> +  } PHDR(text)
> >> +  DECL_SECTION(.igot.plt) {
> >> +       *(.igot.plt)
> >> +  } PHDR(text)
> >> +  DECL_SECTION(.iplt) {
> >> +       *(.iplt)
> >> +  } PHDR(text)
> >> +  DECL_SECTION(.plt) {
> >> +       *(.plt)
> >> +  } PHDR(text)
> >> +  DECL_SECTION(.rela) {
> >> +       *(.rela.*)
> >> +  } PHDR(text)
> > 
> > Why do you need to explicitly place those in the text program header?
> 
> I guess that's largely for consistency with all other directives. With the
> assertions that these need to be empty, we might get away without, as long
> as no linker would decide to set up another zero-size phdr for them.

We already set the debug sections to not be part of any program header
and seem to get away with it. I'm not sure how different the sections
handled below would be, linkers might indeed want to place them
regardless?

If so it might be good to add a comment that while those should be
empty (and thus don't end up in any program header) we assign them to
the text one in order to avoid the linker from creating a new program
header for them.

Thanks, Roger.