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

[Jan Beulich <jbeulich@xxxxxxxx>]

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