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. Thus IMO support for
LLVM on arm
is not ready yet.
> NIT: Also, from the formatting it is not clear that the second sentence is
> part of the last item. How about removing the newline just after the first
> sentence?
>
Ok.
>> 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.or...@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.
>> +/*
>> + * 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.
> 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
>> + *(.comment.*) \
>> + *(.note.*)
>> +#else
>> +#define DISCARD_EFI_SECTIONS
>> +#endif
>> +
>> +/* Sections to be discarded. */
>> +#define DISCARD_SECTIONS \
>> + /DISCARD/ : { \
>> + *(.text.exit) \
>> + *(.exit.text) \
>> + *(.exit.data) \
>> + *(.exitcall.exit) \
>> + *(.discard) \
>> + *(.discard.*) \
>> + *(.eh_frame) \
>> + *(.dtors) \
>> + *(.dtors.*) \
>> + *(.fini_array) \
>> + *(.fini_array.*) \
>> + DISCARD_EFI_SECTIONS \
>> + }
>> +
>> +#define VPCI_ARRAY \
>> + . = ALIGN(POINTER_ALIGN); \
>> + __start_vpci_array = .; \
>> + *(SORT(.data.vpci.*)) \
>> + __end_vpci_array = .;
>> +
>> +#define HYPFS_PARAM \
>> + . = ALIGN(8); \
>> + __paramhypfs_start = .; \
>> + *(.data.paramhypfs) \
>> + __paramhypfs_end = .;
>> +
>> +#define LOCK_PROFILE_DATA \
>> + . = ALIGN(POINTER_ALIGN); \
>> + __lock_profile_start = .; \
>> + *(.lockprofile.data) \
>> + __lock_profile_end = .;
>> +
>> #endif /* __XEN_LDS_H__ */
>
> Cheers,
>
Cheers,
Michal
