[PATCH] Orphan sections and NOLOAD output section

[PATCH] Orphan sections and NOLOAD output section

[Eric Botcazou]

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

Hi,

we recently got a report about a counter-intuitive behavior of the GNU linker
for orphan sections matched by a NOLOAD output section, which turned out to be
an exact duplicate of:
  https://stackoverflow.com/questions/48764136/gcc-noload-directive-cause-wrong-memory-mapping

What happens is that the effect of the command:

  .foo (NOLOAD) : {}

is not equivalent to that of:

  .foo (NOLOAD) : { *(.foo) }

when there is more than 1 object file containing a .foo section: the former
will output two .foo sections, the first with PROGBITS and the second with
NOBITS, where the latter will output only one with NOBITS.

I think that the commands are essentially equivalent so the linker ought to yield
the same outcome, namely the single output section with NOBITS.

Tested on x86-64/Linux, OK for the mainline?


2022-04-21  Eric Botcazou  <ebotcazou@adacore.com>

ld/
	* ldelf.c (ldelf_place_orphan): Match only by name for a NOLOAD
	output section present in the script.
	* testsuite/ld-elf/orphan-13.d: New file.
	* testsuite/ld-elf/orphan-13.ld: Likewise.
	* testsuite/ld-elf/orphan-13a.s: Likewise.
	* testsuite/ld-elf/orphan-13b.s: Likewise.
	* testsuite/ld-elf/orphan-13c.s: Likewise.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 2787 bytes --]

diff --git a/ld/ldelf.c b/ld/ldelf.c
index 4094640b3f7..35718a9113b 100644
--- a/ld/ldelf.c
+++ b/ld/ldelf.c
@@ -2098,13 +2098,17 @@ ldelf_place_orphan (asection *s, const char *secname, int constraint)
 	   lang_insert_orphan to create a new output section.  */
 	constraint = SPECIAL;
 
-	/* Check to see if we already have an output section statement
-	   with this name, and its bfd section has compatible flags.
+	/* Check to see if we already have an output section statement with
+	   this name, and it was either present in the script with a special
+	   type (knowing that lang_insert_orphan only creates normal_section
+	   output sections) or its BFD section has compatible flags.
+
 	   If the section already exists but does not have any flags
 	   set, then it has been created by the linker, possibly as a
 	   result of a --section-start command line switch.  */
 	if (os->bfd_section != NULL
-	    && (os->bfd_section->flags == 0
+	    && (os->sectype == noload_section
+		|| os->bfd_section->flags == 0
 		|| (((s->flags ^ os->bfd_section->flags)
 		     & (SEC_LOAD | SEC_ALLOC)) == 0
 		    && (!elfinput
diff --git a/ld/testsuite/ld-elf/orphan-13.d b/ld/testsuite/ld-elf/orphan-13.d
new file mode 100644
index 00000000000..8f867dc8938
--- /dev/null
+++ b/ld/testsuite/ld-elf/orphan-13.d
@@ -0,0 +1,12 @@
+#source: orphan-13a.s
+#source: orphan-13b.s
+#source: orphan-13c.s
+#ld: -T orphan-13.ld
+#readelf: -S --wide
+#xfail: [uses_genelf]
+#xfail: xstormy16-*-*
+
+#...
+  \[[ 0-9]+\] \.foo +NOBITS +[0-9a-f]+ +[0-9a-f]+ +0+30 +0+ +A +0 +0 +[0-9]+
+  \[[ 0-9]+\] [._][^f].*
+#pass
diff --git a/ld/testsuite/ld-elf/orphan-13.ld b/ld/testsuite/ld-elf/orphan-13.ld
new file mode 100644
index 00000000000..face613f8bd
--- /dev/null
+++ b/ld/testsuite/ld-elf/orphan-13.ld
@@ -0,0 +1,7 @@
+SECTIONS
+{
+  . = SIZEOF_HEADERS;
+  .text : { *(.text) }
+  .data : { *(.data) }
+  .foo (NOLOAD) : {}
+}
diff --git a/ld/testsuite/ld-elf/orphan-13a.s b/ld/testsuite/ld-elf/orphan-13a.s
new file mode 100644
index 00000000000..a8a6f364455
--- /dev/null
+++ b/ld/testsuite/ld-elf/orphan-13a.s
@@ -0,0 +1,14 @@
+ .globl main
+ .globl _main
+ .globl start
+ .globl _start
+ .globl __start
+ .text
+main:
+_main:
+start:
+_start:
+__start:
+
+ .section .foo,"a",%progbits
+ .long 1,1,1,1
diff --git a/ld/testsuite/ld-elf/orphan-13b.s b/ld/testsuite/ld-elf/orphan-13b.s
new file mode 100644
index 00000000000..c475eb11d1b
--- /dev/null
+++ b/ld/testsuite/ld-elf/orphan-13b.s
@@ -0,0 +1,2 @@
+ .section .foo,"a",%progbits
+ .long 1,1,1,1
diff --git a/ld/testsuite/ld-elf/orphan-13c.s b/ld/testsuite/ld-elf/orphan-13c.s
new file mode 100644
index 00000000000..c475eb11d1b
--- /dev/null
+++ b/ld/testsuite/ld-elf/orphan-13c.s
@@ -0,0 +1,2 @@
+ .section .foo,"a",%progbits
+ .long 1,1,1,1

Re: [PATCH] Orphan sections and NOLOAD output section

[Jan Beulich]

On 21.04.2022 17:31, Eric Botcazou via Binutils wrote:
> we recently got a report about a counter-intuitive behavior of the GNU linker
> for orphan sections matched by a NOLOAD output section, which turned out to be
> an exact duplicate of:
>   https://stackoverflow.com/questions/48764136/gcc-noload-directive-cause-wrong-memory-mapping
> 
> What happens is that the effect of the command:
> 
>   .foo (NOLOAD) : {}
> 
> is not equivalent to that of:
> 
>   .foo (NOLOAD) : { *(.foo) }
> 
> when there is more than 1 object file containing a .foo section: the former
> will output two .foo sections, the first with PROGBITS and the second with
> NOBITS, where the latter will output only one with NOBITS.
> 
> I think that the commands are essentially equivalent so the linker ought to yield
> the same outcome, namely the single output section with NOBITS.

Hmm, not having found any statement as to potentially special
meaning of nothing inside the braces, I'm not convinced the two
are "essentially equivalent". What I'm puzzled by though is your
reference to there needing to be more than one object file
involved - I wouldn't expect the behavior to depend on the number
of object files contributing to a section. Instead I'd expect a
single object file to similarly result in two .foo sections in
the output.

Jan

Re: [PATCH] Orphan sections and NOLOAD output section

[Eric Botcazou]

> Hmm, not having found any statement as to potentially special
> meaning of nothing inside the braces, I'm not convinced the two
> are "essentially equivalent".

See the manual, section "3.10.4 Orphan Sections".

-- 
Eric Botcazou

Re: [PATCH] Orphan sections and NOLOAD output section

[Alan Modra]

On Thu, Apr 21, 2022 at 05:31:58PM +0200, Eric Botcazou via Binutils wrote:
> Hi,
> 
> we recently got a report about a counter-intuitive behavior of the GNU linker
> for orphan sections matched by a NOLOAD output section, which turned out to be
> an exact duplicate of:
>   https://stackoverflow.com/questions/48764136/gcc-noload-directive-cause-wrong-memory-mapping
> 
> What happens is that the effect of the command:
> 
>   .foo (NOLOAD) : {}
> 
> is not equivalent to that of:
> 
>   .foo (NOLOAD) : { *(.foo) }
> 
> when there is more than 1 object file containing a .foo section: the former
> will output two .foo sections, the first with PROGBITS and the second with
> NOBITS, where the latter will output only one with NOBITS.

Yes, the first orphan .foo will be placed in the empty noload output
section but the second orphan .foo will find its SEC_LOAD and
SEC_ALLOC flags don't match the now non-empty output section.

> I think that the commands are essentially equivalent so the linker ought to yield
> the same outcome, namely the single output section with NOBITS.
> 
> Tested on x86-64/Linux, OK for the mainline?
> 
> 
> 2022-04-21  Eric Botcazou  <ebotcazou@adacore.com>
> 
> ld/
> 	* ldelf.c (ldelf_place_orphan): Match only by name for a NOLOAD
> 	output section present in the script.
> 	* testsuite/ld-elf/orphan-13.d: New file.
> 	* testsuite/ld-elf/orphan-13.ld: Likewise.
> 	* testsuite/ld-elf/orphan-13a.s: Likewise.
> 	* testsuite/ld-elf/orphan-13b.s: Likewise.
> 	* testsuite/ld-elf/orphan-13c.s: Likewise.
> 
> -- 
> Eric Botcazou

> diff --git a/ld/ldelf.c b/ld/ldelf.c
> index 4094640b3f7..35718a9113b 100644
> --- a/ld/ldelf.c
> +++ b/ld/ldelf.c
> @@ -2098,13 +2098,17 @@ ldelf_place_orphan (asection *s, const char *secname, int constraint)
>  	   lang_insert_orphan to create a new output section.  */
>  	constraint = SPECIAL;
>  
> -	/* Check to see if we already have an output section statement
> -	   with this name, and its bfd section has compatible flags.
> +	/* Check to see if we already have an output section statement with
> +	   this name, and it was either present in the script with a special
> +	   type (knowing that lang_insert_orphan only creates normal_section
> +	   output sections) or its BFD section has compatible flags.
> +
>  	   If the section already exists but does not have any flags
>  	   set, then it has been created by the linker, possibly as a
>  	   result of a --section-start command line switch.  */
>  	if (os->bfd_section != NULL
> -	    && (os->bfd_section->flags == 0
> +	    && (os->sectype == noload_section

I think this test should be os->sectype >= noload_section, so that
noalloc_section and the typed section variants behave the same as
noload_section.  OK with that change.

> +		|| os->bfd_section->flags == 0
>  		|| (((s->flags ^ os->bfd_section->flags)
>  		     & (SEC_LOAD | SEC_ALLOC)) == 0
>  		    && (!elfinput
> diff --git a/ld/testsuite/ld-elf/orphan-13.d b/ld/testsuite/ld-elf/orphan-13.d
> new file mode 100644
> index 00000000000..8f867dc8938
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/orphan-13.d
> @@ -0,0 +1,12 @@
> +#source: orphan-13a.s
> +#source: orphan-13b.s
> +#source: orphan-13c.s
> +#ld: -T orphan-13.ld
> +#readelf: -S --wide
> +#xfail: [uses_genelf]
> +#xfail: xstormy16-*-*
> +
> +#...
> +  \[[ 0-9]+\] \.foo +NOBITS +[0-9a-f]+ +[0-9a-f]+ +0+30 +0+ +A +0 +0 +[0-9]+
> +  \[[ 0-9]+\] [._][^f].*
> +#pass
> diff --git a/ld/testsuite/ld-elf/orphan-13.ld b/ld/testsuite/ld-elf/orphan-13.ld
> new file mode 100644
> index 00000000000..face613f8bd
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/orphan-13.ld
> @@ -0,0 +1,7 @@
> +SECTIONS
> +{
> +  . = SIZEOF_HEADERS;
> +  .text : { *(.text) }
> +  .data : { *(.data) }
> +  .foo (NOLOAD) : {}
> +}
> diff --git a/ld/testsuite/ld-elf/orphan-13a.s b/ld/testsuite/ld-elf/orphan-13a.s
> new file mode 100644
> index 00000000000..a8a6f364455
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/orphan-13a.s
> @@ -0,0 +1,14 @@
> + .globl main
> + .globl _main
> + .globl start
> + .globl _start
> + .globl __start
> + .text
> +main:
> +_main:
> +start:
> +_start:
> +__start:
> +
> + .section .foo,"a",%progbits
> + .long 1,1,1,1
> diff --git a/ld/testsuite/ld-elf/orphan-13b.s b/ld/testsuite/ld-elf/orphan-13b.s
> new file mode 100644
> index 00000000000..c475eb11d1b
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/orphan-13b.s
> @@ -0,0 +1,2 @@
> + .section .foo,"a",%progbits
> + .long 1,1,1,1
> diff --git a/ld/testsuite/ld-elf/orphan-13c.s b/ld/testsuite/ld-elf/orphan-13c.s
> new file mode 100644
> index 00000000000..c475eb11d1b
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/orphan-13c.s
> @@ -0,0 +1,2 @@
> + .section .foo,"a",%progbits
> + .long 1,1,1,1


-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Orphan sections and NOLOAD output section

[Fangrui Song]

On 2022-04-21, Eric Botcazou via Binutils wrote:
>Hi,
>
>we recently got a report about a counter-intuitive behavior of the GNU linker
>for orphan sections matched by a NOLOAD output section, which turned out to be
>an exact duplicate of:
>  https://stackoverflow.com/questions/48764136/gcc-noload-directive-cause-wrong-memory-mapping
>
>What happens is that the effect of the command:
>
>  .foo (NOLOAD) : {}
>
>is not equivalent to that of:
>
>  .foo (NOLOAD) : { *(.foo) }
>
>when there is more than 1 object file containing a .foo section: the former
>will output two .foo sections, the first with PROGBITS and the second with
>NOBITS, where the latter will output only one with NOBITS.
>
>I think that the commands are essentially equivalent so the linker ought to yield
>the same outcome, namely the single output section with NOBITS.
>
>Tested on x86-64/Linux, OK for the mainline?
>
>
>2022-04-21  Eric Botcazou  <ebotcazou@adacore.com>
>
>ld/
>	* ldelf.c (ldelf_place_orphan): Match only by name for a NOLOAD
>	output section present in the script.
>	* testsuite/ld-elf/orphan-13.d: New file.
>	* testsuite/ld-elf/orphan-13.ld: Likewise.
>	* testsuite/ld-elf/orphan-13a.s: Likewise.
>	* testsuite/ld-elf/orphan-13b.s: Likewise.
>	* testsuite/ld-elf/orphan-13c.s: Likewise.
>
>-- 
>Eric Botcazou

For other folks' convenience, the patch effect is changing

% ld.bfd orphan-13[abc].o -T orphan-13.ld && readelf -S a.out | grep foo
   [ 1] .foo              PROGBITS         0000000000000078  00000078
   [ 2] .foo              NOBITS           0000000000000098  00000098

to

% ~/Dev/binutils-gdb/out/release/ld/ld-new orphan-13[abc].o -T orphan-13.ld && readelf -S a.out | grep foo
   [ 1] .foo              NOBITS           0000000000000078  00000078


I think this is a good move.
I have checked `.foo (TYPE=SHT_NOBITS) : {}` which already uses just one output section.

I think in the future it makes sense to report a warning when NOBITS changes the section type from SHT_PROGBITS to SHT_NOBITS.
This kind of section type change is likely not what the user expects.