|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
Hi Jan,
Thanks for review.
On 29.03.2022 11:22, Jan Beulich wrote:
> On 22.03.2022 09:02, Michal Orzel wrote:
>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -69,10 +69,7 @@ SECTIONS
>> __proc_info_end = .;
>>
>> #ifdef CONFIG_HAS_VPCI
>> - . = ALIGN(POINTER_ALIGN);
>> - __start_vpci_array = .;
>> - *(SORT(.data.vpci.*))
>> - __end_vpci_array = .;
>> + VPCI_ARRAY
>> #endif
>
> Here and elsewhere I think the #ifdef should move as well, or to be
> precise VPCI_ARRAY (in the specific case here) should simply expand to
> nothing when CONFIG_HAS_VPCI is not defined.
>
I was thinking about it at the beginning so I'm ok with your solution.
>> @@ -222,22 +213,18 @@ SECTIONS
>> /* Section for the device tree blob (if any). */
>> .dtb : { *(.dtb) } :text
>>
>> + /*
>> + * Explicitly list debug sections, to avoid these sections being viewed as
>> + * "orphan" by the linker.
>> + */
>> + DWARF_DEBUG_SECTIONS
>
> Considering the comment, perhaps better to move ...
>
>> /* Sections to be discarded */
>> - /DISCARD/ : {
>> - *(.exit.text)
>> - *(.exit.data)
>> - *(.exitcall.exit)
>> - *(.eh_frame)
>> - }
>> + DISCARD_SECTIONS
>>
>> /* Stabs debugging 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) }
>> - .comment 0 : { *(.comment) }
>> + STABS_DEBUG_SECTIONS
>
> ... this up there?
That makes sense. Ok.
>
>> --- 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
>> +/*
>> + * 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 \
>
> May I suggest to call this DWARF2_DEBUG_SECTIONS, to make clear no
> Dwarf1 section is included here (and we also don't mean to support
> such debug info)?
>
As this list is not full I thought we are going to add DWARF1 sections one day.
DWARF2_DEBUG_SECTIONS would mean that we list sections only from DWARF2 which
is not true
as we have sections from DWARF3,5, etc.
Maybe we should leave it as it is but modify the comment to state:
/* 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) \
>> + 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) } \
I also need to add *(.comment.*) due to:
https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=69e46280937526db9cf78259cd8a0a9ec62dc847
>> + .symtab 0 : { *(.symtab) } \
>> + .strtab 0 : { *(.strtab) } \
>> + .shstrtab 0 : { *(.shstrtab) }
>> +
>> +#ifdef EFI
>> +#define DISCARD_EFI_SECTIONS \
>> + *(.comment) \
>> + *(.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); \
>
> Since you're generalizing this, you mean POINTER_ALIGN here, not 8.
>
Ok.
>> + __paramhypfs_start = .; \
>> + *(.data.paramhypfs) \
>> + __paramhypfs_end = .;
>> +
>> +#define LOCK_PROFILE_DATA \
>> + . = ALIGN(POINTER_ALIGN); \
>> + __lock_profile_start = .; \
>> + *(.lockprofile.data) \
>> + __lock_profile_end = .;
>
> While for *_SECTIONS I don't care as much, for these last three items
> I think it would be good if we (maybe just informally) established an
> ordering rule, such that we can ask further additions here to not occur
> randomly. Once we've grown a few more of these, this would also help
> quickly locating the perhaps just one construct of interest when
> looking up things. Personally I think the only sensible ordering
> criteria is the name of the construct being defined. This could be
> mentioned in a comment ahead of them, and that comment would then at
> the same time serve as separator between *_SECTIONS and any constructs
> also defining (enclosing) symbols.
>
Yes, name of the constructs is the good criteria.
I will do it in v3.
> Jan
>
Cheers,
Michal
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |