Re: [PATCH v3 2/2] xen: Populate xen.lds.h and make use of its macros

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Jan,

On 05.04.2022 10:49, Jan Beulich wrote:
> On 31.03.2022 09:14, Michal Orzel wrote:
>> --- a/xen/include/xen/xen.lds.h
>> +++ b/xen/include/xen/xen.lds.h
>> @@ -5,4 +5,133 @@
>>   * Common macros to be used in architecture specific linker scripts.
>>   */
>>  
>> +/*
>> + * To avoid any confusion, please note that the EFI macro does not 
>> correspond
>> + * to EFI support and is used when linking a native EFI (i.e. PE/COFF) 
>> binary,
>> + * hence its usage in this header.
>> + */
>> +
>> +/* Macros to declare debug sections. */
>> +#ifdef EFI
>> +/*
>> + * Use the NOLOAD directive, despite currently ignored by (at least) GNU ld
>> + * for PE output, in order to record that we'd prefer these sections to not
>> + * be loaded into memory.
>> + */
>> +#define DECL_DEBUG(x, a) #x ALIGN(a) (NOLOAD) : { *(x) }
>> +#define DECL_DEBUG2(x, y, a) #x ALIGN(a) (NOLOAD) : { *(x) *(y) }
>> +#else
>> +#define DECL_DEBUG(x, a) #x 0 : { *(x) }
>> +#define DECL_DEBUG2(x, y, a) #x 0 : { *(x) *(y) }
>> +#endif
>> +
>> +/*
>> + * DWARF2+ debug sections.
>> + * Explicitly list debug sections, first of all to avoid these sections 
>> being
>> + * viewed as "orphan" by the linker.
>> + *
>> + * For the PE output this is further necessary so that they don't end up at
>> + * VA 0, which is below image base and thus invalid. Note that this macro is
>> + * to be used after _end, so if these sections get loaded they'll be 
>> discarded
>> + * at runtime anyway.
>> + */
>> +#define DWARF2_DEBUG_SECTIONS                     \
>> +  DECL_DEBUG(.debug_abbrev, 1)                    \
>> +  DECL_DEBUG2(.debug_info, .gnu.linkonce.wi.*, 1) \
>> +  DECL_DEBUG(.debug_types, 1)                     \
>> +  DECL_DEBUG(.debug_str, 1)                       \
>> +  DECL_DEBUG2(.debug_line, .debug_line.*, 1)      \
>> +  DECL_DEBUG(.debug_line_str, 1)                  \
>> +  DECL_DEBUG(.debug_names, 4)                     \
>> +  DECL_DEBUG(.debug_frame, 4)                     \
>> +  DECL_DEBUG(.debug_loc, 1)                       \
>> +  DECL_DEBUG(.debug_loclists, 4)                  \
>> +  DECL_DEBUG(.debug_macinfo, 1)                   \
>> +  DECL_DEBUG(.debug_macro, 1)                     \
>> +  DECL_DEBUG(.debug_ranges, 8)                    \
> 
> Here and ...
> 
>> +  DECL_DEBUG(.debug_rnglists, 4)                  \
>> +  DECL_DEBUG(.debug_addr, 8)                      \
> 
> ... here I think you also want to switch to POINTER_ALIGN.
> 
Ok, you're right.

>> +  DECL_DEBUG(.debug_aranges, 1)                   \
>> +  DECL_DEBUG(.debug_pubnames, 1)                  \
>> +  DECL_DEBUG(.debug_pubtypes, 1)
>> +
>> +/* Stabs debug sections. */
>> +#define STABS_DEBUG_SECTIONS                 \
>> +  .stab 0 : { *(.stab) }                     \
>> +  .stabstr 0 : { *(.stabstr) }               \
>> +  .stab.excl 0 : { *(.stab.excl) }           \
>> +  .stab.exclstr 0 : { *(.stab.exclstr) }     \
>> +  .stab.index 0 : { *(.stab.index) }         \
>> +  .stab.indexstr 0 : { *(.stab.indexstr) }
>> +
>> +/*
>> + * Required sections not related to debugging.
> 
> Nit: Perhaps better "Required ELF sections ..."? Personally I'd also
> drop the mentioning of debugging - that's not really relevant here.
> I'm also unsure about "Required" - .comment isn't really required.
> IOW ideally simply "ELF sections" or "Sections to be retained in ELF
> binaries" or some such.
> 
ELF sections is ok for me.

> Jan
> 

I will push updated series soon.

Cheers,
Michal