Re: [PATCH 1/3] xen: Introduce a header to store common linker scripts content

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.03.2022 09:21, Michal Orzel wrote:
> Both x86 and arm linker scripts share quite a lot of common content.
> It is difficult to keep syncing them up, thus introduce a new header
> in include/xen called xen_lds.h to store the internals mutual to all
> the linker scripts.
> 
> Populate xen_lds.h with the first portion of the common sections.
> Some of them are not yet added/completed in arm linker script but they
> definitely should be. 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@xxxxxxx>
> ---
>  xen/include/xen/xen_lds.h | 114 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
>  create mode 100644 xen/include/xen/xen_lds.h

While perhaps just minor, I'm not happy about new files added with underscores
in their names. Dashes are easier to type. Plus, looking at Linux, it may make
sense to name this xen.lds.h.

> --- /dev/null
> +++ b/xen/include/xen/xen_lds.h
> @@ -0,0 +1,114 @@
> +#ifndef __XEN_LDS_H__
> +#define __XEN_LDS_H__
> +
> +/*
> + * 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                      \
> +  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.
> + *
> + * 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 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) }   \
> +  .comment 0 : { *(.comment) }               \
> +  .symtab 0 : { *(.symtab) }                 \
> +  .strtab 0 : { *(.strtab) }                 \
> +  .shstrtab 0 : { *(.shstrtab) }

Please don't add non-Stabs sections to this macro.

> +#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 CTORS_SECTION                           \
> +       . = ALIGN(8);                            \
> +       __ctors_start = .;                       \
> +       *(SORT_BY_INIT_PRIORITY(.init_array.*))  \
> +       *(SORT_BY_INIT_PRIORITY(.ctors.*))       \
> +       *(.init_array)                           \
> +       *(.ctors)                                \
> +       __ctors_end = .;
> +
> +#define VPCI_SECTION             \
> +       . = ALIGN(POINTER_ALIGN); \
> +       __start_vpci_array = .;   \
> +       *(SORT(.data.vpci.*))     \
> +       __end_vpci_array = .;
> +
> +#define HYPFS_SECTION            \
> +       . = ALIGN(8);             \
> +       __paramhypfs_start = .;   \
> +       *(.data.paramhypfs)       \
> +       __paramhypfs_end = .;
> +
> +#define LOCK_PROFILE_SECTION     \
> +       . = ALIGN(POINTER_ALIGN); \
> +       __lock_profile_start = .; \
> +       *(.lockprofile.data)      \
> +       __lock_profile_end = .;
> +
> +#endif /* __XEN_LDS_H__ */

I'm not sure _SECTION is a good suffix to use in the four names above:
These aren't individual sections in the output, and for CTORS_SECTION
it's also not even a single input section.

As to CTORS_SECTION - I'm unconvinced of generalizing this without
first getting it right.

Overall I think it would be better to introduce this header along
with actually using the macros. That way one can check within the
patch that what you move / replace actually matches on both sides
without needing to cross patch boundaries. If you wanted to introduce
(and then include right away) an empty header first, that would be an
acceptable intermediate approach afaic.

Jan