This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]