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

[Jan Beulich <jbeulich@xxxxxxxx>]

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