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

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

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.

> ---
> v2: Don't use (NOLOAD) for ELF; it has undocumented (and unhelpful)
>     behavior with GNU ld there. Also place .{sym,str,shstr}tab for LLVM
>     ld.
> ---
> 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.
> 
> --- 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
> +
>  $(TARGET): TMP = $(@D)/.$(@F).elf32
>  $(TARGET): $(TARGET)-syms $(efi-y) $(obj)/boot/mkelf32
>       $(obj)/boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) 
> $(XEN_IMG_OFFSET) \
> @@ -146,7 +148,7 @@ $(TARGET)-syms: $(BASEDIR)/prelink.o $(o
>               >$(@D)/.$(@F).1.S
>       $(MAKE) $(build)=$(@D) $(@D)/.$(@F).1.o
>       $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \
> -         $(@D)/.$(@F).1.o -o $@
> +         $(orphan-handling-y) $(@D)/.$(@F).1.o -o $@
>       $(NM) -pa --format=sysv $(@D)/$(@F) \
>               | $(BASEDIR)/tools/symbols --all-symbols --xensyms --sysv 
> --sort \
>               >$(@D)/$(@F).map
> @@ -220,7 +222,7 @@ endif
>               | $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort 
> >$(@D)/.$(@F).1s.S
>       $(MAKE) $(build)=$(@D) .$(@F).1r.o .$(@F).1s.o
>       $(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T $(obj)/efi.lds -N $< \
> -                     $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o $(note_file_option) 
> -o $@
> +           $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o $(orphan-handling-y) 
> $(note_file_option) -o $@
>       $(NM) -pa --format=sysv $(@D)/$(@F) \
>               | $(BASEDIR)/tools/symbols --all-symbols --xensyms --sysv 
> --sort >$(@D)/$(@F).map
>       rm -f $(@D)/.$(@F).[0-9]* $(@D)/..$(@F).[0-9]*
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -12,6 +12,13 @@
>  #undef __XEN_VIRT_START
>  #define __XEN_VIRT_START __image_base__
>  #define DECL_SECTION(x) x :
> +/*
> + * Use the NOLOAD directive, despite currently ignored by (at least) GNU ld
> + * for PE output, in order to record that we'd prefer these sections to not
> + * be loaded into memory.
> + */
> +#define DECL_DEBUG(x, a) #x ALIGN(a) (NOLOAD) : { *(x) }
> +#define DECL_DEBUG2(x, y, a) #x ALIGN(a) (NOLOAD) : { *(x) *(y) }
>  
>  ENTRY(efi_start)
>  
> @@ -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 

>  
>  ENTRY(start_pa)
>  
> @@ -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?

> +#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?

Thanks, Roger.