All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] make use of .startof.() and .sizeof.() assembler expressions
@ 2016-08-24  7:44 Jan Beulich
  2016-08-30 12:50 ` Julien Grall
  2016-08-31 15:31 ` Andrew Cooper
  0 siblings, 2 replies; 8+ messages in thread
From: Jan Beulich @ 2016-08-24  7:44 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan, Julien Grall

[-- Attachment #1: Type: text/plain, Size: 13378 bytes --]

Section start symbols frequently obscure the actual symbol name living
at the start of the section. Eliminate them where they can be replaced
by linker resolved .startof.* symbols. (Section end symbols may have
the same undesirable effect, but they're less easy to eliminate, as
they'd need to be represented by the sum of .startof.<section> and
.sizeof.<section>).

Along those lines use .sizeof.* where section sizes get calculated
from the difference of section and and section start symbols.

Note that this would be nice for the build-id section too, but:
- The generated (by ld) section names differ between ELF and COFF/PE
  (ELF: .note.gnu.build-id; COFF/PE[2.26+]: .buildid), yet we compile
  version.c just once (and hence can use only either of the necessary
  .startof./.sizeof. forms).
- The ELF section name needs to be quoted in the .startof./.sizeof.
  expressions, yet that quoting is supported only by gas 2.26+.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Also replace __base_relocs_{start,end}. Comment out linker script
    assertion on .bss start alignment.

--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -184,10 +184,10 @@ hyp:    PRINT("- Xen starting in Hyp mod
         bne   skip_bss
 
         PRINT("- Zero BSS -\r\n")
-        ldr   r0, =__bss_start       /* Load start & end of bss */
-        ldr   r1, =__bss_end
+        ldr   r0, =.startof.(.bss)   /* Load start & size of bss */
+        ldr   r1, =.sizeof.(.bss)
         add   r0, r0, r10            /* Apply physical offset */
-        add   r1, r1, r10
+        add   r1, r1, r0             /* Calculate end of bss */
 
         mov   r2, #0
 1:      str   r2, [r0], #4
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -318,10 +318,10 @@ el2:    PRINT("- Xen starting at EL2 -\r
         cbnz  x22, skip_bss
 
         PRINT("- Zero BSS -\r\n")
-        ldr   x0, =__bss_start       /* Load start & end of bss */
-        ldr   x1, =__bss_end
+        ldr   x0, =.startof.(.bss)   /* Load start & size of bss */
+        ldr   x1, =.sizeof.(.bss)
         add   x0, x0, x20            /* Apply physical offset */
-        add   x1, x1, x20
+        add   x1, x1, x0             /* Calculate end of .bss */
 
 1:      str   xzr, [x0], #8
         cmp   x0, x1
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -21,8 +21,8 @@
 #include <xen/errno.h>
 #include <xen/lib.h>
 
-extern const struct device_desc _sdevice[], _edevice[];
-extern const struct acpi_device_desc _asdevice[], _aedevice[];
+extern const struct device_desc _sdevice[] asm(".startof.(.dev.info)");
+extern const struct device_desc _edevice[];
 
 int __init device_init(struct dt_device_node *dev, enum device_class class,
                        const void *data)
@@ -51,6 +51,11 @@ int __init device_init(struct dt_device_
     return -EBADF;
 }
 
+#ifdef CONFIG_ACPI
+
+extern const struct acpi_device_desc _asdevice[] asm(".startof.(.adev.info)");
+extern const struct acpi_device_desc _aedevice[];
+
 int __init acpi_device_init(enum device_class class, const void *data, int class_type)
 {
     const struct acpi_device_desc *desc;
@@ -68,6 +73,8 @@ int __init acpi_device_init(enum device_
     return -EBADF;
 }
 
+#endif
+
 enum device_class device_get_class(const struct dt_device_node *dev)
 {
     const struct device_desc *desc;
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2058,7 +2058,7 @@ static void __init find_gnttab_region(st
      * enough space for a large grant table
      */
     kinfo->gnttab_start = __pa(_stext);
-    kinfo->gnttab_size = (_etext - _stext) & PAGE_MASK;
+    kinfo->gnttab_size = _sizeof_text & PAGE_MASK;
 
     /* Make sure the grant table will fit in the region */
     if ( (kinfo->gnttab_size >> PAGE_SHIFT) < max_grant_frames )
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -22,7 +22,8 @@
 #include <xen/init.h>
 #include <asm/psci.h>
 
-extern const struct platform_desc _splatform[], _eplatform[];
+extern const struct platform_desc _splatform[] asm(".startof.(.arch.info)");
+extern const struct platform_desc _eplatform[];
 
 /* Pointer to the current platform description */
 static const struct platform_desc *platform;
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -31,7 +31,6 @@ SECTIONS
   . = XEN_VIRT_START;
   _start = .;
   .text : /* XXX should be AT ( XEN_PHYS_START ) */ {
-        _stext = .;            /* Text section */
        *(.text)
        *(.text.cold)
        *(.text.unlikely)
@@ -42,7 +41,6 @@ SECTIONS
 
   . = ALIGN(PAGE_SIZE);
   .rodata : {
-        _srodata = .;          /* Read-only data */
         /* Bug frames table */
        __start_bug_frames = .;
        *(.bug_frames.0)
@@ -104,21 +102,18 @@ SECTIONS
 
   . = ALIGN(8);
   .arch.info : {
-      _splatform = .;
       *(.arch.info)
       _eplatform = .;
   } :text
 
   . = ALIGN(8);
   .dev.info : {
-      _sdevice = .;
       *(.dev.info)
       _edevice = .;
   } :text
 
   . = ALIGN(8);
   .adev.info : {
-      _asdevice = .;
       *(.adev.info)
       _aedevice = .;
   } :text
@@ -126,7 +121,6 @@ SECTIONS
   . = ALIGN(PAGE_SIZE);             /* Init code and data */
   __init_begin = .;
   .init.text : {
-       _sinittext = .;
        *(.init.text)
        _einittext = .;
   } :text
@@ -174,7 +168,6 @@ SECTIONS
   __init_end = .;
 
   .bss : {                     /* BSS */
-       __bss_start = .;
        *(.bss.stack_aligned)
        . = ALIGN(PAGE_SIZE);
        *(.bss.page_aligned)
@@ -186,7 +179,6 @@ SECTIONS
        *(.bss.percpu.read_mostly)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
-       __bss_end = .;
   } :text
   _end = . ;
 
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -124,9 +124,8 @@ __start:
         mov     %eax,sym_phys(multiboot_ptr)
 
         /* Initialize BSS (no nasty surprises!) */
-        mov     $sym_phys(__bss_start),%edi
-        mov     $sym_phys(__bss_end),%ecx
-        sub     %edi,%ecx
+        mov     $sym_phys(.startof.(.bss)),%edi
+        mov     $.sizeof.(.bss),%ecx
         xor     %eax,%eax
         shr     $2,%ecx
         rep stosl
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -39,13 +39,17 @@ extern const struct pe_base_relocs {
     u32 rva;
     u32 size;
     u16 entries[];
-} __base_relocs_start[], __base_relocs_end[];
+} base_relocs_start[] asm(".startof.(.reloc)");
 
 static void __init efi_arch_relocate_image(unsigned long delta)
 {
-    const struct pe_base_relocs *base_relocs;
+    const struct pe_base_relocs *base_relocs = base_relocs_start;
+    const struct pe_base_relocs *base_relocs_end = base_relocs;
 
-    for ( base_relocs = __base_relocs_start; base_relocs < __base_relocs_end; )
+    /* Can't do without asm() due to -fPIC. */
+    asm ( "add $.sizeof.(.reloc),%0" : "+g" (base_relocs_end) );
+
+    while ( base_relocs < base_relocs_end )
     {
         unsigned int i = 0, n;
 
--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -314,9 +314,7 @@ int main(int argc, char *argv[])
     }
 
     puts("\t.section .reloc, \"a\", @progbits\n"
-         "\t.balign 4\n"
-         "\t.globl __base_relocs_start, __base_relocs_end\n"
-         "__base_relocs_start:");
+         "\t.balign 4");
 
     for ( i = 0; i < nsec; ++i )
     {
@@ -368,8 +366,6 @@ int main(int argc, char *argv[])
 
     diff_sections(NULL, NULL, NULL, 0, 0, 0, 0);
 
-    puts("__base_relocs_end:");
-
     close(in1);
     close(in2);
 
--- a/xen/arch/x86/efi/relocs-dummy.S
+++ b/xen/arch/x86/efi/relocs-dummy.S
@@ -2,10 +2,8 @@
 
 	.section .reloc, "a", @progbits
 	.balign 4
-GLOBAL(__base_relocs_start)
 	.long 0
 	.long 8
-GLOBAL(__base_relocs_end)
 
 	.globl VIRT_START, ALT_START
 	.equ VIRT_START, XEN_VIRT_START
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -181,7 +181,7 @@ void __init discard_initial_images(void)
     initial_images = NULL;
 }
 
-extern char __init_begin[], __init_end[], __bss_start[], __bss_end[];
+extern char __init_begin[], __init_end[], __bss_end[];
 
 static void __init init_idle_domain(void)
 {
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -48,7 +48,7 @@ static uint64_t __initdata sinit_base, _
 #define TXTCR_HEAP_BASE             0x0300
 #define TXTCR_HEAP_SIZE             0x0308
 
-extern char __init_begin[], __bss_start[], __bss_end[];
+extern char __init_begin[];
 
 #define SHA1_SIZE      20
 typedef uint8_t   sha1_hash_t[SHA1_SIZE];
@@ -376,7 +376,12 @@ void tboot_shutdown(uint32_t shutdown_ty
                                               __pa(&_stext);
         /* bss */
         g_tboot_shared->mac_regions[2].start = (uint64_t)__pa(&__bss_start);
-        g_tboot_shared->mac_regions[2].size = __pa(&__bss_end) - __pa(&__bss_start);
+#if 0 /* This doesn't work due to -fPIC. */
+        g_tboot_shared->mac_regions[2].size = _sizeof_bss;
+#else
+        asm ( "movl $.sizeof.(.bss),%0"
+              : "=m" (g_tboot_shared->mac_regions[2].size) );
+#endif
 
         /*
          * MAC domains and other Xen memory
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -58,7 +58,6 @@ SECTIONS
   . = __XEN_VIRT_START + MB(1);
   _start = .;
   .text : {
-        _stext = .;            /* Text and read-only data */
        *(.text)
        *(.text.cold)
        *(.text.unlikely)
@@ -77,7 +76,6 @@ SECTIONS
 
   __2M_rodata_start = .;       /* Start of 2M superpages, mapped RO. */
   .rodata : {
-       _srodata = .;
        /* Bug frames table */
        . = ALIGN(4);
        __start_bug_frames = .;
@@ -149,7 +147,6 @@ SECTIONS
   . = ALIGN(PAGE_SIZE);             /* Init code and data */
   __init_begin = .;
   .init.text : {
-       _sinittext = .;
        *(.init.text)
        /*
         * Here are the replacement instructions. The linker sticks them
@@ -230,7 +227,6 @@ SECTIONS
   } :text
 
   .bss : {                     /* BSS */
-       __bss_start = .;
        *(.bss.stack_aligned)
        *(.bss.page_aligned*)
        *(.bss)
@@ -253,7 +249,6 @@ SECTIONS
   __2M_rwdata_end = .;
 
 #ifdef EFI
-  . = ALIGN(4);
   .reloc : {
     *(.reloc)
   } :text
@@ -323,5 +318,7 @@ ASSERT(IS_ALIGNED(__init_end,   PAGE_SIZ
 
 ASSERT(IS_ALIGNED(trampoline_start, 4), "trampoline_start misaligned")
 ASSERT(IS_ALIGNED(trampoline_end,   4), "trampoline_end misaligned")
-ASSERT(IS_ALIGNED(__bss_start,      4), "__bss_start misaligned")
+/* This would be nice, but fails with older ld (observed on 2.20).
+ASSERT(IS_ALIGNED(.startof..bss,    4), ".bss misaligned")
+ */
 ASSERT(IS_ALIGNED(__bss_end,        4), "__bss_end misaligned")
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -71,24 +71,34 @@ extern char _start[], _end[], start[];
     (__p >= _start) && (__p < _end);            \
 })
 
-extern char _stext[], _etext[];
+extern const char _stext[] asm(".startof.(.text)");
+extern const char _etext[];
 #define is_kernel_text(p) ({                    \
     char *__p = (char *)(unsigned long)(p);     \
     (__p >= _stext) && (__p < _etext);          \
 })
 
-extern const char _srodata[], _erodata[];
+extern const char _sizeof_text[] asm(".sizeof.(.text)");
+#define _sizeof_text ((unsigned long)_sizeof_text)
+
+extern const char _srodata[] asm(".startof.(.rodata)");
+extern const char _erodata[];
 #define is_kernel_rodata(p) ({                  \
     const char *__p = (const char *)(unsigned long)(p);     \
     (__p >= _srodata) && (__p < _erodata);      \
 })
 
-extern char _sinittext[], _einittext[];
+extern const char _sinittext[] asm(".startof.(.init.text)");
+extern const char _einittext[];
 #define is_kernel_inittext(p) ({                \
     char *__p = (char *)(unsigned long)(p);     \
     (__p >= _sinittext) && (__p < _einittext);  \
 })
 
+extern char __bss_start[] asm(".startof.(.bss)");
+extern const char _sizeof_bss[] asm(".sizeof.(.bss)");
+#define _sizeof_bss ((unsigned long)_sizeof_bss)
+
 extern enum system_state {
     SYS_STATE_early_boot,
     SYS_STATE_boot,
--- a/xen/tools/symbols.c
+++ b/xen/tools/symbols.c
@@ -143,11 +143,12 @@ static int read_symbol(FILE *in, struct
 		sym++;
 
 	/* Ignore most absolute/undefined (?) symbols. */
-	if (strcmp(sym, "_stext") == 0)
+	if (strcmp(sym, "_stext") == 0 || strcmp(sym, ".startof..text") == 0)
 		_stext = s->addr;
 	else if (strcmp(sym, "_etext") == 0)
 		_etext = s->addr;
-	else if (strcmp(sym, "_sinittext") == 0)
+	else if (strcmp(sym, "_sinittext") == 0 ||
+		 strcmp(sym, ".startof..init.text") == 0)
 		_sinittext = s->addr;
 	else if (strcmp(sym, "_einittext") == 0)
 		_einittext = s->addr;
@@ -168,6 +169,9 @@ static int read_symbol(FILE *in, struct
 	/* exclude also MIPS ELF local symbols ($L123 instead of .L123) */
 	else if (str[0] == '$')
 		goto skip_tail;
+	/* Also strip .startof. symbols. */
+	if (strncmp(sym, ".startof.", 9) == 0)
+		goto skip_tail;
 
 	/* include the type field in the symbol name, so that it gets
 	 * compressed together */



[-- Attachment #2: startof.patch --]
[-- Type: text/plain, Size: 13438 bytes --]

make use of .startof.() and .sizeof.() assembler expressions

Section start symbols frequently obscure the actual symbol name living
at the start of the section. Eliminate them where they can be replaced
by linker resolved .startof.* symbols. (Section end symbols may have
the same undesirable effect, but they're less easy to eliminate, as
they'd need to be represented by the sum of .startof.<section> and
.sizeof.<section>).

Along those lines use .sizeof.* where section sizes get calculated
from the difference of section and and section start symbols.

Note that this would be nice for the build-id section too, but:
- The generated (by ld) section names differ between ELF and COFF/PE
  (ELF: .note.gnu.build-id; COFF/PE[2.26+]: .buildid), yet we compile
  version.c just once (and hence can use only either of the necessary
  .startof./.sizeof. forms).
- The ELF section name needs to be quoted in the .startof./.sizeof.
  expressions, yet that quoting is supported only by gas 2.26+.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Also replace __base_relocs_{start,end}. Comment out linker script
    assertion on .bss start alignment.

--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -184,10 +184,10 @@ hyp:    PRINT("- Xen starting in Hyp mod
         bne   skip_bss
 
         PRINT("- Zero BSS -\r\n")
-        ldr   r0, =__bss_start       /* Load start & end of bss */
-        ldr   r1, =__bss_end
+        ldr   r0, =.startof.(.bss)   /* Load start & size of bss */
+        ldr   r1, =.sizeof.(.bss)
         add   r0, r0, r10            /* Apply physical offset */
-        add   r1, r1, r10
+        add   r1, r1, r0             /* Calculate end of bss */
 
         mov   r2, #0
 1:      str   r2, [r0], #4
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -318,10 +318,10 @@ el2:    PRINT("- Xen starting at EL2 -\r
         cbnz  x22, skip_bss
 
         PRINT("- Zero BSS -\r\n")
-        ldr   x0, =__bss_start       /* Load start & end of bss */
-        ldr   x1, =__bss_end
+        ldr   x0, =.startof.(.bss)   /* Load start & size of bss */
+        ldr   x1, =.sizeof.(.bss)
         add   x0, x0, x20            /* Apply physical offset */
-        add   x1, x1, x20
+        add   x1, x1, x0             /* Calculate end of .bss */
 
 1:      str   xzr, [x0], #8
         cmp   x0, x1
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -21,8 +21,8 @@
 #include <xen/errno.h>
 #include <xen/lib.h>
 
-extern const struct device_desc _sdevice[], _edevice[];
-extern const struct acpi_device_desc _asdevice[], _aedevice[];
+extern const struct device_desc _sdevice[] asm(".startof.(.dev.info)");
+extern const struct device_desc _edevice[];
 
 int __init device_init(struct dt_device_node *dev, enum device_class class,
                        const void *data)
@@ -51,6 +51,11 @@ int __init device_init(struct dt_device_
     return -EBADF;
 }
 
+#ifdef CONFIG_ACPI
+
+extern const struct acpi_device_desc _asdevice[] asm(".startof.(.adev.info)");
+extern const struct acpi_device_desc _aedevice[];
+
 int __init acpi_device_init(enum device_class class, const void *data, int class_type)
 {
     const struct acpi_device_desc *desc;
@@ -68,6 +73,8 @@ int __init acpi_device_init(enum device_
     return -EBADF;
 }
 
+#endif
+
 enum device_class device_get_class(const struct dt_device_node *dev)
 {
     const struct device_desc *desc;
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2058,7 +2058,7 @@ static void __init find_gnttab_region(st
      * enough space for a large grant table
      */
     kinfo->gnttab_start = __pa(_stext);
-    kinfo->gnttab_size = (_etext - _stext) & PAGE_MASK;
+    kinfo->gnttab_size = _sizeof_text & PAGE_MASK;
 
     /* Make sure the grant table will fit in the region */
     if ( (kinfo->gnttab_size >> PAGE_SHIFT) < max_grant_frames )
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -22,7 +22,8 @@
 #include <xen/init.h>
 #include <asm/psci.h>
 
-extern const struct platform_desc _splatform[], _eplatform[];
+extern const struct platform_desc _splatform[] asm(".startof.(.arch.info)");
+extern const struct platform_desc _eplatform[];
 
 /* Pointer to the current platform description */
 static const struct platform_desc *platform;
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -31,7 +31,6 @@ SECTIONS
   . = XEN_VIRT_START;
   _start = .;
   .text : /* XXX should be AT ( XEN_PHYS_START ) */ {
-        _stext = .;            /* Text section */
        *(.text)
        *(.text.cold)
        *(.text.unlikely)
@@ -42,7 +41,6 @@ SECTIONS
 
   . = ALIGN(PAGE_SIZE);
   .rodata : {
-        _srodata = .;          /* Read-only data */
         /* Bug frames table */
        __start_bug_frames = .;
        *(.bug_frames.0)
@@ -104,21 +102,18 @@ SECTIONS
 
   . = ALIGN(8);
   .arch.info : {
-      _splatform = .;
       *(.arch.info)
       _eplatform = .;
   } :text
 
   . = ALIGN(8);
   .dev.info : {
-      _sdevice = .;
       *(.dev.info)
       _edevice = .;
   } :text
 
   . = ALIGN(8);
   .adev.info : {
-      _asdevice = .;
       *(.adev.info)
       _aedevice = .;
   } :text
@@ -126,7 +121,6 @@ SECTIONS
   . = ALIGN(PAGE_SIZE);             /* Init code and data */
   __init_begin = .;
   .init.text : {
-       _sinittext = .;
        *(.init.text)
        _einittext = .;
   } :text
@@ -174,7 +168,6 @@ SECTIONS
   __init_end = .;
 
   .bss : {                     /* BSS */
-       __bss_start = .;
        *(.bss.stack_aligned)
        . = ALIGN(PAGE_SIZE);
        *(.bss.page_aligned)
@@ -186,7 +179,6 @@ SECTIONS
        *(.bss.percpu.read_mostly)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
-       __bss_end = .;
   } :text
   _end = . ;
 
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -124,9 +124,8 @@ __start:
         mov     %eax,sym_phys(multiboot_ptr)
 
         /* Initialize BSS (no nasty surprises!) */
-        mov     $sym_phys(__bss_start),%edi
-        mov     $sym_phys(__bss_end),%ecx
-        sub     %edi,%ecx
+        mov     $sym_phys(.startof.(.bss)),%edi
+        mov     $.sizeof.(.bss),%ecx
         xor     %eax,%eax
         shr     $2,%ecx
         rep stosl
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -39,13 +39,17 @@ extern const struct pe_base_relocs {
     u32 rva;
     u32 size;
     u16 entries[];
-} __base_relocs_start[], __base_relocs_end[];
+} base_relocs_start[] asm(".startof.(.reloc)");
 
 static void __init efi_arch_relocate_image(unsigned long delta)
 {
-    const struct pe_base_relocs *base_relocs;
+    const struct pe_base_relocs *base_relocs = base_relocs_start;
+    const struct pe_base_relocs *base_relocs_end = base_relocs;
 
-    for ( base_relocs = __base_relocs_start; base_relocs < __base_relocs_end; )
+    /* Can't do without asm() due to -fPIC. */
+    asm ( "add $.sizeof.(.reloc),%0" : "+g" (base_relocs_end) );
+
+    while ( base_relocs < base_relocs_end )
     {
         unsigned int i = 0, n;
 
--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -314,9 +314,7 @@ int main(int argc, char *argv[])
     }
 
     puts("\t.section .reloc, \"a\", @progbits\n"
-         "\t.balign 4\n"
-         "\t.globl __base_relocs_start, __base_relocs_end\n"
-         "__base_relocs_start:");
+         "\t.balign 4");
 
     for ( i = 0; i < nsec; ++i )
     {
@@ -368,8 +366,6 @@ int main(int argc, char *argv[])
 
     diff_sections(NULL, NULL, NULL, 0, 0, 0, 0);
 
-    puts("__base_relocs_end:");
-
     close(in1);
     close(in2);
 
--- a/xen/arch/x86/efi/relocs-dummy.S
+++ b/xen/arch/x86/efi/relocs-dummy.S
@@ -2,10 +2,8 @@
 
 	.section .reloc, "a", @progbits
 	.balign 4
-GLOBAL(__base_relocs_start)
 	.long 0
 	.long 8
-GLOBAL(__base_relocs_end)
 
 	.globl VIRT_START, ALT_START
 	.equ VIRT_START, XEN_VIRT_START
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -181,7 +181,7 @@ void __init discard_initial_images(void)
     initial_images = NULL;
 }
 
-extern char __init_begin[], __init_end[], __bss_start[], __bss_end[];
+extern char __init_begin[], __init_end[], __bss_end[];
 
 static void __init init_idle_domain(void)
 {
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -48,7 +48,7 @@ static uint64_t __initdata sinit_base, _
 #define TXTCR_HEAP_BASE             0x0300
 #define TXTCR_HEAP_SIZE             0x0308
 
-extern char __init_begin[], __bss_start[], __bss_end[];
+extern char __init_begin[];
 
 #define SHA1_SIZE      20
 typedef uint8_t   sha1_hash_t[SHA1_SIZE];
@@ -376,7 +376,12 @@ void tboot_shutdown(uint32_t shutdown_ty
                                               __pa(&_stext);
         /* bss */
         g_tboot_shared->mac_regions[2].start = (uint64_t)__pa(&__bss_start);
-        g_tboot_shared->mac_regions[2].size = __pa(&__bss_end) - __pa(&__bss_start);
+#if 0 /* This doesn't work due to -fPIC. */
+        g_tboot_shared->mac_regions[2].size = _sizeof_bss;
+#else
+        asm ( "movl $.sizeof.(.bss),%0"
+              : "=m" (g_tboot_shared->mac_regions[2].size) );
+#endif
 
         /*
          * MAC domains and other Xen memory
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -58,7 +58,6 @@ SECTIONS
   . = __XEN_VIRT_START + MB(1);
   _start = .;
   .text : {
-        _stext = .;            /* Text and read-only data */
        *(.text)
        *(.text.cold)
        *(.text.unlikely)
@@ -77,7 +76,6 @@ SECTIONS
 
   __2M_rodata_start = .;       /* Start of 2M superpages, mapped RO. */
   .rodata : {
-       _srodata = .;
        /* Bug frames table */
        . = ALIGN(4);
        __start_bug_frames = .;
@@ -149,7 +147,6 @@ SECTIONS
   . = ALIGN(PAGE_SIZE);             /* Init code and data */
   __init_begin = .;
   .init.text : {
-       _sinittext = .;
        *(.init.text)
        /*
         * Here are the replacement instructions. The linker sticks them
@@ -230,7 +227,6 @@ SECTIONS
   } :text
 
   .bss : {                     /* BSS */
-       __bss_start = .;
        *(.bss.stack_aligned)
        *(.bss.page_aligned*)
        *(.bss)
@@ -253,7 +249,6 @@ SECTIONS
   __2M_rwdata_end = .;
 
 #ifdef EFI
-  . = ALIGN(4);
   .reloc : {
     *(.reloc)
   } :text
@@ -323,5 +318,7 @@ ASSERT(IS_ALIGNED(__init_end,   PAGE_SIZ
 
 ASSERT(IS_ALIGNED(trampoline_start, 4), "trampoline_start misaligned")
 ASSERT(IS_ALIGNED(trampoline_end,   4), "trampoline_end misaligned")
-ASSERT(IS_ALIGNED(__bss_start,      4), "__bss_start misaligned")
+/* This would be nice, but fails with older ld (observed on 2.20).
+ASSERT(IS_ALIGNED(.startof..bss,    4), ".bss misaligned")
+ */
 ASSERT(IS_ALIGNED(__bss_end,        4), "__bss_end misaligned")
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -71,24 +71,34 @@ extern char _start[], _end[], start[];
     (__p >= _start) && (__p < _end);            \
 })
 
-extern char _stext[], _etext[];
+extern const char _stext[] asm(".startof.(.text)");
+extern const char _etext[];
 #define is_kernel_text(p) ({                    \
     char *__p = (char *)(unsigned long)(p);     \
     (__p >= _stext) && (__p < _etext);          \
 })
 
-extern const char _srodata[], _erodata[];
+extern const char _sizeof_text[] asm(".sizeof.(.text)");
+#define _sizeof_text ((unsigned long)_sizeof_text)
+
+extern const char _srodata[] asm(".startof.(.rodata)");
+extern const char _erodata[];
 #define is_kernel_rodata(p) ({                  \
     const char *__p = (const char *)(unsigned long)(p);     \
     (__p >= _srodata) && (__p < _erodata);      \
 })
 
-extern char _sinittext[], _einittext[];
+extern const char _sinittext[] asm(".startof.(.init.text)");
+extern const char _einittext[];
 #define is_kernel_inittext(p) ({                \
     char *__p = (char *)(unsigned long)(p);     \
     (__p >= _sinittext) && (__p < _einittext);  \
 })
 
+extern char __bss_start[] asm(".startof.(.bss)");
+extern const char _sizeof_bss[] asm(".sizeof.(.bss)");
+#define _sizeof_bss ((unsigned long)_sizeof_bss)
+
 extern enum system_state {
     SYS_STATE_early_boot,
     SYS_STATE_boot,
--- a/xen/tools/symbols.c
+++ b/xen/tools/symbols.c
@@ -143,11 +143,12 @@ static int read_symbol(FILE *in, struct
 		sym++;
 
 	/* Ignore most absolute/undefined (?) symbols. */
-	if (strcmp(sym, "_stext") == 0)
+	if (strcmp(sym, "_stext") == 0 || strcmp(sym, ".startof..text") == 0)
 		_stext = s->addr;
 	else if (strcmp(sym, "_etext") == 0)
 		_etext = s->addr;
-	else if (strcmp(sym, "_sinittext") == 0)
+	else if (strcmp(sym, "_sinittext") == 0 ||
+		 strcmp(sym, ".startof..init.text") == 0)
 		_sinittext = s->addr;
 	else if (strcmp(sym, "_einittext") == 0)
 		_einittext = s->addr;
@@ -168,6 +169,9 @@ static int read_symbol(FILE *in, struct
 	/* exclude also MIPS ELF local symbols ($L123 instead of .L123) */
 	else if (str[0] == '$')
 		goto skip_tail;
+	/* Also strip .startof. symbols. */
+	if (strncmp(sym, ".startof.", 9) == 0)
+		goto skip_tail;
 
 	/* include the type field in the symbol name, so that it gets
 	 * compressed together */

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] make use of .startof.() and .sizeof.() assembler expressions
  2016-08-24  7:44 [PATCH v2] make use of .startof.() and .sizeof.() assembler expressions Jan Beulich
@ 2016-08-30 12:50 ` Julien Grall
  2016-08-30 13:09   ` Jan Beulich
  2016-08-31 15:31 ` Andrew Cooper
  1 sibling, 1 reply; 8+ messages in thread
From: Julien Grall @ 2016-08-30 12:50 UTC (permalink / raw)
  To: Jan Beulich, xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan

Hi Jan,

On 24/08/16 08:44, Jan Beulich wrote:
> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -21,8 +21,8 @@
>  #include <xen/errno.h>
>  #include <xen/lib.h>
>
> -extern const struct device_desc _sdevice[], _edevice[];
> -extern const struct acpi_device_desc _asdevice[], _aedevice[];
> +extern const struct device_desc _sdevice[] asm(".startof.(.dev.info)");
> +extern const struct device_desc _edevice[];
>
>  int __init device_init(struct dt_device_node *dev, enum device_class class,
>                         const void *data)
> @@ -51,6 +51,11 @@ int __init device_init(struct dt_device_
>      return -EBADF;
>  }
>
> +#ifdef CONFIG_ACPI

Why did you add the #ifdef CONFIG_ACPI here? It is not explained in the 
commit message.

If you want to add it, it should be a separate patch and we should go 
further by removing the section in the linker script.

> +
> +extern const struct acpi_device_desc _asdevice[] asm(".startof.(.adev.info)");
> +extern const struct acpi_device_desc _aedevice[];
> +
>  int __init acpi_device_init(enum device_class class, const void *data, int class_type)
>  {
>      const struct acpi_device_desc *desc;
> @@ -68,6 +73,8 @@ int __init acpi_device_init(enum device_
>      return -EBADF;
>  }
>
> +#endif
> +
>  enum device_class device_get_class(const struct dt_device_node *dev)
>  {
>      const struct device_desc *desc;
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2058,7 +2058,7 @@ static void __init find_gnttab_region(st
>       * enough space for a large grant table
>       */
>      kinfo->gnttab_start = __pa(_stext);
> -    kinfo->gnttab_size = (_etext - _stext) & PAGE_MASK;
> +    kinfo->gnttab_size = _sizeof_text & PAGE_MASK;
>
>      /* Make sure the grant table will fit in the region */
>      if ( (kinfo->gnttab_size >> PAGE_SHIFT) < max_grant_frames )
> --- a/xen/arch/arm/platform.c
> +++ b/xen/arch/arm/platform.c
> @@ -22,7 +22,8 @@
>  #include <xen/init.h>
>  #include <asm/psci.h>
>
> -extern const struct platform_desc _splatform[], _eplatform[];
> +extern const struct platform_desc _splatform[] asm(".startof.(.arch.info)");
> +extern const struct platform_desc _eplatform[];
>
>  /* Pointer to the current platform description */
>  static const struct platform_desc *platform;
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -31,7 +31,6 @@ SECTIONS
>    . = XEN_VIRT_START;
>    _start = .;
>    .text : /* XXX should be AT ( XEN_PHYS_START ) */ {
> -        _stext = .;            /* Text section */

I don't see any removal of _stext in xen/arch/arm/alternative.c.

>         *(.text)
>         *(.text.cold)
>         *(.text.unlikely)
> @@ -42,7 +41,6 @@ SECTIONS
>
>    . = ALIGN(PAGE_SIZE);
>    .rodata : {
> -        _srodata = .;          /* Read-only data */
>          /* Bug frames table */
>         __start_bug_frames = .;
>         *(.bug_frames.0)
> @@ -104,21 +102,18 @@ SECTIONS
>
>    . = ALIGN(8);
>    .arch.info : {
> -      _splatform = .;
>        *(.arch.info)
>        _eplatform = .;
>    } :text
>
>    . = ALIGN(8);
>    .dev.info : {
> -      _sdevice = .;
>        *(.dev.info)
>        _edevice = .;
>    } :text
>
>    . = ALIGN(8);
>    .adev.info : {
> -      _asdevice = .;
>        *(.adev.info)
>        _aedevice = .;
>    } :text
> @@ -126,7 +121,6 @@ SECTIONS
>    . = ALIGN(PAGE_SIZE);             /* Init code and data */
>    __init_begin = .;
>    .init.text : {
> -       _sinittext = .;
>         *(.init.text)
>         _einittext = .;
>    } :text
> @@ -174,7 +168,6 @@ SECTIONS
>    __init_end = .;
>
>    .bss : {                     /* BSS */
> -       __bss_start = .;
>         *(.bss.stack_aligned)
>         . = ALIGN(PAGE_SIZE);
>         *(.bss.page_aligned)
> @@ -186,7 +179,6 @@ SECTIONS
>         *(.bss.percpu.read_mostly)
>         . = ALIGN(SMP_CACHE_BYTES);
>         __per_cpu_data_end = .;
> -       __bss_end = .;
>    } :text
>    _end = . ;

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] make use of .startof.() and .sizeof.() assembler expressions
  2016-08-30 12:50 ` Julien Grall
@ 2016-08-30 13:09   ` Jan Beulich
  2016-08-30 13:18     ` Julien Grall
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2016-08-30 13:09 UTC (permalink / raw)
  To: Julien Grall
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan, xen-devel

>>> On 30.08.16 at 14:50, <julien.grall@arm.com> wrote:
> On 24/08/16 08:44, Jan Beulich wrote:
>> --- a/xen/arch/arm/device.c
>> +++ b/xen/arch/arm/device.c
>> @@ -21,8 +21,8 @@
>>  #include <xen/errno.h>
>>  #include <xen/lib.h>
>>
>> -extern const struct device_desc _sdevice[], _edevice[];
>> -extern const struct acpi_device_desc _asdevice[], _aedevice[];
>> +extern const struct device_desc _sdevice[] asm(".startof.(.dev.info)");
>> +extern const struct device_desc _edevice[];
>>
>>  int __init device_init(struct dt_device_node *dev, enum device_class class,
>>                         const void *data)
>> @@ -51,6 +51,11 @@ int __init device_init(struct dt_device_
>>      return -EBADF;
>>  }
>>
>> +#ifdef CONFIG_ACPI
> 
> Why did you add the #ifdef CONFIG_ACPI here? It is not explained in the 
> commit message.

.startof.(section_which_not_exist) gives a linker error. And ld
doesn't add to the section table any sections mentioned in the
linker script, but without constituents.

> If you want to add it, it should be a separate patch and we should go 
> further by removing the section in the linker script.

While I don't think this needs to be broken out, it certainly can (but
then really there should have been a conditional like the one I
introduce from the beginning, to avoid having dead code in a non-
ACPI enabled binary).

>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -31,7 +31,6 @@ SECTIONS
>>    . = XEN_VIRT_START;
>>    _start = .;
>>    .text : /* XXX should be AT ( XEN_PHYS_START ) */ {
>> -        _stext = .;            /* Text section */
> 
> I don't see any removal of _stext in xen/arch/arm/alternative.c.

Why should that be removed? _stext doesn't cease to exist:

--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -71,24 +71,34 @@ extern char _start[], _end[], start[];
     (__p >= _start) && (__p < _end);            \
 })
 
-extern char _stext[], _etext[];
+extern const char _stext[] asm(".startof.(.text)");
+extern const char _etext[];
 #define is_kernel_text(p) ({                    \

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] make use of .startof.() and .sizeof.() assembler expressions
  2016-08-30 13:09   ` Jan Beulich
@ 2016-08-30 13:18     ` Julien Grall
  2016-08-30 15:15       ` Jan Beulich
  0 siblings, 1 reply; 8+ messages in thread
From: Julien Grall @ 2016-08-30 13:18 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan, xen-devel

Hi Jan,

On 30/08/16 14:09, Jan Beulich wrote:
>>>> On 30.08.16 at 14:50, <julien.grall@arm.com> wrote:
>> On 24/08/16 08:44, Jan Beulich wrote:
>>> --- a/xen/arch/arm/device.c
>>> +++ b/xen/arch/arm/device.c
>>> @@ -21,8 +21,8 @@
>>>  #include <xen/errno.h>
>>>  #include <xen/lib.h>
>>>
>>> -extern const struct device_desc _sdevice[], _edevice[];
>>> -extern const struct acpi_device_desc _asdevice[], _aedevice[];
>>> +extern const struct device_desc _sdevice[] asm(".startof.(.dev.info)");
>>> +extern const struct device_desc _edevice[];
>>>
>>>  int __init device_init(struct dt_device_node *dev, enum device_class class,
>>>                         const void *data)
>>> @@ -51,6 +51,11 @@ int __init device_init(struct dt_device_
>>>      return -EBADF;
>>>  }
>>>
>>> +#ifdef CONFIG_ACPI
>>
>> Why did you add the #ifdef CONFIG_ACPI here? It is not explained in the
>> commit message.
>
> .startof.(section_which_not_exist) gives a linker error. And ld
> doesn't add to the section table any sections mentioned in the
> linker script, but without constituents.

This looks quite fragile, with this patch you are now assuming that all 
the sections will not be empty. As we go towards a modular Xen, this may 
lead to more linker error.

For instance, it might be possible in the future to not compiled 
unnecessary platform specific code. As not all the boards requires 
platform specific code, the section may get empty.

>
>> If you want to add it, it should be a separate patch and we should go
>> further by removing the section in the linker script.
>
> While I don't think this needs to be broken out, it certainly can (but
> then really there should have been a conditional like the one I
> introduce from the beginning, to avoid having dead code in a non-
> ACPI enabled binary).

It was an oversight while reviewing the patch adding acpi_device_init 
and I thought you added CONFIG_ACPI just for comestic.

>
>>> --- a/xen/arch/arm/xen.lds.S
>>> +++ b/xen/arch/arm/xen.lds.S
>>> @@ -31,7 +31,6 @@ SECTIONS
>>>    . = XEN_VIRT_START;
>>>    _start = .;
>>>    .text : /* XXX should be AT ( XEN_PHYS_START ) */ {
>>> -        _stext = .;            /* Text section */
>>
>> I don't see any removal of _stext in xen/arch/arm/alternative.c.
>
> Why should that be removed? _stext doesn't cease to exist:

Oh, sorry I got confused.

> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -71,24 +71,34 @@ extern char _start[], _end[], start[];
>      (__p >= _start) && (__p < _end);            \
>  })
>
> -extern char _stext[], _etext[];
> +extern const char _stext[] asm(".startof.(.text)");
> +extern const char _etext[];
>  #define is_kernel_text(p) ({                    \
>
> Jan
>

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] make use of .startof.() and .sizeof.() assembler expressions
  2016-08-30 13:18     ` Julien Grall
@ 2016-08-30 15:15       ` Jan Beulich
  2016-08-30 16:01         ` Julien Grall
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2016-08-30 15:15 UTC (permalink / raw)
  To: Julien Grall
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan, xen-devel

>>> On 30.08.16 at 15:18, <julien.grall@arm.com> wrote:
> On 30/08/16 14:09, Jan Beulich wrote:
>>>>> On 30.08.16 at 14:50, <julien.grall@arm.com> wrote:
>>> If you want to add it, it should be a separate patch and we should go
>>> further by removing the section in the linker script.
>>
>> While I don't think this needs to be broken out, it certainly can (but
>> then really there should have been a conditional like the one I
>> introduce from the beginning, to avoid having dead code in a non-
>> ACPI enabled binary).
> 
> It was an oversight while reviewing the patch adding acpi_device_init 
> and I thought you added CONFIG_ACPI just for comestic.

So with that, do you then still require me to split this off?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] make use of .startof.() and .sizeof.() assembler expressions
  2016-08-30 15:15       ` Jan Beulich
@ 2016-08-30 16:01         ` Julien Grall
  0 siblings, 0 replies; 8+ messages in thread
From: Julien Grall @ 2016-08-30 16:01 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan, xen-devel



On 30/08/16 16:15, Jan Beulich wrote:
>>>> On 30.08.16 at 15:18, <julien.grall@arm.com> wrote:
>> On 30/08/16 14:09, Jan Beulich wrote:
>>>>>> On 30.08.16 at 14:50, <julien.grall@arm.com> wrote:
>>>> If you want to add it, it should be a separate patch and we should go
>>>> further by removing the section in the linker script.
>>>
>>> While I don't think this needs to be broken out, it certainly can (but
>>> then really there should have been a conditional like the one I
>>> introduce from the beginning, to avoid having dead code in a non-
>>> ACPI enabled binary).
>>
>> It was an oversight while reviewing the patch adding acpi_device_init
>> and I thought you added CONFIG_ACPI just for comestic.
>
> So with that, do you then still require me to split this off?

No, I am fine with you keep it here.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] make use of .startof.() and .sizeof.() assembler expressions
  2016-08-24  7:44 [PATCH v2] make use of .startof.() and .sizeof.() assembler expressions Jan Beulich
  2016-08-30 12:50 ` Julien Grall
@ 2016-08-31 15:31 ` Andrew Cooper
  2016-08-31 15:51   ` Jan Beulich
  1 sibling, 1 reply; 8+ messages in thread
From: Andrew Cooper @ 2016-08-31 15:31 UTC (permalink / raw)
  To: Jan Beulich, xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan,
	Ian Jackson, Julien Grall

On 24/08/16 08:44, Jan Beulich wrote:
> Section start symbols frequently obscure the actual symbol name living
> at the start of the section. Eliminate them where they can be replaced
> by linker resolved .startof.* symbols. (Section end symbols may have
> the same undesirable effect, but they're less easy to eliminate, as
> they'd need to be represented by the sum of .startof.<section> and
> .sizeof.<section>).
>
> Along those lines use .sizeof.* where section sizes get calculated
> from the difference of section and and section start symbols.
>
> Note that this would be nice for the build-id section too, but:
> - The generated (by ld) section names differ between ELF and COFF/PE
>   (ELF: .note.gnu.build-id; COFF/PE[2.26+]: .buildid), yet we compile
>   version.c just once (and hence can use only either of the necessary
>   .startof./.sizeof. forms).
> - The ELF section name needs to be quoted in the .startof./.sizeof.
>   expressions, yet that quoting is supported only by gas 2.26+.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Other than Juliens concerns, I am generally -1 so far for this patch.

I can appreciate the improvement you are trying to make, but this patch
is using an entirely undocumented feature of gnu LD, with no guarantees
that the feature won't disappear under our feet.  Were these to become
formally documented as stable, then that would be fine.

It also doesn't cater for the Clang build, which is a hard blocker to
accepting the patch.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] make use of .startof.() and .sizeof.() assembler expressions
  2016-08-31 15:31 ` Andrew Cooper
@ 2016-08-31 15:51   ` Jan Beulich
  0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2016-08-31 15:51 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan,
	Ian Jackson, Julien Grall, xen-devel

>>> On 31.08.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
> On 24/08/16 08:44, Jan Beulich wrote:
>> Section start symbols frequently obscure the actual symbol name living
>> at the start of the section. Eliminate them where they can be replaced
>> by linker resolved .startof.* symbols. (Section end symbols may have
>> the same undesirable effect, but they're less easy to eliminate, as
>> they'd need to be represented by the sum of .startof.<section> and
>> .sizeof.<section>).
>>
>> Along those lines use .sizeof.* where section sizes get calculated
>> from the difference of section and and section start symbols.
>>
>> Note that this would be nice for the build-id section too, but:
>> - The generated (by ld) section names differ between ELF and COFF/PE
>>   (ELF: .note.gnu.build-id; COFF/PE[2.26+]: .buildid), yet we compile
>>   version.c just once (and hence can use only either of the necessary
>>   .startof./.sizeof. forms).
>> - The ELF section name needs to be quoted in the .startof./.sizeof.
>>   expressions, yet that quoting is supported only by gas 2.26+.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Other than Juliens concerns, I am generally -1 so far for this patch.
> 
> I can appreciate the improvement you are trying to make, but this patch
> is using an entirely undocumented feature of gnu LD, with no guarantees
> that the feature won't disappear under our feet.  Were these to become
> formally documented as stable, then that would be fine.

It's been there for years, but I can certainly point out the
undocumentedness to the binutils maintainers. Not that I
expect much to come out of this other than them suggesting
me to write up something.

> It also doesn't cater for the Clang build, which is a hard blocker to
> accepting the patch.

I'll see if I can get a Clang environment set up somehow.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-08-31 15:51 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-24  7:44 [PATCH v2] make use of .startof.() and .sizeof.() assembler expressions Jan Beulich
2016-08-30 12:50 ` Julien Grall
2016-08-30 13:09   ` Jan Beulich
2016-08-30 13:18     ` Julien Grall
2016-08-30 15:15       ` Jan Beulich
2016-08-30 16:01         ` Julien Grall
2016-08-31 15:31 ` Andrew Cooper
2016-08-31 15:51   ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.