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

[Julien Grall <julien@xen.org>]

Hi,

On 29/03/2022 11:12, Michal Orzel wrote:
> On 29.03.2022 11:54, Julien Grall wrote:
>> Hi,
>>
>> On 22/03/2022 08:02, Michal Orzel wrote:
>>> Populate header file xen.lds.h with the first portion of macros storing
>>> constructs common to x86 and arm linker scripts. Replace the original
>>> constructs with these helpers.
>>>
>>> No functional improvements to x86 linker script.
>>>
>>> Making use of common macros improves arm linker script with:
>>> -explicit list of debug sections that otherwise are seen as "orphans"
>>
>> NIT: This is a bit confusing to see no space after -. Can you add one?
>>
> Ok.
> 
>> I would also recommend to start with (soft)tab to make clearer this is a list.
>>
>> Same goes for the  other use below.
>>
> Ok.
> 
>>
>>> by the linker. This will allow to fix issues after enabling linker
>>> option --orphan-handling one day
>>> -extended list of discarded section to include: .discard, desctructors
>>
>> Typo: s/desctructors/destructors/
>>
> Ok.
> 
>>> related sections, .fini_array which can reference .text.exit
>>> -sections not related to debugging that are placed by ld.lld.
>>> Even though Xen on arm compilation with LLVM support is not ready yet,
>>
>> Building natively Xen on Arm with Clang works. So do you mean you using LLD?
>>
> I mean using the LLVM replacements not only for CC + supporting cross-compilation.
> As for the linker, Xen sets llvm-ld which is very very old and in fact README states
> LLVM 3.5 or later but llvm-ld was removed before that.

I am confused. I looked at the llvm repo and lld is still there. So why 
are you saying is lld is very old and removed?

> Thus IMO support for LLVM on arm
> is not ready yet.

I agree that building Xen on Arm only with LLVM tools is not possible 
yet. But this statement seems to be a bit too broad here. I think what 
matters is we don't support linking with LLD on Arm.

>>> these sections do not cause problem to GNU ld.
>>>
>>> Please note that this patch does not aim to perform the full sync up
>>> between the linker scripts. It creates a base for further work.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>>
>> [...]
>>
>>> diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
>>> index dd292fa7dc..ad1d199021 100644
>>> --- a/xen/include/xen/xen.lds.h
>>> +++ b/xen/include/xen/xen.lds.h
>>> @@ -5,4 +5,104 @@
>>>     * Common macros to be used in architecture specific linker scripts.
>>>     */
>>>    +/* Macros to declare debug sections. */
>>> +#ifdef EFI
>>
>> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support EFI on arm64.
>>
>> As this #ifdef is now in generic code, can you explain how this is meant to be used?
>>
> As we do not define EFI on arm, all the stuff protected by #ifdef EFI is x86 specific.

I find the name "EFI" too generic to figure out that this code can only 
be used by x86.

But, from my understanding, this header is meant to contain generic 
code. It feels a bit odd that we are moving arch specific code.

To be honest, I don't quite understand why we need to make the 
diffferentiation on x86. So I guess the first question is how this is 
meant to be used on x86?

Once we answered that, we can decide whether this is correct to use EFI 
in generic code. IOW, is thish going to be useful for other arch?

> 
>>> +/*
>>> + * 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
>>> +
>>> +/* DWARF debug sections. */
>>> +#define DWARF_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)                    \
>>> +  DECL_DEBUG(.debug_rnglists, 4)                  \
>>> +  DECL_DEBUG(.debug_addr, 8)                      \
>>> +  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.
>>> + *
>>> + * LLVM ld also wants .symtab, .strtab, and .shstrtab placed. These look to
>>> + * be benign to GNU ld, so we can have them here unconditionally.
>>> + */
>>> +#define ELF_DETAILS_SECTIONS     \
>>> +  .comment 0 : { *(.comment) }   \
>>
>> This is a bit confusing. Here you seem to use the section .comment. But...
>>
>>> +  .symtab 0 : { *(.symtab) }     \
>>> +  .strtab 0 : { *(.strtab) }     \
>>> +  .shstrtab 0 : { *(.shstrtab) }
>>> +
>>> +#ifdef EFI
>>> +#define DISCARD_EFI_SECTIONS \
>>> +       *(.comment)   \
>>
>> ... here you will discard it if EFI is set. Which one take precedence if the caller use both ELF_DETAILS_SECTIONS and DISCARD_EFI_SECTION?
>>
> ELF_DETAILS_SECTIONS is protected by #ifndef EFI and DISCARD_EFI_SECTION by #ifdef EFI
> so the caller cannot use both ELF_DETAILS_SECTIONS and DISCARD_EFI_SECTION.

The caller will protect it. But it is not in the header. I don't think 
we should expect the user to check x86 to understand how this is meant 
to be used.

> 
>> Also, can you explain why we need to drop those sections when EFI is set?
>>
> This is related to x86. Please see the commit: 7844f90abd551f6d5cd9b670b5ed8a4683258a21

Why is this in the generic header then?

Cheers,

-- 
Julien Grall