| Message ID | 289684f6-fa73-bf02-137c-680ad8891640@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] x86/build: use --orphan-handling linker option if available | expand |
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
--- 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,
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.