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

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

[Roland McGrath]

The change Maciej made to make the __ehdr_start symbol hidden broke the
cases when __ehdr_start cannot be defined (because the beginning of the
file does not appear in the memory image).

The failure mode was a BFD abort in elf_link_output_extsym.  When all the
references to __ehdr_start were weak, there were also undefined symbol
messages (which of course there should never be for weak references) before
the abort.

This fixes it by moving the logic that changes the __ehdr_start symbol's
visibility to be part of the logic that creates the symbol in the first
place (which does not run at all when the layout makes it impossible to
give it a proper definition).  I also added test coverage for both weak and
strong references to __ehdr_start when it cannot be defined.

I don't know why the call was put where it was (the before_allocation hook)
rather than where I moved it (the place where the __ehdr_start symbol is
created) and the latter seems the far more obvious thing to do, so perhaps
there is some reason the former is better that I don't understand.

I have tested this only for {x86_64,i386}-{linux-gnu,nacl} targets.
I am not set up to test any mips targets.

OK for trunk and 2.24?


Thanks,
Roland


bfd/
	* elf.c (assign_file_positions_for_non_load_sections):
	Call bfd_elf_record_link_assignment here to mark __ehdr_start as
	STV_HIDDEN when creating it.

ld/
	* emultempl/elf32.em (gld${EMULATION_NAME}_before_allocation):
	Don't call bfd_elf_record_link_assignment on __ehdr_start here.

ld/testsuite/
	* 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.

--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -4962,6 +4962,14 @@ assign_file_positions_for_non_load_sections (bfd *abfd,
 	  hash->root.type = bfd_link_hash_defined;
 	  hash->def_regular = 1;
 	  hash->non_elf = 0;
+
+	  /* 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))
+	    (*link_info->callbacks->einfo)
+	      (_("%P%F: failed to record assignment to %s: %E\n"),
+	       "__ehdr_start");
 	}
     }

--- a/ld/emultempl/elf32.em
+++ b/ld/emultempl/elf32.em
@@ -1485,13 +1485,6 @@ gld${EMULATION_NAME}_before_allocation (void)
     {
       _bfd_elf_tls_setup (link_info.output_bfd, &link_info);

-      /* 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 we are going to make any variable assignments, we need to
 	 let the ELF backend know about them in case the variables are
 	 referred to by dynamic objects.  */
--- /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-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+w __ehdr_start
+#pass

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

[Maciej W. Rozycki]

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

On Thu, 7 Nov 2013, Roland McGrath wrote:

> I don't know why the call was put where it was (the before_allocation hook)
> rather than where I moved it (the place where the __ehdr_start symbol is
> created) and the latter seems the far more obvious thing to do, so perhaps
> there is some reason the former is better that I don't understand.

 Unfortunately this regresses the MIPS tests that I added back when making 
the original fix -- for the very reason the fix was placed where it was
(please see http://sourceware.org/ml/binutils/2013-04/msg00260.html and
http://sourceware.org/bugzilla/show_bug.cgi?id=15365 for further 
reference):

mips-linux-gnu-ld: not enough GOT space for local GOT entries
mips-linux-gnu-ld: BFD 2.24.51.20131107 internal error, aborting at .../bfd/elfxx-mips.c line 10042 in _bfd_mips_elf_relocate_section

mips-linux-gnu-ld: Please report this bug.

FAIL: MIPS magic __ehdr_start symbol test 1 (o32)

and likewise:

FAIL: MIPS magic __ehdr_start symbol test 2 (o32)
FAIL: MIPS magic __ehdr_start symbol test 1 (n32)
FAIL: MIPS magic __ehdr_start symbol test 1 (n64)

 Please try finding a better fix that can decide whether __ehdr_start can 
or cannot be defined no later in the link flow than where I put my change, 
i.e. before bfd_elf_size_dynamic_sections has been called.  All symbols 
and their bindings have to be known by the time that function is called, 
you cannot add any symbols later on.

> I have tested this only for {x86_64,i386}-{linux-gnu,nacl} targets.
> I am not set up to test any mips targets.

 Setting up testing binutils e.g. for mips-linux-gnu should be fairly 
straightforward (unlike with other pieces of the toolchain), e.g.:

$ cd path/to/binutils
$ mkdir obj
$ cd obj
$ ../configure --target=mips-linux-gnu
$ make
$ make -k -i check

 And in any case for changes touching generic pieces of binutils I 
recommend regression-testing across a reasonable number of targets, e.g. 
I've been using a script that Alan Modra posted here a while ago 
(attached).  I have modified it a bit to add some more MIPS targets; 
please feel free to tweak the list to suit your needs (and set --build 
appropriately).

  Maciej

[-- Attachment #2: Type: application/x-sh, Size: 2994 bytes --]

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

[Roland McGrath]

On Thu, Nov 7, 2013 at 4:44 PM, Maciej W. Rozycki
<macro@codesourcery.com> wrote:
>  Unfortunately this regresses the MIPS tests that I added back when making
> the original fix -- for the very reason the fix was placed where it was
> (please see http://sourceware.org/ml/binutils/2013-04/msg00260.html and
> http://sourceware.org/bugzilla/show_bug.cgi?id=15365 for further
> reference):

Thanks for the pointers.  The most helpful thing to do is to write
comments in the code itself that explain why it's where it is and why
that matters.  The comment there now only says (vaguely) why it should
be marked hidden.  There is no explanation for the utterly nonobvious
issue that it's being marked hidden in this code before it's defined by
some far-off other code.

I should not have to come back later and ask (as I had to), "Why is this
code here?"  It's of course fine if I have to come back later and ask,
"How do I address the needs pointed out by this comment?"  But the
comments in the code should at least give some clue about the rationale.

>  Please try finding a better fix that can decide whether __ehdr_start can
> or cannot be defined no later in the link flow than where I put my change,
> i.e. before bfd_elf_size_dynamic_sections has been called.

I'm not really sure that's possible.  It's only knowable when deciding
the file layout, which is after the address layout has been done.  If
you know another place than assign_file_positions_for_non_load_sections
where the __ehdr_start decision could be made correctly in all cases,
please enlighten me.

We're about at the limit of my understanding of the linker's
organization.  Given that what I'm trying to fix is a regression in the
original __ehdr_start behavior that was introduced by your change to
support MIPS (which was not fixing any regression, just inability to use
the new feature in all circumstances on MIPS), I hope you will put some
effort into figuring out how to fix the regression you introduced.

I should have included in the first place the test cases that I've added
in my patch.  That extends the test coverage to the case that your
change broke.  (So of course if I had included those to begin with, you
would have done then as part of your MIPS implementation what I'm trying
to do now to fix the regression you introduced, and it would have stayed
entirely your problem!)

>  Setting up testing binutils e.g. for mips-linux-gnu should be fairly
> straightforward (unlike with other pieces of the toolchain), e.g.:

Duh.  Sorry about the thinko.  Obviously I can test any binutils target
at least as far as 'make check'.

The problem (vs the original __ehdr_start implementation) seems to arise this:

      /* Since we're defining the symbol, don't let it seem to have not
	 been defined.  record_dynamic_symbol and size_dynamic_sections
	 may depend on this.  */
      h->root.type = bfd_link_hash_new;
      if (h->root.u.undef.next != NULL || htab->root.undefs_tail == &h->root)
	bfd_link_repair_undef_list (&htab->root);

in bfd_elf_record_link_assignment.

In the case of __ehdr_start, we're not defining the symbol and it should
indeed seem to have not been defined if it is not defined later.  My
test case become happy if I just avoid those statements for this use.
(The only other use of the function is for symbol assignments actually
made explicitly in linker scripts.)

But "record_dynamic_symbol and size_dynamic_sections may depend on this"
is far from being enough for me to have any idea what issues the code is
addressing.  So I don't know if there is some way those issues could
apply to a case with __ehdr_start and thus arise with this change.

So here's a new patch that passes both my new tests and the existing
MIPS tests.  But I sure would like someone to claim sufficient
understanding of the comment cited above either to formulate a test case
where things go wrong because of it or to assure me categorically that
there can be no such issue for __ehdr_start (a hidden symbol that should
never have any effect on dynamic symbol tables or other dynamic sections
for any reasons I understand--but my that same logic, adding such a
symbol later would not affect MIPS the way it apparently does, so
clearly there are reasons I don't understand).


Thanks,
Roland


bfd/
2013-11-11  Roland McGrath  <mcgrathr@google.com>

	* elflink.c (bfd_elf_record_link_assignment): Take new flag
	argument MAYBE_UNDEFINED.  If true, do not morph types
	bfd_link_hash_undefweak or bfd_link_hash_undefined into
	bfd_link_hash_new.
	* bfd-in.h: Update decl.
	* bfd-in2.h: Regenerate.

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

	* emultempl/elf32.em (gld${EMULATION_NAME}_find_exp_assignment):
	Pass new argument to bfd_elf_record_link_assignment, false.
	(gld${EMULATION_NAME}_before_allocation):
	Pass new argument to bfd_elf_record_link_assignment, true.
	Mark error message string for translation.

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

	* 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.

--- a/bfd/bfd-in.h
+++ b/bfd/bfd-in.h
@@ -641,7 +641,7 @@ enum notice_asneeded_action {

 extern bfd_boolean bfd_elf_record_link_assignment
   (bfd *, struct bfd_link_info *, const char *, bfd_boolean,
-   bfd_boolean);
+   bfd_boolean, bfd_boolean);
 extern struct bfd_link_needed_list *bfd_elf_get_needed_list
   (bfd *, struct bfd_link_info *);
 extern bfd_boolean bfd_elf_get_bfd_needed_list
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -648,7 +648,7 @@ enum notice_asneeded_action {

 extern bfd_boolean bfd_elf_record_link_assignment
   (bfd *, struct bfd_link_info *, const char *, bfd_boolean,
-   bfd_boolean);
+   bfd_boolean, bfd_boolean);
 extern struct bfd_link_needed_list *bfd_elf_get_needed_list
   (bfd *, struct bfd_link_info *);
 extern bfd_boolean bfd_elf_get_bfd_needed_list
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -498,7 +498,8 @@ bfd_elf_record_link_assignment (bfd *output_bfd,
 				struct bfd_link_info *info,
 				const char *name,
 				bfd_boolean provide,
-				bfd_boolean hidden)
+				bfd_boolean hidden,
+				bfd_boolean maybe_undefined)
 {
   struct elf_link_hash_entry *h, *hv;
   struct elf_link_hash_table *htab;
@@ -520,6 +521,8 @@ bfd_elf_record_link_assignment (bfd *output_bfd,
       break;
     case bfd_link_hash_undefweak:
     case bfd_link_hash_undefined:
+      if (maybe_undefined)
+	break;
       /* Since we're defining the symbol, don't let it seem to have not
 	 been defined.  record_dynamic_symbol and size_dynamic_sections
 	 may depend on this.  */
--- a/ld/emultempl/elf32.em
+++ b/ld/emultempl/elf32.em
@@ -1374,7 +1374,7 @@ gld${EMULATION_NAME}_find_exp_assignment (etree_type *exp)
 	  if (!bfd_elf_record_link_assignment (link_info.output_bfd,
 					       &link_info,
 					       exp->assign.dst, provide,
-					       exp->assign.hidden))
+					       exp->assign.hidden, FALSE))
 	    einfo ("%P%F: failed to record assignment to %s: %E\n",
 		   exp->assign.dst);
 	}
@@ -1488,8 +1488,8 @@ 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", TRUE, TRUE, TRUE))
+	einfo (_("%P%F: failed to record assignment to %s: %E\n"),
 	       "__ehdr_start");

       /* If we are going to make any variable assignments, we need to
--- /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-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

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

[Alan Modra]

On Mon, Nov 11, 2013 at 03:06:54PM -0800, Roland McGrath wrote:
> I'm not really sure that's possible.  It's only knowable when deciding
> the file layout, which is after the address layout has been done.  If
[snip]

It's quite possible to define a symbol then come back later and fix
the value, but this is all by the way.

[snip]
> The problem (vs the original __ehdr_start implementation) seems to arise this:
> 
>       /* Since we're defining the symbol, don't let it seem to have not
> 	 been defined.  record_dynamic_symbol and size_dynamic_sections
> 	 may depend on this.  */
>       h->root.type = bfd_link_hash_new;
>       if (h->root.u.undef.next != NULL || htab->root.undefs_tail == &h->root)
> 	bfd_link_repair_undef_list (&htab->root);
> 
> in bfd_elf_record_link_assignment.
> 
> In the case of __ehdr_start, we're not defining the symbol and it should

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.

Ideally, we would have some function to prevent symbols from being
made dynamic, which is what elf32.em:before_allocation is trying to do
for "__ehdr_start".  See also elf64-ppc.c:ppc64_elf_func_desc_adjust
where the same thing is done for ".TOC." without knowing the proper
value.

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.

	* emultempl/elf32.em (before_allocation): Don't use
	bfd_link_record_link_assignment to hide __ehdr_start.

diff --git a/ld/emultempl/elf32.em b/ld/emultempl/elf32.em
index 682f5e5..5f776a0 100644
--- a/ld/emultempl/elf32.em
+++ b/ld/emultempl/elf32.em
@@ -1487,10 +1487,26 @@ 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;
+
+	  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


-- 
Alan Modra
Australia Development Lab, IBM

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

[Roland McGrath]

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

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

[Maciej W. Rozycki]

On Mon, 18 Nov 2013, Roland McGrath wrote:

> 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.

 Alan, thanks for stepping in and sorting out this issue.

> > 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.

 Roland, the test case isn't right or wrong, it merely reflects the 
current semantics.  I have no objections to making __ehdr_start so special 
as to change the attribute depending on whether it's defined by a user 
object or provided by the linker.  In fact I find it natural this way, 
however please note that it's going to be a unique arrangement that cannot 
be recreated for another, non-special symbol with the available command 
line and linker script syntax as this will match neither PROVIDE (that'd 
make a symbol provided by the linker global and respect the export class 
chosen by the user otherwise) nor PROVIDE_HIDDEN (that'd make it hidden no 
matter if provided by the linker or already defined as global by a user 
object).

 In fact we already had a discussion about PROVIDE_HIDDEN where I 
suggested making the command not hide symbols defined by user objects, but 
my proposal was rejected -- see the two parts of the thread here:

http://sourceware.org/ml/binutils/2013-04/msg00315.html
http://sourceware.org/ml/binutils/2013-05/msg00003.html

So while I'm in favour to your approach, with the current semantics of 
PROVIDE and PROVIDE_HIDDEN it is your proposal that is making an exception 
to the usual rules and not my previous implementation (that you correctly 
observed mimics PROVIDE_HIDDEN).

> 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.

 Thanks, I have tested it as well and I see no problems with it.  However 
I think in this case gold/layout.cc should be adjusted accordingly.

 And for the record -- since you were surprised as to why a hidden symbol 
should take any effect on dynamic sections -- the original MIPS SVR4 psABI 
has been formulated such that there is no dynamic relocation defined and 
used to relocate local GOT entries.  Instead the GOT is split into the 
local part that comes first and the global part that goes next.  The point 
of the split is indicated by the DT_MIPS_LOCAL_GOTNO dynamic section tag 
and all the GOT entries before that point undergo implicit local 
relocation.  Therefore local GOT entries cannot be inserted at arbitrary 
places in the GOT.  Then the point of the split is determined by 
bfd_elf_size_dynamic_sections and therefore no further local symbols can 
be created afterwards.

 Furthermore the ABI does not use a PLT or copy relocations, not even for 
executables and all final binaries are PIC, so all the consequences apply, 
local GOT entries still need to be made, although normally they won't need 
any run-time relocation as the base address is supposed to be always 0.  
To avoid these drawbacks an ABI extension has been developed that uses a 
PLT and copy relocations for executables, but the original MIPS SVR4 psABI 
remains supported (and also the default for the 64-bit "new" ABIs).

 I hope this explanation makes the matter clear, please feel free to ask 
further if I missed anything you might be interested in.

  Maciej

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

[Alan Modra]

On Mon, Nov 18, 2013 at 03:03:40PM -0800, Roland McGrath wrote:
> 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.

OK.

-- 
Alan Modra
Australia Development Lab, IBM

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

[Roland McGrath]

Committed.

Thanks,
Roland

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

[Roland McGrath]

On Mon, Nov 18, 2013 at 4:46 PM, Maciej W. Rozycki
<macro@codesourcery.com> wrote:
>  Roland, the test case isn't right or wrong, it merely reflects the
> current semantics.  I have no objections to making __ehdr_start so special
> as to change the attribute depending on whether it's defined by a user
> object or provided by the linker.  In fact I find it natural this way,
> however please note that it's going to be a unique arrangement that cannot
> be recreated for another, non-special symbol with the available command
> line and linker script syntax as this will match neither PROVIDE (that'd
> make a symbol provided by the linker global and respect the export class
> chosen by the user otherwise) nor PROVIDE_HIDDEN (that'd make it hidden no
> matter if provided by the linker or already defined as global by a user
> object).

I am surprised to learn that PROVIDE_HIDDEN has those semantics.  Like you,
I am not convinced at all by the rationales given in the earlier threads.
But I'm not going to argue about what PROVIDE_HIDDEN does.

I think the argument against magic hiding of a symbol a user defined with
different visibility is even stronger when it's built-in magic like
__ehdr_start as opposed to something defined in a linker script.  (After
all, you can override the linker script with a different one.  But there is
no way to prevent the magic provision of __ehdr_start when it's undefined.)

Moreover, I now have maintainer approval for my patch as is and so I'm just
going to be done with it now.

>  Thanks, I have tested it as well and I see no problems with it.  However
> I think in this case gold/layout.cc should be adjusted accordingly.

I don't understand.  In what case is gold now inconsistent?
It already didn't perturb the visibility of a user-defined __ehdr_start.

>  And for the record [...]

Thanks for the background.  I still don't understand the MIPS oddities very
clearly, but I think I understand this issue adequately now.


Thanks,
Roland

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

[Maciej W. Rozycki]

On Tue, 19 Nov 2013, Roland McGrath wrote:

> >  Thanks, I have tested it as well and I see no problems with it.  However
> > I think in this case gold/layout.cc should be adjusted accordingly.
> 
> I don't understand.  In what case is gold now inconsistent?
> It already didn't perturb the visibility of a user-defined __ehdr_start.

 OK, I misunderstood how it works then, sorry about that and thanks for 
confirming it does the right thing.

  Maciej