Re: ld bug when self-referential variables are used as output section addresses ?

[Alan Modra <amodra at gmail dot com>]

On Thu, Jan 16, 2014 at 02:55:37PM +0100, Samuel Jones wrote:
> I have the following linker script:
> 
> 
> TOTO = 1024;
> TOTO += 1024;
> 
> SECTIONS
> {
>   .rodata    TOTO :
>   {
>   __toto_symbol_abs = ABSOLUTE(TOTO);
>     *(.rodata)
>     *(.rodata.*)
>   }
> 
> }
> 
> 
> When I link this example with the latest ld, the section ".rodata"
> is placed at 1024, whereas I would have expected 2048.
> __toto_symbol_abs gets the value 2048.
> 
> 
> With ld 2.22.52 .rodata is placed at 2048. I believe this commit by
> Alan Modra changed ld's behaviour:
> 4194268f43623a5f893b9a92b0456d3cb43ab915. My understanding of the
> patch is that it delays the evaluation of self-referential
> expressions until section sizing has been completed. We depended on
> the previous behaviour for correct placement of output sections
> (mostly calculating alignment constraints).
> 
> 
> Is what I observe expected behaviour? If so is there any way to
> achieve what I want apart from using static single assignment? And
> why the change?

See https://sourceware.org/ml/binutils/2012-12/msg00155.html

Clearly, your linker script is quite reasonable.  No matter how many
times the script is evaluated, you should get the same result.  The
tricky part is differentiating expressions involving self-assignment 
to absolute symbols with a previous linker script definition from all
other symbols..

Fortunately we already have some machinery in ld to track such
symbols, but it needs some modification.

ld/
	PR ld/14962
	* ldlang.h (struct lang_definedness_hash_entry): Add by_object and
	by_script.  Make iteration a single bit field.
	(lang_track_definedness, lang_symbol_definition_iteration): Delete.
	(lang_symbol_defined): Declare.
	* ldlang.c (lang_statement_iteration): Expand comment a little.
	(lang_init <lang_definedness_table>): Make it bigger.
	(lang_track_definedness, lang_symbol_definition): Delete.
	(lang_definedness_newfunc): Update.
	(lang_symbol_defined): New function.
	(lang_update_definedness): Create entries here.  Do track whether
	script definition of symbol is valid, even when also defined in
	an object file.
	* ldexp.c (fold_name <DEFINED>): Update.
	(fold_name <NAME>): Allow self-assignment for absolute symbols
	defined in a linker script.
ld/testsuite/
	* ld-scripts/pr14962-2.d,
	* ld-scripts/pr14962-2.t: New test.
	* ld-scripts/expr.exp: Run it.

diff --git a/ld/ldexp.c b/ld/ldexp.c
index 49e7c65..9a529db 100644
--- a/ld/ldexp.c
+++ b/ld/ldexp.c
@@ -575,13 +575,10 @@ fold_name (etree_type *tree)
       break;
 
     case DEFINED:
-      if (expld.phase == lang_first_phase_enum)
-	lang_track_definedness (tree->name.name);
-      else
+      if (expld.phase != lang_first_phase_enum)
 	{
 	  struct bfd_link_hash_entry *h;
-	  int def_iteration
-	    = lang_symbol_definition_iteration (tree->name.name);
+	  struct lang_definedness_hash_entry *def;
 
 	  h = bfd_wrapped_link_hash_lookup (link_info.output_bfd,
 					    &link_info,
@@ -591,15 +588,33 @@ fold_name (etree_type *tree)
 		      && (h->type == bfd_link_hash_defined
 			  || h->type == bfd_link_hash_defweak
 			  || h->type == bfd_link_hash_common)
-		      && (def_iteration == lang_statement_iteration
-			  || def_iteration == -1));
+		      && ((def = lang_symbol_defined (tree->name.name)) == NULL
+			  || def->by_object
+			  || def->iteration == (lang_statement_iteration & 1)));
 	}
       break;
 
     case NAME:
       if (expld.assign_name != NULL
 	  && strcmp (expld.assign_name, tree->name.name) == 0)
-	expld.assign_name = NULL;
+	{
+	  /* Self-assignment is only allowed for absolute symbols
+	     defined in a linker script.  */
+	  struct bfd_link_hash_entry *h;
+	  struct lang_definedness_hash_entry *def;
+
+	  h = bfd_wrapped_link_hash_lookup (link_info.output_bfd,
+					    &link_info,
+					    tree->name.name,
+					    FALSE, FALSE, TRUE);
+	  if (!(h != NULL
+		&& (h->type == bfd_link_hash_defined
+		    || h->type == bfd_link_hash_defweak)
+		&& h->u.def.section == bfd_abs_section_ptr
+		&& (def = lang_symbol_defined (tree->name.name)) != NULL
+		&& def->iteration == (lang_statement_iteration & 1)))
+	    expld.assign_name = NULL;
+	}
       if (expld.phase == lang_first_phase_enum)
 	;
       else if (tree->name.name[0] == '.' && tree->name.name[1] == 0)
diff --git a/ld/ldlang.c b/ld/ldlang.c
index f6d713c..9e35527 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -107,7 +107,7 @@ struct lang_phdr *lang_phdr_list;
 struct lang_nocrossrefs *nocrossref_list;
 
  /* Functions that traverse the linker script and might evaluate
-    DEFINED() need to increment this.  */
+    DEFINED() need to increment this at the start of the traversal.  */
 int lang_statement_iteration = 0;
 
 etree_type *base; /* Relocation base - or null */
@@ -1221,16 +1221,12 @@ lang_init (void)
 
   abs_output_section->bfd_section = bfd_abs_section_ptr;
 
-  /* The value "3" is ad-hoc, somewhat related to the expected number of
-     DEFINED expressions in a linker script.  For most default linker
-     scripts, there are none.  Why a hash table then?  Well, it's somewhat
-     simpler to re-use working machinery than using a linked list in terms
-     of code-complexity here in ld, besides the initialization which just
-     looks like other code here.  */
+  /* The value "13" is ad-hoc, somewhat related to the expected number of
+     assignments in a linker script.  */
   if (!bfd_hash_table_init_n (&lang_definedness_table,
 			      lang_definedness_newfunc,
 			      sizeof (struct lang_definedness_hash_entry),
-			      3))
+			      13))
     einfo (_("%P%F: can not create hash table: %E\n"));
 }
 
@@ -2054,7 +2050,7 @@ lang_map (void)
       obstack_begin (&map_obstack, 1000);
       bfd_link_hash_traverse (link_info.hash, sort_def_symbol, 0);
     }
-  lang_statement_iteration ++;
+  lang_statement_iteration++;
   print_statements ();
 }
 
@@ -3286,15 +3282,6 @@ open_input_bfds (lang_statement_union_type *s, enum open_bfd_mode mode)
     einfo ("%F");
 }
 
-/* Add a symbol to a hash of symbols used in DEFINED (NAME) expressions.  */
-
-void
-lang_track_definedness (const char *name)
-{
-  if (bfd_hash_lookup (&lang_definedness_table, name, TRUE, FALSE) == NULL)
-    einfo (_("%P%F: bfd_hash_lookup failed creating symbol %s\n"), name);
-}
-
 /* New-function for the definedness hash table.  */
 
 static struct bfd_hash_entry *
@@ -3312,28 +3299,22 @@ lang_definedness_newfunc (struct bfd_hash_entry *entry,
   if (ret == NULL)
     einfo (_("%P%F: bfd_hash_allocate failed creating symbol %s\n"), name);
 
-  ret->iteration = -1;
+  ret->by_object = 0;
+  ret->by_script = 0;
+  ret->iteration = 0;
   return &ret->root;
 }
 
-/* Return the iteration when the definition of NAME was last updated.  A
-   value of -1 means that the symbol is not defined in the linker script
-   or the command line, but may be defined in the linker symbol table.  */
+/* Called during processing of linker script script expressions.
+   For symbols assigned in a linker script, return a struct describing
+   where the symbol is defined relative to the current expression,
+   otherwise return NULL.  */
 
-int
-lang_symbol_definition_iteration (const char *name)
+struct lang_definedness_hash_entry *
+lang_symbol_defined (const char *name)
 {
-  struct lang_definedness_hash_entry *defentry
-    = (struct lang_definedness_hash_entry *)
-    bfd_hash_lookup (&lang_definedness_table, name, FALSE, FALSE);
-
-  /* We've already created this one on the presence of DEFINED in the
-     script, so it can't be NULL unless something is borked elsewhere in
-     the code.  */
-  if (defentry == NULL)
-    FAIL ();
-
-  return defentry->iteration;
+  return ((struct lang_definedness_hash_entry *)
+	  bfd_hash_lookup (&lang_definedness_table, name, FALSE, FALSE));
 }
 
 /* Update the definedness state of NAME.  */
@@ -3343,25 +3324,20 @@ lang_update_definedness (const char *name, struct bfd_link_hash_entry *h)
 {
   struct lang_definedness_hash_entry *defentry
     = (struct lang_definedness_hash_entry *)
-    bfd_hash_lookup (&lang_definedness_table, name, FALSE, FALSE);
+    bfd_hash_lookup (&lang_definedness_table, name, TRUE, FALSE);
 
-  /* We don't keep track of symbols not tested with DEFINED.  */
   if (defentry == NULL)
-    return;
+    einfo (_("%P%F: bfd_hash_lookup failed creating symbol %s\n"), name);
 
-  /* If the symbol was already defined, and not from an earlier statement
-     iteration, don't update the definedness iteration, because that'd
-     make the symbol seem defined in the linker script at this point, and
-     it wasn't; it was defined in some object.  If we do anyway, DEFINED
-     would start to yield false before this point and the construct "sym =
-     DEFINED (sym) ? sym : X;" would change sym to X despite being defined
-     in an object.  */
-  if (h->type != bfd_link_hash_undefined
+  /* If the symbol was already defined, and not by a script, then it
+     must be defined by an object file.  */
+  if (!defentry->by_script
+      && h->type != bfd_link_hash_undefined
       && h->type != bfd_link_hash_common
-      && h->type != bfd_link_hash_new
-      && defentry->iteration == -1)
-    return;
+      && h->type != bfd_link_hash_new)
+    defentry->by_object = 1;
 
+  defentry->by_script = 1;
   defentry->iteration = lang_statement_iteration;
 }
 
diff --git a/ld/ldlang.h b/ld/ldlang.h
index e91153c..8c815c6 100644
--- a/ld/ldlang.h
+++ b/ld/ldlang.h
@@ -465,7 +465,9 @@ struct unique_sections
 struct lang_definedness_hash_entry
 {
   struct bfd_hash_entry root;
-  int iteration;
+  unsigned int by_object : 1;
+  unsigned int by_script : 1;
+  unsigned int iteration : 1;
 };
 
 /* Used by place_orphan to keep track of orphan sections and statements.  */
@@ -658,8 +660,7 @@ extern void lang_add_unique
   (const char *);
 extern const char *lang_get_output_target
   (void);
-extern void lang_track_definedness (const char *);
-extern int lang_symbol_definition_iteration (const char *);
+extern struct lang_definedness_hash_entry *lang_symbol_defined (const char *);
 extern void lang_update_definedness
   (const char *, struct bfd_link_hash_entry *);
 
diff --git a/ld/testsuite/ld-scripts/expr.exp b/ld/testsuite/ld-scripts/expr.exp
index 0f92d97..ee8da7f 100644
--- a/ld/testsuite/ld-scripts/expr.exp
+++ b/ld/testsuite/ld-scripts/expr.exp
@@ -25,3 +25,4 @@ run_dump_test expr2
 run_dump_test sane1
 run_dump_test assign-loc
 run_dump_test pr14962
+run_dump_test pr14962-2
diff --git a/ld/testsuite/ld-scripts/pr14962-2.d b/ld/testsuite/ld-scripts/pr14962-2.d
new file mode 100644
index 0000000..5e71433
--- /dev/null
+++ b/ld/testsuite/ld-scripts/pr14962-2.d
@@ -0,0 +1,10 @@
+#ld: -T pr14962-2.t
+#source: pr14962a.s
+#nm: -n
+#notarget: rx-*-* frv-linux
+
+#...
+0+2000 [AT] _start
+#...
+0+2000 A x
+#pass
diff --git a/ld/testsuite/ld-scripts/pr14962-2.t b/ld/testsuite/ld-scripts/pr14962-2.t
new file mode 100644
index 0000000..f2c603b
--- /dev/null
+++ b/ld/testsuite/ld-scripts/pr14962-2.t
@@ -0,0 +1,11 @@
+TOTO = 4096;
+TOTO += 4096;
+
+SECTIONS
+{
+  .text TOTO :
+  {
+    x = ABSOLUTE(TOTO);
+    *(*.text)
+  }
+}

-- 
Alan Modra
Australia Development Lab, IBM