| Message ID | 96cb4039-5f43-8eac-d855-c9f1587ed2d0@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/EFI: build adjustments | expand |
On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote: > ... provided the linker supports it (which it does as of commit > 2dfa8341e079 ["ELF DWARF in PE output"]). > > Without mentioning debugging sections, the linker would put them at > VA 0, thus making them unreachable by 32-bit (relative or absolute) > relocations. If relocations were resolvable (or absent) the resulting > binary would have invalid section RVAs (0 - __image_base__, truncated to > 32 bits). Mentioning debugging sections without specifying an address > will result in the linker putting them all on the same RVA. A loader is, > afaict, free to reject loading such an image, as sections shouldn't > overlap. (The above describes GNU ld 2.36 behavior, which - if deemed > buggy - could change.) Isn't just using (NOLOAD) to signal those sections shouldn't be loaded enough, and thus don't care about it's RVA? > > Make sure our up-to-16Mb padding doesn't unnecessarily further extend > the image. > > Take the opportunity and also switch to using $(call ld-option,...). > > Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > This way we could also avoid discarding .comment for xen.efi. > > I'd like to point out that the linking of the debug info takes far > longer than the linking of the "normal" parts of the image. The change > therefore has the downside of slowing down debug builds. > > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -126,8 +126,14 @@ export XEN_BUILD_EFI := $(shell $(CC) $( > CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI > > # Check if the linker supports PE. > -EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(XEN_LDFLAGS)) --subsystem=10 --strip-debug > -XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(shell $(LD) $(EFI_LDFLAGS) -o efi/check.efi efi/check.o 2>/dev/null && echo y)) > +EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(XEN_LDFLAGS)) --subsystem=10 > +XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(call ld-option,$(EFI_LDFLAGS) --image-base=0x100000000 -o efi/check.efi efi/check.o)) > +# If the above failed, it may be merely because of the linker not dealing well > +# with debug info. Try again with stripping it. > +ifeq ($(CONFIG_DEBUG_INFO)-$(XEN_BUILD_PE),y-n) > +EFI_LDFLAGS += --strip-debug > +XEN_BUILD_PE := $(call ld-option,$(EFI_LDFLAGS) --image-base=0x100000000 -o efi/check.efi efi/check.o) > +endif > > ifeq ($(XEN_BUILD_PE),y) > > @@ -232,6 +238,9 @@ note_file_option ?= $(note_file) > > ifeq ($(XEN_BUILD_PE),y) > $(TARGET).efi: prelink.o $(note_file) efi.lds efi/relocs-dummy.o efi/mkreloc > +ifeq ($(CONFIG_DEBUG_INFO),y) > + $(if $(filter --strip-debug,$(EFI_LDFLAGS)),echo,:) "Will strip debug info from $(@F)" > +endif > $(foreach base, $(VIRT_BASE) $(ALT_BASE), \ > $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< $(relocs-dummy) \ > $(BASEDIR)/common/symbols-dummy.o $(note_file_option) -o $(@D)/.$(@F).$(base).0 &&) : > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -312,10 +312,60 @@ SECTIONS > *(.reloc) > __base_relocs_end = .; > } > - /* Trick the linker into setting the image size to exactly 16Mb. */ > - . = ALIGN(__section_alignment__); > - DECL_SECTION(.pad) { > - . = ALIGN(MB(16)); > + .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)) : .; I think this is inside an ifdef EFI region, since this is DWARF info couldn't it also be present when building with EFI disabled? Maybe I'm missing part of the point here, sorry. Thanks, Roger.
On 21.04.2021 13:15, Roger Pau Monné wrote: > On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote: >> ... provided the linker supports it (which it does as of commit >> 2dfa8341e079 ["ELF DWARF in PE output"]). >> >> Without mentioning debugging sections, the linker would put them at >> VA 0, thus making them unreachable by 32-bit (relative or absolute) >> relocations. If relocations were resolvable (or absent) the resulting >> binary would have invalid section RVAs (0 - __image_base__, truncated to >> 32 bits). Mentioning debugging sections without specifying an address >> will result in the linker putting them all on the same RVA. A loader is, >> afaict, free to reject loading such an image, as sections shouldn't >> overlap. (The above describes GNU ld 2.36 behavior, which - if deemed >> buggy - could change.) > > Isn't just using (NOLOAD) to signal those sections shouldn't be > loaded enough, and thus don't care about it's RVA? As said in a reply earlier on another sub-thread, (NOLOAD) is meaningless for PE. The fact that I add them nevertheless is just for docs purposes (or if, in the future, the item gains significance). The main problem though isn't "load" vs "no-load" but, as I thought I have expressed in the description, that there's no "don't care about it's RVA" in PE. All sections have to have a non-zero VA above the image base. This is the only way via which sane RVA values can result. If sections get placed at VA 0 (which is perfectly fine for ELF), the RVA would be a huge negative number truncated to 32 bits. (Again, prior to binutils 2.37 this will go all silently.) Plus the respective sections would come first (rather than last) in the binary (which by itself may or may not be a problem for the EFI loader, but I wouldn't want to chance it). >> --- a/xen/arch/x86/xen.lds.S >> +++ b/xen/arch/x86/xen.lds.S >> @@ -312,10 +312,60 @@ SECTIONS >> *(.reloc) >> __base_relocs_end = .; >> } >> - /* Trick the linker into setting the image size to exactly 16Mb. */ >> - . = ALIGN(__section_alignment__); >> - DECL_SECTION(.pad) { >> - . = ALIGN(MB(16)); >> + .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)) : .; > > I think this is inside an ifdef EFI region, since this is DWARF info > couldn't it also be present when building with EFI disabled? Of course (and it's not just "could" but "will"), yet the linker will do fine (and perhaps even better) without when building ELF. Also note that we'll be responsible for keeping the list of sections up-to- date. The linker will recognize Dwarf sections by looking for a .debug_ prefix. We can't use such here (or at least I'm not aware of a suitable mechanism); .debug_* would mean munging together all the different kinds of Dwarf sections. Hence by limiting the explicit enumeration to PE, I'm trying to avoid anomalies in ELF down the road. Jan
On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote: > On 21.04.2021 13:15, Roger Pau Monné wrote: > > On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote: > >> ... provided the linker supports it (which it does as of commit > >> 2dfa8341e079 ["ELF DWARF in PE output"]). > >> > >> Without mentioning debugging sections, the linker would put them at > >> VA 0, thus making them unreachable by 32-bit (relative or absolute) > >> relocations. If relocations were resolvable (or absent) the resulting > >> binary would have invalid section RVAs (0 - __image_base__, truncated to > >> 32 bits). Mentioning debugging sections without specifying an address > >> will result in the linker putting them all on the same RVA. A loader is, > >> afaict, free to reject loading such an image, as sections shouldn't > >> overlap. (The above describes GNU ld 2.36 behavior, which - if deemed > >> buggy - could change.) > > > > Isn't just using (NOLOAD) to signal those sections shouldn't be > > loaded enough, and thus don't care about it's RVA? > > As said in a reply earlier on another sub-thread, (NOLOAD) is meaningless > for PE. The fact that I add them nevertheless is just for docs purposes > (or if, in the future, the item gains significance). > > The main problem though isn't "load" vs "no-load" but, as I thought I > have expressed in the description, that there's no "don't care about it's > RVA" in PE. All sections have to have a non-zero VA above the image base. > This is the only way via which sane RVA values can result. If sections > get placed at VA 0 (which is perfectly fine for ELF), the RVA would be a > huge negative number truncated to 32 bits. (Again, prior to binutils 2.37 > this will go all silently.) Plus the respective sections would come first > (rather than last) in the binary (which by itself may or may not be a > problem for the EFI loader, but I wouldn't want to chance it). Thanks for the explanation. > >> --- a/xen/arch/x86/xen.lds.S > >> +++ b/xen/arch/x86/xen.lds.S > >> @@ -312,10 +312,60 @@ SECTIONS > >> *(.reloc) > >> __base_relocs_end = .; > >> } > >> - /* Trick the linker into setting the image size to exactly 16Mb. */ > >> - . = ALIGN(__section_alignment__); > >> - DECL_SECTION(.pad) { > >> - . = ALIGN(MB(16)); > >> + .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)) : .; > > > > I think this is inside an ifdef EFI region, since this is DWARF info > > couldn't it also be present when building with EFI disabled? > > Of course (and it's not just "could" but "will"), yet the linker will > do fine (and perhaps even better) without when building ELF. Also > note that we'll be responsible for keeping the list of sections up-to- > date. The linker will recognize Dwarf sections by looking for a > .debug_ prefix. We can't use such here (or at least I'm not aware of > a suitable mechanism); .debug_* would mean munging together all the > different kinds of Dwarf sections. Hence by limiting the explicit > enumeration to PE, I'm trying to avoid anomalies in ELF down the road. Right, so we will have to keep this list of debug_ sections updated manually if/when more of those appear as part of DWARF updates? Do we have a way to get some kind of warning or error when a new section not explicitly handled here appears? Instead of adding DWARF debug info to the xen.efi binary, is there a way to translate the DWARF from the ELF build into the native debug format for PE/COFF? Thanks, Roger.
On 21.04.2021 17:30, Roger Pau Monné wrote: > On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote: >> On 21.04.2021 13:15, Roger Pau Monné wrote: >>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote: >>>> --- a/xen/arch/x86/xen.lds.S >>>> +++ b/xen/arch/x86/xen.lds.S >>>> @@ -312,10 +312,60 @@ SECTIONS >>>> *(.reloc) >>>> __base_relocs_end = .; >>>> } >>>> - /* Trick the linker into setting the image size to exactly 16Mb. */ >>>> - . = ALIGN(__section_alignment__); >>>> - DECL_SECTION(.pad) { >>>> - . = ALIGN(MB(16)); >>>> + .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)) : .; >>> >>> I think this is inside an ifdef EFI region, since this is DWARF info >>> couldn't it also be present when building with EFI disabled? >> >> Of course (and it's not just "could" but "will"), yet the linker will >> do fine (and perhaps even better) without when building ELF. Also >> note that we'll be responsible for keeping the list of sections up-to- >> date. The linker will recognize Dwarf sections by looking for a >> .debug_ prefix. We can't use such here (or at least I'm not aware of >> a suitable mechanism); .debug_* would mean munging together all the >> different kinds of Dwarf sections. Hence by limiting the explicit >> enumeration to PE, I'm trying to avoid anomalies in ELF down the road. > > Right, so we will have to keep this list of debug_ sections updated > manually if/when more of those appear as part of DWARF updates? Yes. > Do we have a way to get some kind of warning or error when a new > section not explicitly handled here appears? ld 2.37 will start warning about such sections, as they'd land at VA 0 and hence below image base. > Instead of adding DWARF debug info to the xen.efi binary, is there a > way to translate the DWARF from the ELF build into the native debug > format for PE/COFF? I'm unaware of a single, "native" debug format (Dwarf also isn't specifically tied to ELF). I further don't think all of the Dwarf bits could reasonably be expressed in another debug format. Jan
On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote: > On 21.04.2021 17:30, Roger Pau Monné wrote: > > On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote: > >> On 21.04.2021 13:15, Roger Pau Monné wrote: > >>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote: > >>>> --- a/xen/arch/x86/xen.lds.S > >>>> +++ b/xen/arch/x86/xen.lds.S > >>>> @@ -312,10 +312,60 @@ SECTIONS > >>>> *(.reloc) > >>>> __base_relocs_end = .; > >>>> } > >>>> - /* Trick the linker into setting the image size to exactly 16Mb. */ > >>>> - . = ALIGN(__section_alignment__); > >>>> - DECL_SECTION(.pad) { > >>>> - . = ALIGN(MB(16)); > >>>> + .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)) : .; > >>> > >>> I think this is inside an ifdef EFI region, since this is DWARF info > >>> couldn't it also be present when building with EFI disabled? > >> > >> Of course (and it's not just "could" but "will"), yet the linker will > >> do fine (and perhaps even better) without when building ELF. Also > >> note that we'll be responsible for keeping the list of sections up-to- > >> date. The linker will recognize Dwarf sections by looking for a > >> .debug_ prefix. We can't use such here (or at least I'm not aware of > >> a suitable mechanism); .debug_* would mean munging together all the > >> different kinds of Dwarf sections. Hence by limiting the explicit > >> enumeration to PE, I'm trying to avoid anomalies in ELF down the road. > > > > Right, so we will have to keep this list of debug_ sections updated > > manually if/when more of those appear as part of DWARF updates? > > Yes. > > > Do we have a way to get some kind of warning or error when a new > > section not explicitly handled here appears? > > ld 2.37 will start warning about such sections, as they'd land at > VA 0 and hence below image base. That seems like a bug in ld? The '--image-base' option description mentions: "This is the lowest memory location that will be used when your program or dll is loaded.", so I would expect that if the option is used the default VA should be >= image-base, or else the description of the option is not consistent, as ld will still place sections at addresses below image-base. Thanks, Roger.
On 22.04.2021 10:14, Roger Pau Monné wrote: > On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote: >> On 21.04.2021 17:30, Roger Pau Monné wrote: >>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote: >>>> On 21.04.2021 13:15, Roger Pau Monné wrote: >>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote: >>>>>> --- a/xen/arch/x86/xen.lds.S >>>>>> +++ b/xen/arch/x86/xen.lds.S >>>>>> @@ -312,10 +312,60 @@ SECTIONS >>>>>> *(.reloc) >>>>>> __base_relocs_end = .; >>>>>> } >>>>>> - /* Trick the linker into setting the image size to exactly 16Mb. */ >>>>>> - . = ALIGN(__section_alignment__); >>>>>> - DECL_SECTION(.pad) { >>>>>> - . = ALIGN(MB(16)); >>>>>> + .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)) : .; >>>>> >>>>> I think this is inside an ifdef EFI region, since this is DWARF info >>>>> couldn't it also be present when building with EFI disabled? >>>> >>>> Of course (and it's not just "could" but "will"), yet the linker will >>>> do fine (and perhaps even better) without when building ELF. Also >>>> note that we'll be responsible for keeping the list of sections up-to- >>>> date. The linker will recognize Dwarf sections by looking for a >>>> .debug_ prefix. We can't use such here (or at least I'm not aware of >>>> a suitable mechanism); .debug_* would mean munging together all the >>>> different kinds of Dwarf sections. Hence by limiting the explicit >>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road. >>> >>> Right, so we will have to keep this list of debug_ sections updated >>> manually if/when more of those appear as part of DWARF updates? >> >> Yes. >> >>> Do we have a way to get some kind of warning or error when a new >>> section not explicitly handled here appears? >> >> ld 2.37 will start warning about such sections, as they'd land at >> VA 0 and hence below image base. > > That seems like a bug in ld? > > The '--image-base' option description mentions: "This is the lowest > memory location that will be used when your program or dll is > loaded.", so I would expect that if the option is used the default VA > should be >= image-base, or else the description of the option is not > consistent, as ld will still place sections at addresses below > image-base. ld's "general" logic is pretty ELF-centric. Hence debugging sections get placed at VA 0 by default, not matter what the (PE-specific) --image-base says. Whether that's a bug though I'm not sure: There are no really good alternatives that could be used by default. Doing what we do here (and what e.g. Cygwin does) via linker script may not be appropriate in the common case. In particular it is not generally correct for debug info to be part of what gets loaded into memory. Microsoft's CodeView / PDB debug info gets represented differently, for example, such that it can be part of the file, but wouldn't get loaded. But that's a vendor's choice, again not something that's necessarily generalizable. Jan
On Thu, Apr 22, 2021 at 01:03:13PM +0200, Jan Beulich wrote: > On 22.04.2021 10:14, Roger Pau Monné wrote: > > On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote: > >> On 21.04.2021 17:30, Roger Pau Monné wrote: > >>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote: > >>>> On 21.04.2021 13:15, Roger Pau Monné wrote: > >>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote: > >>>>>> --- a/xen/arch/x86/xen.lds.S > >>>>>> +++ b/xen/arch/x86/xen.lds.S > >>>>>> @@ -312,10 +312,60 @@ SECTIONS > >>>>>> *(.reloc) > >>>>>> __base_relocs_end = .; > >>>>>> } > >>>>>> - /* Trick the linker into setting the image size to exactly 16Mb. */ > >>>>>> - . = ALIGN(__section_alignment__); > >>>>>> - DECL_SECTION(.pad) { > >>>>>> - . = ALIGN(MB(16)); > >>>>>> + .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)) : .; > >>>>> > >>>>> I think this is inside an ifdef EFI region, since this is DWARF info > >>>>> couldn't it also be present when building with EFI disabled? > >>>> > >>>> Of course (and it's not just "could" but "will"), yet the linker will > >>>> do fine (and perhaps even better) without when building ELF. Also > >>>> note that we'll be responsible for keeping the list of sections up-to- > >>>> date. The linker will recognize Dwarf sections by looking for a > >>>> .debug_ prefix. We can't use such here (or at least I'm not aware of > >>>> a suitable mechanism); .debug_* would mean munging together all the > >>>> different kinds of Dwarf sections. Hence by limiting the explicit > >>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road. > >>> > >>> Right, so we will have to keep this list of debug_ sections updated > >>> manually if/when more of those appear as part of DWARF updates? > >> > >> Yes. > >> > >>> Do we have a way to get some kind of warning or error when a new > >>> section not explicitly handled here appears? > >> > >> ld 2.37 will start warning about such sections, as they'd land at > >> VA 0 and hence below image base. > > > > That seems like a bug in ld? > > > > The '--image-base' option description mentions: "This is the lowest > > memory location that will be used when your program or dll is > > loaded.", so I would expect that if the option is used the default VA > > should be >= image-base, or else the description of the option is not > > consistent, as ld will still place sections at addresses below > > image-base. > > ld's "general" logic is pretty ELF-centric. Hence debugging sections > get placed at VA 0 by default, not matter what the (PE-specific) > --image-base says. Whether that's a bug though I'm not sure: There > are no really good alternatives that could be used by default. Doing > what we do here (and what e.g. Cygwin does) via linker script may not > be appropriate in the common case. In particular it is not generally > correct for debug info to be part of what gets loaded into memory. So with this change here you placate the warnings from newer ld about having a VA < image base, but the end result is that now the debug sections will also get loaded when booted from the EFI loader? (because the NOLOAD doesn't have any effect with PE) Thanks, Roger.
On 22.04.2021 16:56, Roger Pau Monné wrote: > On Thu, Apr 22, 2021 at 01:03:13PM +0200, Jan Beulich wrote: >> On 22.04.2021 10:14, Roger Pau Monné wrote: >>> On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote: >>>> On 21.04.2021 17:30, Roger Pau Monné wrote: >>>>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote: >>>>>> On 21.04.2021 13:15, Roger Pau Monné wrote: >>>>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote: >>>>>>>> --- a/xen/arch/x86/xen.lds.S >>>>>>>> +++ b/xen/arch/x86/xen.lds.S >>>>>>>> @@ -312,10 +312,60 @@ SECTIONS >>>>>>>> *(.reloc) >>>>>>>> __base_relocs_end = .; >>>>>>>> } >>>>>>>> - /* Trick the linker into setting the image size to exactly 16Mb. */ >>>>>>>> - . = ALIGN(__section_alignment__); >>>>>>>> - DECL_SECTION(.pad) { >>>>>>>> - . = ALIGN(MB(16)); >>>>>>>> + .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)) : .; >>>>>>> >>>>>>> I think this is inside an ifdef EFI region, since this is DWARF info >>>>>>> couldn't it also be present when building with EFI disabled? >>>>>> >>>>>> Of course (and it's not just "could" but "will"), yet the linker will >>>>>> do fine (and perhaps even better) without when building ELF. Also >>>>>> note that we'll be responsible for keeping the list of sections up-to- >>>>>> date. The linker will recognize Dwarf sections by looking for a >>>>>> .debug_ prefix. We can't use such here (or at least I'm not aware of >>>>>> a suitable mechanism); .debug_* would mean munging together all the >>>>>> different kinds of Dwarf sections. Hence by limiting the explicit >>>>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road. >>>>> >>>>> Right, so we will have to keep this list of debug_ sections updated >>>>> manually if/when more of those appear as part of DWARF updates? >>>> >>>> Yes. >>>> >>>>> Do we have a way to get some kind of warning or error when a new >>>>> section not explicitly handled here appears? >>>> >>>> ld 2.37 will start warning about such sections, as they'd land at >>>> VA 0 and hence below image base. >>> >>> That seems like a bug in ld? >>> >>> The '--image-base' option description mentions: "This is the lowest >>> memory location that will be used when your program or dll is >>> loaded.", so I would expect that if the option is used the default VA >>> should be >= image-base, or else the description of the option is not >>> consistent, as ld will still place sections at addresses below >>> image-base. >> >> ld's "general" logic is pretty ELF-centric. Hence debugging sections >> get placed at VA 0 by default, not matter what the (PE-specific) >> --image-base says. Whether that's a bug though I'm not sure: There >> are no really good alternatives that could be used by default. Doing >> what we do here (and what e.g. Cygwin does) via linker script may not >> be appropriate in the common case. In particular it is not generally >> correct for debug info to be part of what gets loaded into memory. > > So with this change here you placate the warnings from newer ld about > having a VA < image base, It's not just about silencing the warnings. The resulting image is unusable when the sections don't get placed at a suitable VA. > but the end result is that now the debug > sections will also get loaded when booted from the EFI loader? > (because the NOLOAD doesn't have any effect with PE) Yes. I currently see no other way to retain debug info in xen.efi. But to be clear, the memory debug info occupies isn't lost - we still free space from _end (or alike) onwards. .reloc, for example, also lives there. And I was wondering whether we shouldn't keep .comment this way as well. Jan
On Thu, Apr 22, 2021 at 05:46:28PM +0200, Jan Beulich wrote: > On 22.04.2021 16:56, Roger Pau Monné wrote: > > On Thu, Apr 22, 2021 at 01:03:13PM +0200, Jan Beulich wrote: > >> On 22.04.2021 10:14, Roger Pau Monné wrote: > >>> On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote: > >>>> On 21.04.2021 17:30, Roger Pau Monné wrote: > >>>>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote: > >>>>>> On 21.04.2021 13:15, Roger Pau Monné wrote: > >>>>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote: > >>>>>>>> --- a/xen/arch/x86/xen.lds.S > >>>>>>>> +++ b/xen/arch/x86/xen.lds.S > >>>>>>>> @@ -312,10 +312,60 @@ SECTIONS > >>>>>>>> *(.reloc) > >>>>>>>> __base_relocs_end = .; > >>>>>>>> } > >>>>>>>> - /* Trick the linker into setting the image size to exactly 16Mb. */ > >>>>>>>> - . = ALIGN(__section_alignment__); > >>>>>>>> - DECL_SECTION(.pad) { > >>>>>>>> - . = ALIGN(MB(16)); > >>>>>>>> + .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)) : .; > >>>>>>> > >>>>>>> I think this is inside an ifdef EFI region, since this is DWARF info > >>>>>>> couldn't it also be present when building with EFI disabled? > >>>>>> > >>>>>> Of course (and it's not just "could" but "will"), yet the linker will > >>>>>> do fine (and perhaps even better) without when building ELF. Also > >>>>>> note that we'll be responsible for keeping the list of sections up-to- > >>>>>> date. The linker will recognize Dwarf sections by looking for a > >>>>>> .debug_ prefix. We can't use such here (or at least I'm not aware of > >>>>>> a suitable mechanism); .debug_* would mean munging together all the > >>>>>> different kinds of Dwarf sections. Hence by limiting the explicit > >>>>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road. > >>>>> > >>>>> Right, so we will have to keep this list of debug_ sections updated > >>>>> manually if/when more of those appear as part of DWARF updates? > >>>> > >>>> Yes. > >>>> > >>>>> Do we have a way to get some kind of warning or error when a new > >>>>> section not explicitly handled here appears? > >>>> > >>>> ld 2.37 will start warning about such sections, as they'd land at > >>>> VA 0 and hence below image base. > >>> > >>> That seems like a bug in ld? > >>> > >>> The '--image-base' option description mentions: "This is the lowest > >>> memory location that will be used when your program or dll is > >>> loaded.", so I would expect that if the option is used the default VA > >>> should be >= image-base, or else the description of the option is not > >>> consistent, as ld will still place sections at addresses below > >>> image-base. > >> > >> ld's "general" logic is pretty ELF-centric. Hence debugging sections > >> get placed at VA 0 by default, not matter what the (PE-specific) > >> --image-base says. Whether that's a bug though I'm not sure: There > >> are no really good alternatives that could be used by default. Doing > >> what we do here (and what e.g. Cygwin does) via linker script may not > >> be appropriate in the common case. In particular it is not generally > >> correct for debug info to be part of what gets loaded into memory. > > > > So with this change here you placate the warnings from newer ld about > > having a VA < image base, > > It's not just about silencing the warnings. The resulting image is > unusable when the sections don't get placed at a suitable VA. And this wasn't an issue before because the linker won't even attempt to place DWARF sections into a PE output. > > but the end result is that now the debug > > sections will also get loaded when booted from the EFI loader? > > (because the NOLOAD doesn't have any effect with PE) > > Yes. I currently see no other way to retain debug info in xen.efi. > But to be clear, the memory debug info occupies isn't lost - we > still free space from _end (or alike) onwards. .reloc, for example, > also lives there. And I was wondering whether we shouldn't keep > .comment this way as well. Yes, I already realized all this is past _end. I wonder however if the use of (NOLOAD) makes all this more confusing, such sections should only be present on the linker script used for the PE output, and then the (NOLOAD) doesn't make sense there? If so, I think the (NOLOAD) directive should be dropped, and a comment noting that the debug sections need to be manually added to the PE output in order to avoid them being placed at VA 0 would be helpful IMO, likely also mentioning that they would be loaded but discarded afterwards by Xen because they are all past _end. Thanks, Roger.
On 22.04.2021 17:53, Roger Pau Monné wrote: > On Thu, Apr 22, 2021 at 05:46:28PM +0200, Jan Beulich wrote: >> On 22.04.2021 16:56, Roger Pau Monné wrote: >>> On Thu, Apr 22, 2021 at 01:03:13PM +0200, Jan Beulich wrote: >>>> On 22.04.2021 10:14, Roger Pau Monné wrote: >>>>> On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote: >>>>>> On 21.04.2021 17:30, Roger Pau Monné wrote: >>>>>>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote: >>>>>>>> On 21.04.2021 13:15, Roger Pau Monné wrote: >>>>>>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote: >>>>>>>>>> --- a/xen/arch/x86/xen.lds.S >>>>>>>>>> +++ b/xen/arch/x86/xen.lds.S >>>>>>>>>> @@ -312,10 +312,60 @@ SECTIONS >>>>>>>>>> *(.reloc) >>>>>>>>>> __base_relocs_end = .; >>>>>>>>>> } >>>>>>>>>> - /* Trick the linker into setting the image size to exactly 16Mb. */ >>>>>>>>>> - . = ALIGN(__section_alignment__); >>>>>>>>>> - DECL_SECTION(.pad) { >>>>>>>>>> - . = ALIGN(MB(16)); >>>>>>>>>> + .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)) : .; >>>>>>>>> >>>>>>>>> I think this is inside an ifdef EFI region, since this is DWARF info >>>>>>>>> couldn't it also be present when building with EFI disabled? >>>>>>>> >>>>>>>> Of course (and it's not just "could" but "will"), yet the linker will >>>>>>>> do fine (and perhaps even better) without when building ELF. Also >>>>>>>> note that we'll be responsible for keeping the list of sections up-to- >>>>>>>> date. The linker will recognize Dwarf sections by looking for a >>>>>>>> .debug_ prefix. We can't use such here (or at least I'm not aware of >>>>>>>> a suitable mechanism); .debug_* would mean munging together all the >>>>>>>> different kinds of Dwarf sections. Hence by limiting the explicit >>>>>>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road. >>>>>>> >>>>>>> Right, so we will have to keep this list of debug_ sections updated >>>>>>> manually if/when more of those appear as part of DWARF updates? >>>>>> >>>>>> Yes. >>>>>> >>>>>>> Do we have a way to get some kind of warning or error when a new >>>>>>> section not explicitly handled here appears? >>>>>> >>>>>> ld 2.37 will start warning about such sections, as they'd land at >>>>>> VA 0 and hence below image base. >>>>> >>>>> That seems like a bug in ld? >>>>> >>>>> The '--image-base' option description mentions: "This is the lowest >>>>> memory location that will be used when your program or dll is >>>>> loaded.", so I would expect that if the option is used the default VA >>>>> should be >= image-base, or else the description of the option is not >>>>> consistent, as ld will still place sections at addresses below >>>>> image-base. >>>> >>>> ld's "general" logic is pretty ELF-centric. Hence debugging sections >>>> get placed at VA 0 by default, not matter what the (PE-specific) >>>> --image-base says. Whether that's a bug though I'm not sure: There >>>> are no really good alternatives that could be used by default. Doing >>>> what we do here (and what e.g. Cygwin does) via linker script may not >>>> be appropriate in the common case. In particular it is not generally >>>> correct for debug info to be part of what gets loaded into memory. >>> >>> So with this change here you placate the warnings from newer ld about >>> having a VA < image base, >> >> It's not just about silencing the warnings. The resulting image is >> unusable when the sections don't get placed at a suitable VA. > > And this wasn't an issue before because the linker won't even attempt > to place DWARF sections into a PE output. No, this wasn't an issue before since, for things to work, we simply had to uniformly strip debug info when linking xen.efi. And this is what Andrew said should change. I was initially opposed, until I saw that Cygwin does just this as well. >>> but the end result is that now the debug >>> sections will also get loaded when booted from the EFI loader? >>> (because the NOLOAD doesn't have any effect with PE) >> >> Yes. I currently see no other way to retain debug info in xen.efi. >> But to be clear, the memory debug info occupies isn't lost - we >> still free space from _end (or alike) onwards. .reloc, for example, >> also lives there. And I was wondering whether we shouldn't keep >> .comment this way as well. > > Yes, I already realized all this is past _end. > > I wonder however if the use of (NOLOAD) makes all this more confusing, > such sections should only be present on the linker script used for the > PE output, and then the (NOLOAD) doesn't make sense there? > > If so, I think the (NOLOAD) directive should be dropped, and a comment > noting that the debug sections need to be manually added to the PE > output in order to avoid them being placed at VA 0 would be helpful > IMO, likely also mentioning that they would be loaded but discarded > afterwards by Xen because they are all past _end. Earlier on (another sub-thread, maybe) I think I've already said that I'd like to keep (NOLOAD) both for documentation purposes and just in case the linker develops some smarts to actually translate it into anything sensible when linking PE. This is quite different from .reloc, after all - that section has to be loaded for our re- relocation to work correctly. Hence that section not having (NOLOAD) and the debugging sections having it points out the difference. Jan
On Thu, Apr 22, 2021 at 06:01:06PM +0200, Jan Beulich wrote: > On 22.04.2021 17:53, Roger Pau Monné wrote: > > On Thu, Apr 22, 2021 at 05:46:28PM +0200, Jan Beulich wrote: > >> On 22.04.2021 16:56, Roger Pau Monné wrote: > >>> On Thu, Apr 22, 2021 at 01:03:13PM +0200, Jan Beulich wrote: > >>>> On 22.04.2021 10:14, Roger Pau Monné wrote: > >>>>> On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote: > >>>>>> On 21.04.2021 17:30, Roger Pau Monné wrote: > >>>>>>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote: > >>>>>>>> On 21.04.2021 13:15, Roger Pau Monné wrote: > >>>>>>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote: > >>>>>>>>>> --- a/xen/arch/x86/xen.lds.S > >>>>>>>>>> +++ b/xen/arch/x86/xen.lds.S > >>>>>>>>>> @@ -312,10 +312,60 @@ SECTIONS > >>>>>>>>>> *(.reloc) > >>>>>>>>>> __base_relocs_end = .; > >>>>>>>>>> } > >>>>>>>>>> - /* Trick the linker into setting the image size to exactly 16Mb. */ > >>>>>>>>>> - . = ALIGN(__section_alignment__); > >>>>>>>>>> - DECL_SECTION(.pad) { > >>>>>>>>>> - . = ALIGN(MB(16)); > >>>>>>>>>> + .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)) : .; > >>>>>>>>> > >>>>>>>>> I think this is inside an ifdef EFI region, since this is DWARF info > >>>>>>>>> couldn't it also be present when building with EFI disabled? > >>>>>>>> > >>>>>>>> Of course (and it's not just "could" but "will"), yet the linker will > >>>>>>>> do fine (and perhaps even better) without when building ELF. Also > >>>>>>>> note that we'll be responsible for keeping the list of sections up-to- > >>>>>>>> date. The linker will recognize Dwarf sections by looking for a > >>>>>>>> .debug_ prefix. We can't use such here (or at least I'm not aware of > >>>>>>>> a suitable mechanism); .debug_* would mean munging together all the > >>>>>>>> different kinds of Dwarf sections. Hence by limiting the explicit > >>>>>>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road. > >>>>>>> > >>>>>>> Right, so we will have to keep this list of debug_ sections updated > >>>>>>> manually if/when more of those appear as part of DWARF updates? > >>>>>> > >>>>>> Yes. > >>>>>> > >>>>>>> Do we have a way to get some kind of warning or error when a new > >>>>>>> section not explicitly handled here appears? > >>>>>> > >>>>>> ld 2.37 will start warning about such sections, as they'd land at > >>>>>> VA 0 and hence below image base. > >>>>> > >>>>> That seems like a bug in ld? > >>>>> > >>>>> The '--image-base' option description mentions: "This is the lowest > >>>>> memory location that will be used when your program or dll is > >>>>> loaded.", so I would expect that if the option is used the default VA > >>>>> should be >= image-base, or else the description of the option is not > >>>>> consistent, as ld will still place sections at addresses below > >>>>> image-base. > >>>> > >>>> ld's "general" logic is pretty ELF-centric. Hence debugging sections > >>>> get placed at VA 0 by default, not matter what the (PE-specific) > >>>> --image-base says. Whether that's a bug though I'm not sure: There > >>>> are no really good alternatives that could be used by default. Doing > >>>> what we do here (and what e.g. Cygwin does) via linker script may not > >>>> be appropriate in the common case. In particular it is not generally > >>>> correct for debug info to be part of what gets loaded into memory. > >>> > >>> So with this change here you placate the warnings from newer ld about > >>> having a VA < image base, > >> > >> It's not just about silencing the warnings. The resulting image is > >> unusable when the sections don't get placed at a suitable VA. > > > > And this wasn't an issue before because the linker won't even attempt > > to place DWARF sections into a PE output. > > No, this wasn't an issue before since, for things to work, we > simply had to uniformly strip debug info when linking xen.efi. And > this is what Andrew said should change. I was initially opposed, > until I saw that Cygwin does just this as well. Just for my own education, do you have a reference about this way of packaging debug data by Cygwin? I've found: https://cygwin.com/pipermail/cygwin/2003-January/090110.html Which mentions not setting the ALLOC flag on the debug sections in order to prevent them from being loaded. I'm however not able to figure out which flag is that on the PE spec, or whether it can be set from the linker script. > >>> but the end result is that now the debug > >>> sections will also get loaded when booted from the EFI loader? > >>> (because the NOLOAD doesn't have any effect with PE) > >> > >> Yes. I currently see no other way to retain debug info in xen.efi. > >> But to be clear, the memory debug info occupies isn't lost - we > >> still free space from _end (or alike) onwards. .reloc, for example, > >> also lives there. And I was wondering whether we shouldn't keep > >> .comment this way as well. > > > > Yes, I already realized all this is past _end. > > > > I wonder however if the use of (NOLOAD) makes all this more confusing, > > such sections should only be present on the linker script used for the > > PE output, and then the (NOLOAD) doesn't make sense there? > > > > If so, I think the (NOLOAD) directive should be dropped, and a comment > > noting that the debug sections need to be manually added to the PE > > output in order to avoid them being placed at VA 0 would be helpful > > IMO, likely also mentioning that they would be loaded but discarded > > afterwards by Xen because they are all past _end. > > Earlier on (another sub-thread, maybe) I think I've already said that > I'd like to keep (NOLOAD) both for documentation purposes and just in > case the linker develops some smarts to actually translate it into > anything sensible when linking PE. This is quite different from > .reloc, after all - that section has to be loaded for our re- > relocation to work correctly. Hence that section not having (NOLOAD) > and the debugging sections having it points out the difference. Sure, that's all fine. I think a comment could be appropriate here, to note both that NOLOAD is likely useless and just used for documentation purposes, and to also mention the sections needs to be explicitly placed in the PE linker script so they are not set at VA 0. /* * 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, even when currently ignored by PE output, in * order to note those sections shouldn't be loaded into memory. * * Note such sections are past _end, so if loaded will be discarded by * Xen anyway. */ Feel free to reword or expand the comment. Not sure there's some reference we could add here about how debug sections are placed in PE files usually. Thanks, Roger.
On 23.04.2021 09:30, Roger Pau Monné wrote: > On Thu, Apr 22, 2021 at 06:01:06PM +0200, Jan Beulich wrote: >> On 22.04.2021 17:53, Roger Pau Monné wrote: >>> On Thu, Apr 22, 2021 at 05:46:28PM +0200, Jan Beulich wrote: >>>> On 22.04.2021 16:56, Roger Pau Monné wrote: >>>>> On Thu, Apr 22, 2021 at 01:03:13PM +0200, Jan Beulich wrote: >>>>>> On 22.04.2021 10:14, Roger Pau Monné wrote: >>>>>>> On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote: >>>>>>>> On 21.04.2021 17:30, Roger Pau Monné wrote: >>>>>>>>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote: >>>>>>>>>> On 21.04.2021 13:15, Roger Pau Monné wrote: >>>>>>>>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote: >>>>>>>>>>>> --- a/xen/arch/x86/xen.lds.S >>>>>>>>>>>> +++ b/xen/arch/x86/xen.lds.S >>>>>>>>>>>> @@ -312,10 +312,60 @@ SECTIONS >>>>>>>>>>>> *(.reloc) >>>>>>>>>>>> __base_relocs_end = .; >>>>>>>>>>>> } >>>>>>>>>>>> - /* Trick the linker into setting the image size to exactly 16Mb. */ >>>>>>>>>>>> - . = ALIGN(__section_alignment__); >>>>>>>>>>>> - DECL_SECTION(.pad) { >>>>>>>>>>>> - . = ALIGN(MB(16)); >>>>>>>>>>>> + .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)) : .; >>>>>>>>>>> >>>>>>>>>>> I think this is inside an ifdef EFI region, since this is DWARF info >>>>>>>>>>> couldn't it also be present when building with EFI disabled? >>>>>>>>>> >>>>>>>>>> Of course (and it's not just "could" but "will"), yet the linker will >>>>>>>>>> do fine (and perhaps even better) without when building ELF. Also >>>>>>>>>> note that we'll be responsible for keeping the list of sections up-to- >>>>>>>>>> date. The linker will recognize Dwarf sections by looking for a >>>>>>>>>> .debug_ prefix. We can't use such here (or at least I'm not aware of >>>>>>>>>> a suitable mechanism); .debug_* would mean munging together all the >>>>>>>>>> different kinds of Dwarf sections. Hence by limiting the explicit >>>>>>>>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road. >>>>>>>>> >>>>>>>>> Right, so we will have to keep this list of debug_ sections updated >>>>>>>>> manually if/when more of those appear as part of DWARF updates? >>>>>>>> >>>>>>>> Yes. >>>>>>>> >>>>>>>>> Do we have a way to get some kind of warning or error when a new >>>>>>>>> section not explicitly handled here appears? >>>>>>>> >>>>>>>> ld 2.37 will start warning about such sections, as they'd land at >>>>>>>> VA 0 and hence below image base. >>>>>>> >>>>>>> That seems like a bug in ld? >>>>>>> >>>>>>> The '--image-base' option description mentions: "This is the lowest >>>>>>> memory location that will be used when your program or dll is >>>>>>> loaded.", so I would expect that if the option is used the default VA >>>>>>> should be >= image-base, or else the description of the option is not >>>>>>> consistent, as ld will still place sections at addresses below >>>>>>> image-base. >>>>>> >>>>>> ld's "general" logic is pretty ELF-centric. Hence debugging sections >>>>>> get placed at VA 0 by default, not matter what the (PE-specific) >>>>>> --image-base says. Whether that's a bug though I'm not sure: There >>>>>> are no really good alternatives that could be used by default. Doing >>>>>> what we do here (and what e.g. Cygwin does) via linker script may not >>>>>> be appropriate in the common case. In particular it is not generally >>>>>> correct for debug info to be part of what gets loaded into memory. >>>>> >>>>> So with this change here you placate the warnings from newer ld about >>>>> having a VA < image base, >>>> >>>> It's not just about silencing the warnings. The resulting image is >>>> unusable when the sections don't get placed at a suitable VA. >>> >>> And this wasn't an issue before because the linker won't even attempt >>> to place DWARF sections into a PE output. >> >> No, this wasn't an issue before since, for things to work, we >> simply had to uniformly strip debug info when linking xen.efi. And >> this is what Andrew said should change. I was initially opposed, >> until I saw that Cygwin does just this as well. > > Just for my own education, do you have a reference about this way of > packaging debug data by Cygwin? I've simply built a test program and looked at the binary. The best reference I could think of is their default linker script in binutils (ld/scripttempl/pep.sc). > I've found: > > https://cygwin.com/pipermail/cygwin/2003-January/090110.html > > Which mentions not setting the ALLOC flag on the debug sections in > order to prevent them from being loaded. I'm however not able to > figure out which flag is that on the PE spec, or whether it can be set > from the linker script. There's no truly corresponding flag in COFF. >>>>> but the end result is that now the debug >>>>> sections will also get loaded when booted from the EFI loader? >>>>> (because the NOLOAD doesn't have any effect with PE) >>>> >>>> Yes. I currently see no other way to retain debug info in xen.efi. >>>> But to be clear, the memory debug info occupies isn't lost - we >>>> still free space from _end (or alike) onwards. .reloc, for example, >>>> also lives there. And I was wondering whether we shouldn't keep >>>> .comment this way as well. >>> >>> Yes, I already realized all this is past _end. >>> >>> I wonder however if the use of (NOLOAD) makes all this more confusing, >>> such sections should only be present on the linker script used for the >>> PE output, and then the (NOLOAD) doesn't make sense there? >>> >>> If so, I think the (NOLOAD) directive should be dropped, and a comment >>> noting that the debug sections need to be manually added to the PE >>> output in order to avoid them being placed at VA 0 would be helpful >>> IMO, likely also mentioning that they would be loaded but discarded >>> afterwards by Xen because they are all past _end. >> >> Earlier on (another sub-thread, maybe) I think I've already said that >> I'd like to keep (NOLOAD) both for documentation purposes and just in >> case the linker develops some smarts to actually translate it into >> anything sensible when linking PE. This is quite different from >> .reloc, after all - that section has to be loaded for our re- >> relocation to work correctly. Hence that section not having (NOLOAD) >> and the debugging sections having it points out the difference. > > Sure, that's all fine. I think a comment could be appropriate here, to > note both that NOLOAD is likely useless and just used for > documentation purposes, and to also mention the sections needs to be > explicitly placed in the PE linker script so they are not set at VA 0. > > /* > * 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, even when currently ignored by PE output, in > * order to note those sections shouldn't be loaded into memory. > * > * Note such sections are past _end, so if loaded will be discarded by > * Xen anyway. > */ > > Feel free to reword or expand the comment. Yes, I've edited it some while inserting. Will see to get to submitting v2 then. > Not sure there's some > reference we could add here about how debug sections are placed in PE > files usually. As said before - I don't think there's any "usually" here, which is why different environments have invented different ways. The debug info native to COFF is more like ELF's symbol table (with a little bit of extra information) plus Dwarf's .debug_line, but not really fully covering what you'd expect from debug info. Jan
On Fri, Apr 23, 2021 at 10:51:40AM +0200, Jan Beulich wrote: > On 23.04.2021 09:30, Roger Pau Monné wrote: > > /* > > * 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, even when currently ignored by PE output, in > > * order to note those sections shouldn't be loaded into memory. > > * > > * Note such sections are past _end, so if loaded will be discarded by > > * Xen anyway. > > */ > > > > Feel free to reword or expand the comment. > > Yes, I've edited it some while inserting. Will see to get to > submitting v2 then. > > > Not sure there's some > > reference we could add here about how debug sections are placed in PE > > files usually. > > As said before - I don't think there's any "usually" here, which is > why different environments have invented different ways. The debug > info native to COFF is more like ELF's symbol table (with a little > bit of extra information) plus Dwarf's .debug_line, but not really > fully covering what you'd expect from debug info. One last thing, do you know if the newly added debug_* sections get the IMAGE_SCN_MEM_DISCARDABLE section flag set? Not sure there's a way we can force it from the linker script TBH, but would be nice and seems to be a recommended flag for debug sections according to Microsoft [0]. Thanks, Roger. [0] https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#section-flags
On 23.04.2021 12:07, Roger Pau Monné wrote: > On Fri, Apr 23, 2021 at 10:51:40AM +0200, Jan Beulich wrote: >> On 23.04.2021 09:30, Roger Pau Monné wrote: >>> /* >>> * 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, even when currently ignored by PE output, in >>> * order to note those sections shouldn't be loaded into memory. >>> * >>> * Note such sections are past _end, so if loaded will be discarded by >>> * Xen anyway. >>> */ >>> >>> Feel free to reword or expand the comment. >> >> Yes, I've edited it some while inserting. Will see to get to >> submitting v2 then. >> >>> Not sure there's some >>> reference we could add here about how debug sections are placed in PE >>> files usually. >> >> As said before - I don't think there's any "usually" here, which is >> why different environments have invented different ways. The debug >> info native to COFF is more like ELF's symbol table (with a little >> bit of extra information) plus Dwarf's .debug_line, but not really >> fully covering what you'd expect from debug info. > > One last thing, do you know if the newly added debug_* sections get > the IMAGE_SCN_MEM_DISCARDABLE section flag set? At least with an up-to-date ld (i.e. one meeting the requirements so we wouldn't force debug info to be stripped) they do. > Not sure there's a way we can force it from the linker script TBH, but > would be nice and seems to be a recommended flag for debug sections > according to Microsoft [0]. The linker does this for debugging sections irrespective of what the linker script says: if ((sec_flags & SEC_DEBUGGING) != 0) styp_flags |= IMAGE_SCN_MEM_DISCARDABLE; Jan
On Fri, Apr 23, 2021 at 12:45:14PM +0200, Jan Beulich wrote: > On 23.04.2021 12:07, Roger Pau Monné wrote: > > On Fri, Apr 23, 2021 at 10:51:40AM +0200, Jan Beulich wrote: > >> On 23.04.2021 09:30, Roger Pau Monné wrote: > >>> /* > >>> * 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, even when currently ignored by PE output, in > >>> * order to note those sections shouldn't be loaded into memory. > >>> * > >>> * Note such sections are past _end, so if loaded will be discarded by > >>> * Xen anyway. > >>> */ > >>> > >>> Feel free to reword or expand the comment. > >> > >> Yes, I've edited it some while inserting. Will see to get to > >> submitting v2 then. > >> > >>> Not sure there's some > >>> reference we could add here about how debug sections are placed in PE > >>> files usually. > >> > >> As said before - I don't think there's any "usually" here, which is > >> why different environments have invented different ways. The debug > >> info native to COFF is more like ELF's symbol table (with a little > >> bit of extra information) plus Dwarf's .debug_line, but not really > >> fully covering what you'd expect from debug info. > > > > One last thing, do you know if the newly added debug_* sections get > > the IMAGE_SCN_MEM_DISCARDABLE section flag set? > > At least with an up-to-date ld (i.e. one meeting the requirements so > we wouldn't force debug info to be stripped) they do. > > > Not sure there's a way we can force it from the linker script TBH, but > > would be nice and seems to be a recommended flag for debug sections > > according to Microsoft [0]. > > The linker does this for debugging sections irrespective of what the > linker script says: > > if ((sec_flags & SEC_DEBUGGING) != 0) > styp_flags |= IMAGE_SCN_MEM_DISCARDABLE; Great, that's good to know. Note sure it's worth adding to the commit message, maybe I've just missed part of the documentation where LD notes that IMAGE_SCN_MEM_DISCARDABLE will be unconditionally added to debug sections. Thanks, Roger.
--- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -126,8 +126,14 @@ export XEN_BUILD_EFI := $(shell $(CC) $( CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI # Check if the linker supports PE. -EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(XEN_LDFLAGS)) --subsystem=10 --strip-debug -XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(shell $(LD) $(EFI_LDFLAGS) -o efi/check.efi efi/check.o 2>/dev/null && echo y)) +EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(XEN_LDFLAGS)) --subsystem=10 +XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(call ld-option,$(EFI_LDFLAGS) --image-base=0x100000000 -o efi/check.efi efi/check.o)) +# If the above failed, it may be merely because of the linker not dealing well +# with debug info. Try again with stripping it. +ifeq ($(CONFIG_DEBUG_INFO)-$(XEN_BUILD_PE),y-n) +EFI_LDFLAGS += --strip-debug +XEN_BUILD_PE := $(call ld-option,$(EFI_LDFLAGS) --image-base=0x100000000 -o efi/check.efi efi/check.o) +endif ifeq ($(XEN_BUILD_PE),y) @@ -232,6 +238,9 @@ note_file_option ?= $(note_file) ifeq ($(XEN_BUILD_PE),y) $(TARGET).efi: prelink.o $(note_file) efi.lds efi/relocs-dummy.o efi/mkreloc +ifeq ($(CONFIG_DEBUG_INFO),y) + $(if $(filter --strip-debug,$(EFI_LDFLAGS)),echo,:) "Will strip debug info from $(@F)" +endif $(foreach base, $(VIRT_BASE) $(ALT_BASE), \ $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< $(relocs-dummy) \ $(BASEDIR)/common/symbols-dummy.o $(note_file_option) -o $(@D)/.$(@F).$(base).0 &&) : --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -312,10 +312,60 @@ SECTIONS *(.reloc) __base_relocs_end = .; } - /* Trick the linker into setting the image size to exactly 16Mb. */ - . = ALIGN(__section_alignment__); - DECL_SECTION(.pad) { - . = ALIGN(MB(16)); + .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) /*
... provided the linker supports it (which it does as of commit 2dfa8341e079 ["ELF DWARF in PE output"]). Without mentioning debugging sections, the linker would put them at VA 0, thus making them unreachable by 32-bit (relative or absolute) relocations. If relocations were resolvable (or absent) the resulting binary would have invalid section RVAs (0 - __image_base__, truncated to 32 bits). Mentioning debugging sections without specifying an address will result in the linker putting them all on the same RVA. A loader is, afaict, free to reject loading such an image, as sections shouldn't overlap. (The above describes GNU ld 2.36 behavior, which - if deemed buggy - could change.) Make sure our up-to-16Mb padding doesn't unnecessarily further extend the image. Take the opportunity and also switch to using $(call ld-option,...). Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- This way we could also avoid discarding .comment for xen.efi. I'd like to point out that the linking of the debug info takes far longer than the linking of the "normal" parts of the image. The change therefore has the downside of slowing down debug builds.