Re: [PATCH] Fix references to __ehdr_start when it cannot be defined

[Roland McGrath <mcgrathr at google dot com>]

On Fri, Nov 15, 2013 at 11:49 PM, Alan Modra <amodra@gmail.com> wrote:
> Right, and bfd_elf_record_link_assignment was designed to handle
> defining symbols..  I owe you and Maciej an apology, since it was my
> suggestion to use bfd_elf_record_link_assignment here.  Let's go back
> to Maciej's original submission, modified somewhat.

Fine by me.

> Maciej, this gets FAIL: MIPS magic __ehdr_start symbol test 2 (o32)
> because the linker script defines __ehdr_start and so the symbol
> is not made hidden and forced_local.  Should we update the testcase,
> or do you think __ehdr_start should always be hidden?  The latter can
> be done by simply removing the h->root.type tests.

IMHO the test case is wrong.  The original intent with __ehdr_start when
I implemented it was that it was doing the equivalent of PROVIDE.  After
Maciej's change (which I agree with), it is doing the equivalent of
PROVIDE_HIDDEN.  This symbol name should not be deeply magical--that is
just bizarre and unlike any other feature I know of.  If the user is
defining it, all the details of the symbol are up to the user.  If the
user refers to it without defining it, we provide it as STV_HIDDEN just
like any PROVIDE_HIDDEN case.

Here is a new version of my patch that uses your code to set STV_HIDDEN
when providing the symbol.  I've adjusted the MIPS test case not to
expect magic hiding of a user-defined symbol with the name __ehdr_start.
I've also added a generic test case that verifies an explicit
linker-script definition doesn't get its visibility perturbed.

Tested on mips-linux-gnu, x86_64-linux-gnu, and x86_64-nacl.

Ok for trunk and 2.24?


Thanks,
Roland


ld/
2013-11-18  Roland McGrath  <mcgrathr@google.com>
	    Alan Modra  <amodra@gmail.com>

	* emultempl/elf32.em (gld${EMULATION_NAME}_before_allocation):
	Don't use bfd_elf_record_link_assignment to mark __ehdr_start
	hidden.  Instead, just do it directly here, and only if it was
	referenced but not defined.

ld/testsuite/
2013-11-18  Roland McGrath  <mcgrathr@google.com>

	* ld-elf/ehdr_start-userdef.t: New file.
	* ld-elf/ehdr_start-userdef.d: New file.
	* ld-elf/ehdr_start-strongref.s: New file.
	* ld-elf/ehdr_start-missing.t: New file.
	* ld-elf/ehdr_start-missing.d: New file.
	* ld-elf/ehdr_start-weak.d: New file.
	* ld-mips-elf/ehdr_start-2.nd: Expect __ehdr_start to be global.

--- a/ld/emultempl/elf32.em
+++ b/ld/emultempl/elf32.em
@@ -1487,10 +1487,25 @@ gld${EMULATION_NAME}_before_allocation (void)

       /* Make __ehdr_start hidden if it has been referenced, to
 	 prevent the symbol from being dynamic.  */
-      if (!bfd_elf_record_link_assignment (link_info.output_bfd, &link_info,
-					   "__ehdr_start", TRUE, TRUE))
-	einfo ("%P%F: failed to record assignment to %s: %E\n",
-	       "__ehdr_start");
+      if (!link_info.relocatable)
+       {
+         struct elf_link_hash_entry *h
+           = elf_link_hash_lookup (elf_hash_table (&link_info), "__ehdr_start",
+                                   FALSE, FALSE, TRUE);
+
+         /* Only adjust the export class if the symbol was referenced
+            and not defined, otherwise leave it alone.  */
+         if (h != NULL
+             && (h->root.type == bfd_link_hash_new
+                 || h->root.type == bfd_link_hash_undefined
+                 || h->root.type == bfd_link_hash_undefweak
+                 || h->root.type == bfd_link_hash_common))
+           {
+             _bfd_elf_link_hash_hide_symbol (&link_info, h, TRUE);
+             if (ELF_ST_VISIBILITY (h->other) != STV_INTERNAL)
+               h->other = (h->other & ~ELF_ST_VISIBILITY (-1)) | STV_HIDDEN;
+           }
+       }

       /* If we are going to make any variable assignments, we need to
 	 let the ELF backend know about them in case the variables are
--- /dev/null
+++ b/ld/testsuite/ld-elf/ehdr_start-missing.d
@@ -0,0 +1,4 @@
+#source: ehdr_start-strongref.s
+#ld: -e _start -T ehdr_start-missing.t
+#error: .*: undefined reference to `__ehdr_start'
+#target: *-*-linux* *-*-gnu* *-*-nacl*
--- /dev/null
+++ b/ld/testsuite/ld-elf/ehdr_start-missing.t
@@ -0,0 +1,8 @@
+SECTIONS
+{
+  . = 0x10000000;
+  .text : { *(.text) }
+
+  . = 0x20000000;
+  .rodata : { *(.rodata) }
+}
--- /dev/null
+++ b/ld/testsuite/ld-elf/ehdr_start-strongref.s
@@ -0,0 +1,9 @@
+	.text
+	.globl _start
+_start:
+	.space 16
+
+	.section .rodata,"a"
+	.globl foo
+foo:
+	.dc.a __ehdr_start
--- /dev/null
+++ b/ld/testsuite/ld-elf/ehdr_start-userdef.d
@@ -0,0 +1,9 @@
+#source: ehdr_start-strongref.s
+#ld: -e _start -T ehdr_start-userdef.t
+#readelf: -Ws
+#target: *-*-linux* *-*-gnu* *-*-nacl*
+
+Symbol table '\.symtab' contains [0-9]+ entries:
+#...
+ *[0-9]+: 0*12345678 +0 +NOTYPE +GLOBAL +DEFAULT +ABS +__ehdr_start
+#pass
--- /dev/null
+++ b/ld/testsuite/ld-elf/ehdr_start-userdef.t
@@ -0,0 +1,10 @@
+SECTIONS
+{
+  . = 0x10000000;
+  .text : { *(.text) }
+
+  __ehdr_start = 0x12345678;
+
+  . = 0x20000000;
+  .rodata : { *(.rodata) }
+}
--- /dev/null
+++ b/ld/testsuite/ld-elf/ehdr_start-weak.d
@@ -0,0 +1,8 @@
+#source: ehdr_start.s
+#ld: -e _start -T ehdr_start-missing.t
+#nm: -n
+#target: *-*-linux* *-*-gnu* *-*-nacl*
+
+#...
+\s+[wU] __ehdr_start
+#pass
--- a/ld/testsuite/ld-mips-elf/ehdr_start-2.nd
+++ b/ld/testsuite/ld-mips-elf/ehdr_start-2.nd
@@ -1,4 +1,4 @@
 Symbol table '\.symtab' contains [0-9]+ entries:
 #...
- *[0-9]+: 0*23400000 +0 (?:NOTYPE|OBJECT) +LOCAL +DEFAULT +[0-9]+ __ehdr_start
+ *[0-9]+: 0*23400000 +0 (?:NOTYPE|OBJECT) +GLOBAL +DEFAULT +[0-9]+ __ehdr_start
 #pass