Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi

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

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@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> 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.