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

--- 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)
   /*

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