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@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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) }
 
 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)
+#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)
+#endif
+
   . = ALIGN(SECTION_ALIGN);
   __init_end = .;
   __2M_init_end = .;
@@ -320,71 +362,6 @@ SECTIONS
     *(.reloc)
     __base_relocs_end = .;
   }
-  /*
-   * Explicitly list debug section for the PE output so that they don't end
-   * up at VA 0 which is below image base and thus invalid. Also use the
-   * NOLOAD directive, despite currently ignored by ld for PE output, in
-   * order to record that we'd prefer these sections to not be loaded into
-   * memory.
-   *
-   * Note that we're past _end here, so if these sections get loaded they'll
-   * be discarded at runtime anyway.
-   */
-  .debug_abbrev ALIGN(1) (NOLOAD) : {
-     *(.debug_abbrev)
-  }
-  .debug_info ALIGN(1) (NOLOAD) : {
-    *(.debug_info)
-    *(.gnu.linkonce.wi.*)
-  }
-  .debug_types ALIGN(1) (NOLOAD) : {
-    *(.debug_types)
-  }
-  .debug_str ALIGN(1) (NOLOAD) : {
-    *(.debug_str)
-  }
-  .debug_line ALIGN(1) (NOLOAD) : {
-    *(.debug_line)
-    *(.debug_line.*)
-  }
-  .debug_line_str ALIGN(1) (NOLOAD) : {
-    *(.debug_line_str)
-  }
-  .debug_names ALIGN(4) (NOLOAD) : {
-    *(.debug_names)
-  }
-  .debug_frame ALIGN(4) (NOLOAD) : {
-    *(.debug_frame)
-  }
-  .debug_loc ALIGN(1) (NOLOAD) : {
-    *(.debug_loc)
-  }
-  .debug_loclists ALIGN(4) (NOLOAD) : {
-    *(.debug_loclists)
-  }
-  .debug_ranges ALIGN(8) (NOLOAD) : {
-    *(.debug_ranges)
-  }
-  .debug_rnglists ALIGN(4) (NOLOAD) : {
-    *(.debug_rnglists)
-  }
-  .debug_addr ALIGN(8) (NOLOAD) : {
-    *(.debug_addr)
-  }
-  .debug_aranges ALIGN(1) (NOLOAD) : {
-    *(.debug_aranges)
-  }
-  .debug_pubnames ALIGN(1) (NOLOAD) : {
-    *(.debug_pubnames)
-  }
-  .debug_pubtypes ALIGN(1) (NOLOAD) : {
-    *(.debug_pubtypes)
-  }
-  /* Trick the linker into setting the image size to no less than 16Mb. */
-  __image_end__ = .;
-  .pad ALIGN(__section_alignment__) : {
-    . = __image_end__ < __image_base__ + MB(16) ? ALIGN(MB(16)) : .;
-  }
 #elif defined(XEN_BUILD_EFI)
   /*
    * Due to the way EFI support is currently implemented, these two symbols
@@ -399,6 +376,42 @@ SECTIONS
   efi = .;
 #endif
 
+  /*
+   * Explicitly list debug sections, first of all to avoid these sections being
+   * viewed as "orphan" by the linker.
+   *
+   * For the PE output this is further necessary so that they don't end up at
+   * VA 0, which is below image base and thus invalid. Note that we're past
+   * _end here, so if these sections get loaded they'll be discarded at runtime
+   * anyway.
+   */
+  DECL_DEBUG(.debug_abbrev, 1)
+  DECL_DEBUG2(.debug_info, .gnu.linkonce.wi.*, 1)
+  DECL_DEBUG(.debug_types, 1)
+  DECL_DEBUG(.debug_str, 1)
+  DECL_DEBUG2(.debug_line, .debug_line.*, 1)
+  DECL_DEBUG(.debug_line_str, 1)
+  DECL_DEBUG(.debug_names, 4)
+  DECL_DEBUG(.debug_frame, 4)
+  DECL_DEBUG(.debug_loc, 1)
+  DECL_DEBUG(.debug_loclists, 4)
+  DECL_DEBUG(.debug_macinfo, 1)
+  DECL_DEBUG(.debug_macro, 1)
+  DECL_DEBUG(.debug_ranges, 8)
+  DECL_DEBUG(.debug_rnglists, 4)
+  DECL_DEBUG(.debug_addr, 8)
+  DECL_DEBUG(.debug_aranges, 1)
+  DECL_DEBUG(.debug_pubnames, 1)
+  DECL_DEBUG(.debug_pubtypes, 1)
+
+#ifdef EFI
+  /* Trick the linker into setting the image size to no less than 16Mb. */
+  __image_end__ = .;
+  .pad ALIGN(__section_alignment__) : {
+    . = __image_end__ < __image_base__ + MB(16) ? ALIGN(MB(16)) : .;
+  }
+#endif
+
 #ifdef CONFIG_HYPERV_GUEST
   hv_hcall_page = ABSOLUTE(HV_HCALL_PAGE - XEN_VIRT_START + __XEN_VIRT_START);
 #endif
@@ -419,8 +432,7 @@ SECTIONS
 #ifdef EFI
        *(.comment)
        *(.comment.*)
-       *(.note.Xen)
-       *(.note.gnu.*)
+       *(.note.*)
 #endif
   }
 
@@ -433,6 +445,13 @@ SECTIONS
   .stab.index 0 : { *(.stab.index) }
   .stab.indexstr 0 : { *(.stab.indexstr) }
   .comment 0 : { *(.comment) }
+  /*
+   * LLVM ld also wants .symtab, .strtab, and .shstrtab placed. These look to
+   * be benign to GNU ld, so we can have them here unconditionally.
+   */
+  .symtab 0 : { *(.symtab) }
+  .strtab 0 : { *(.strtab) }
+  .shstrtab 0 : { *(.shstrtab) }
 #endif
 }
 
@@ -466,6 +485,15 @@ ASSERT(IS_ALIGNED(trampoline_end,   4),
 ASSERT(IS_ALIGNED(__bss_start,      8), "__bss_start misaligned")
 ASSERT(IS_ALIGNED(__bss_end,        8), "__bss_end misaligned")
 
+#ifndef EFI
+ASSERT(!SIZEOF(.got),      ".got non-empty")
+ASSERT(!SIZEOF(.got.plt),  ".got.plt non-empty")
+ASSERT(!SIZEOF(.igot.plt), ".igot.plt non-empty")
+ASSERT(!SIZEOF(.iplt),     ".iplt non-empty")
+ASSERT(!SIZEOF(.plt),      ".plt non-empty")
+ASSERT(!SIZEOF(.rela),     "leftover relocations")
+#endif
+
 ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN,
     "not enough room for trampoline and mbi data")
 ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,

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@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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.

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@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> 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?

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

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

Jan

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@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > 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.

On 08.03.2022 13:11, Roger Pau Monné wrote:
> 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:
>>>> @@ -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?

In an ideal world we likely could, yes. But do we know for sure that
nothing recognizes the Xen notes by section name? .note.gnu.build-id
cannot be folded in any event - see the rule for generating note.o,
to be used by xen.efi linking in certain cases.

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

Simply because I don't know I'd like to be on the safe side. Debug sections
can't really be taken as reference: At least GNU ld heavily special-cases
them anyway.

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

I'll add a sentence to the comment I'm already adding here.

Jan

On Tue, Mar 08, 2022 at 01:34:06PM +0100, Jan Beulich wrote:
> On 08.03.2022 13:11, Roger Pau Monné wrote:
> > 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:
> >>>> @@ -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?
> 
> In an ideal world we likely could, yes. But do we know for sure that
> nothing recognizes the Xen notes by section name?

Wouldn't that be wrong? In the elfnotes.h file it's clearly specified
that Xen notes live in a PT_NOTE program header and have 'Xen' in the
name field. There's no requirement of them being in any specific
section.

> .note.gnu.build-id
> cannot be folded in any event - see the rule for generating note.o,
> to be used by xen.efi linking in certain cases.

Right, so we need to keep the .note.gnu.build-id section, but we could
likely fold .note.Xen into .note I think?

Or at least add a comment to mention that we don't want to fold
.note.Xen into .note in case there are tools that search for specific
Xen notes to be contained in .note.Xen.

> >>>> +#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?
> 
> Simply because I don't know I'd like to be on the safe side. Debug sections
> can't really be taken as reference: At least GNU ld heavily special-cases
> them anyway.
> 
> > 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.
> 
> I'll add a sentence to the comment I'm already adding here.

Thanks, Roger.

On 08.03.2022 15:07, Roger Pau Monné wrote:
> On Tue, Mar 08, 2022 at 01:34:06PM +0100, Jan Beulich wrote:
>> On 08.03.2022 13:11, Roger Pau Monné wrote:
>>> 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:
>>>>>> @@ -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?
>>
>> In an ideal world we likely could, yes. But do we know for sure that
>> nothing recognizes the Xen notes by section name?
> 
> Wouldn't that be wrong? In the elfnotes.h file it's clearly specified
> that Xen notes live in a PT_NOTE program header and have 'Xen' in the
> name field. There's no requirement of them being in any specific
> section.

True. But ELF also tells us to not go from section names (only), but
to consider type and attribute as well. Yet what do most tools do?

>> .note.gnu.build-id
>> cannot be folded in any event - see the rule for generating note.o,
>> to be used by xen.efi linking in certain cases.
> 
> Right, so we need to keep the .note.gnu.build-id section, but we could
> likely fold .note.Xen into .note I think?
> 
> Or at least add a comment to mention that we don't want to fold
> .note.Xen into .note in case there are tools that search for specific
> Xen notes to be contained in .note.Xen.

I can add such a comment, sure.

Jan