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

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

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.