PATCH: PR ld/12380: Assertion in linker script failed twice

PATCH: PR ld/12380: Assertion in linker script failed twice

[H.J. Lu]

Hi,

This patch avoids duplicated assert messages.  OK to install?

Thanks.


H.J.
----
2012-01-09  H.J. Lu  <hongjiu.lu@intel.com>

	PR ld/12380
	* ldexp.c (exp_fold_tree_1): Check and set issued for
	etree_assert.
	(exp_assert): Initalize issued to FALSE.

	* ldexp.h (etree_union): Add issued to assert_s.

diff --git a/ld/ldexp.c b/ld/ldexp.c
index 3261884..a5b8ad9 100644
--- a/ld/ldexp.c
+++ b/ld/ldexp.c
@@ -723,8 +723,13 @@ exp_fold_tree_1 (etree_type *tree)
 
     case etree_assert:
       exp_fold_tree_1 (tree->assert_s.child);
-      if (expld.phase == lang_final_phase_enum && !expld.result.value)
-	einfo ("%X%P: %s\n", tree->assert_s.message);
+      if (expld.phase == lang_final_phase_enum
+	  && !expld.result.value
+	  && !tree->assert_s.issued)
+	{
+	  einfo ("%X%P: %s\n", tree->assert_s.message);
+	  tree->assert_s.issued = TRUE;
+	}
       break;
 
     case etree_unary:
@@ -1009,6 +1014,7 @@ exp_assert (etree_type *exp, const char *message)
   n->assert_s.type.node_class = etree_assert;
   n->assert_s.child = exp;
   n->assert_s.message = message;
+  n->assert_s.issued = FALSE;
   return n;
 }
 
diff --git a/ld/ldexp.h b/ld/ldexp.h
index 31a06ac..74983bd 100644
--- a/ld/ldexp.h
+++ b/ld/ldexp.h
@@ -90,6 +90,7 @@ typedef union etree_union {
     node_type type;
     union etree_union *child;
     const char *message;
+    bfd_boolean issued;
   } assert_s;
 } etree_type;
 

Re: PATCH: PR ld/12380: Assertion in linker script failed twice

[Nick Clifton]

Hi H.J.

> This patch avoids duplicated assert messages.  OK to install?

The patch needs to include a testsuite entry as well.

Cheers
   Nick

Re: PATCH: PR ld/12380: Assertion in linker script failed twice

[H.J. Lu]

On Mon, Jan 10, 2011 at 5:41 AM, Nick Clifton <nickc@redhat.com> wrote:
> Hi H.J.
>
>> This patch avoids duplicated assert messages.  OK to install?
>
> The patch needs to include a testsuite entry as well.
>

I tried to create a testcase. But I couldn't find a way to check there was
no duplicated linker warning. The only testcase I can find is to check
the linker warning, which passes with and without my patch.


-- 
H.J.

Re: PATCH: PR ld/12380: Assertion in linker script failed twice

[Alan Modra]

On Sun, Jan 09, 2011 at 10:06:12AM -0800, H.J. Lu wrote:
> Hi,
> 
> This patch avoids duplicated assert messages.  OK to install?

This is not the correct fix.  The reason we get duplicate messages is
that relaxation and ELF section header sizing cause multiple
iterations of section sizing/placement.  Since an assert can be based
on section size and/or address, the assert you want is the one from
the *last* pass, not the first one as you have in your patch.

I believe the real problem is that we set lang_final_phase too soon,
so something like the following untested patch should work.

	* ldlang.c (lang_size_sections): Don't set final phase here.
	(lang_process): Do so here, just before lang_do_assignments.

Index: ld/ldlang.c
===================================================================
RCS file: /cvs/src/src/ld/ldlang.c,v
retrieving revision 1.355
diff -u -p -r1.355 ldlang.c
--- ld/ldlang.c	20 Dec 2010 13:00:13 -0000	1.355
+++ ld/ldlang.c	11 Jan 2011 01:23:55 -0000
@@ -5427,8 +5427,6 @@ lang_size_sections (bfd_boolean *relax, 
 	  one_lang_size_sections_pass (relax, check_regions);
 	}
     }
-
-  expld.phase = lang_final_phase_enum;
 }
 
 /* Worker function for lang_do_assignments.  Recursiveness goes here.  */
@@ -6522,6 +6520,7 @@ lang_process (void)
   /* Do all the assignments, now that we know the final resting places
      of all the symbols.  */
 
+  expld.phase = lang_final_phase_enum;
   lang_do_assignments ();
 
   ldemul_finish ();

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: PR ld/12380: Assertion in linker script failed twice

[H.J. Lu]

On Mon, Jan 10, 2011 at 5:27 PM, Alan Modra <amodra@gmail.com> wrote:
> On Sun, Jan 09, 2011 at 10:06:12AM -0800, H.J. Lu wrote:
>> Hi,
>>
>> This patch avoids duplicated assert messages.  OK to install?
>
> This is not the correct fix.  The reason we get duplicate messages is
> that relaxation and ELF section header sizing cause multiple
> iterations of section sizing/placement.  Since an assert can be based
> on section size and/or address, the assert you want is the one from
> the *last* pass, not the first one as you have in your patch.
>
> I believe the real problem is that we set lang_final_phase too soon,
> so something like the following untested patch should work.
>
>        * ldlang.c (lang_size_sections): Don't set final phase here.
>        (lang_process): Do so here, just before lang_do_assignments.
>

Do we know the impact of such change on existing linker scripts?


-- 
H.J.

Re: PATCH: PR ld/12380: Assertion in linker script failed twice

[Alan Modra]

On Mon, Jan 10, 2011 at 06:45:03PM -0800, H.J. Lu wrote:
> Do we know the impact of such change on existing linker scripts?

I said it was untested!  I didn't even run the linker testsuite.
Doing so reveals a bunch of fails that I'll need to fix before the
patch can be committed.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: PR ld/12380: Assertion in linker script failed twice

[H.J. Lu]

On Mon, Jan 10, 2011 at 9:38 PM, Alan Modra <amodra@gmail.com> wrote:
> On Mon, Jan 10, 2011 at 06:45:03PM -0800, H.J. Lu wrote:
>> Do we know the impact of such change on existing linker scripts?
>
> I said it was untested!  I didn't even run the linker testsuite.
> Doing so reveals a bunch of fails that I'll need to fix before the
> patch can be committed.
>

I think you need test more than just linker testsuites.

-- 
H.J.

Re: PATCH: PR ld/12380: Assertion in linker script failed twice

[Alan Modra]

On Tue, Jan 11, 2011 at 11:57:59AM +1030, Alan Modra wrote:
> I believe the real problem is that we set lang_final_phase too soon,

Jakub, would you please cast your eye over this patch that touches
DATA_SEGMENT_ALIGN support?  I'm committing it as I'm reasonably
confident the patch is correct, but would appreciate your review.

	PR ld/12380
	* ldexp.h (enum phase_enum): Comment.  Add exp_dataseg_done.
	* ldexp.c (fold_unary <DATA_SEGMENT_END>): Rearrange code.  Test
	for exp_dataseg_done rather than expld.phase == lang_final_phase_enum
	to detect when we've finished sizing sections.
	(fold_binary <DATA_SEGMENT_ALIGN>): Likewise.
	(fold_binary <DATA_SEGMENT_RELRO_END>): Likewise.  Also test
	that we are not inside an output section statement.
	* ldlang.c (lang_size_sections): Set exp_dataseg_done on exit if
	not exp_dataseg_relro_adjust or exp_dataseg_adjust.  Don't set
	lang_final_phase_enum here.
	(lang_process): Set lang_final_phase_enum here.

Index: ld/ldexp.h
===================================================================
RCS file: /cvs/src/src/ld/ldexp.h,v
retrieving revision 1.25
diff -u -p -r1.25 ldexp.h
--- ld/ldexp.h	20 Dec 2010 13:00:13 -0000	1.25
+++ ld/ldexp.h	12 Jan 2011 06:22:23 -0000
@@ -103,12 +103,17 @@ typedef enum {
 union lang_statement_union;
 
 enum phase_enum {
+  /* We step through the first four states here as we see the
+     associated linker script tokens.  */
   exp_dataseg_none,
   exp_dataseg_align_seen,
   exp_dataseg_relro_seen,
   exp_dataseg_end_seen,
+  /* The last three states are final, and affect the value returned
+     by DATA_SEGMENT_ALIGN.  */
   exp_dataseg_relro_adjust,
-  exp_dataseg_adjust
+  exp_dataseg_adjust,
+  exp_dataseg_done
 };
 
 enum relro_enum {
Index: ld/ldexp.c
===================================================================
RCS file: /cvs/src/src/ld/ldexp.c,v
retrieving revision 1.89
diff -u -p -r1.89 ldexp.c
--- ld/ldexp.c	20 Dec 2010 13:00:13 -0000	1.89
+++ ld/ldexp.c	12 Jan 2011 08:15:23 -0000
@@ -259,20 +259,22 @@ fold_unary (etree_type *tree)
 	  break;
 
 	case DATA_SEGMENT_END:
-	  if (expld.phase != lang_first_phase_enum
-	      && expld.section == bfd_abs_section_ptr
-	      && (expld.dataseg.phase == exp_dataseg_align_seen
-		  || expld.dataseg.phase == exp_dataseg_relro_seen
-		  || expld.dataseg.phase == exp_dataseg_adjust
-		  || expld.dataseg.phase == exp_dataseg_relro_adjust
-		  || expld.phase == lang_final_phase_enum))
+	  if (expld.phase == lang_first_phase_enum
+	      || expld.section != bfd_abs_section_ptr)
 	    {
-	      if (expld.dataseg.phase == exp_dataseg_align_seen
-		  || expld.dataseg.phase == exp_dataseg_relro_seen)
-		{
-		  expld.dataseg.phase = exp_dataseg_end_seen;
-		  expld.dataseg.end = expld.result.value;
-		}
+	      expld.result.valid_p = FALSE;
+	    }
+	  else if (expld.dataseg.phase == exp_dataseg_align_seen
+		   || expld.dataseg.phase == exp_dataseg_relro_seen)
+	    {
+	      expld.dataseg.phase = exp_dataseg_end_seen;
+	      expld.dataseg.end = expld.result.value;
+	    }
+	  else if (expld.dataseg.phase == exp_dataseg_done
+		   || expld.dataseg.phase == exp_dataseg_adjust
+		   || expld.dataseg.phase == exp_dataseg_relro_adjust)
+	    {
+	      /* OK.  */
 	    }
 	  else
 	    expld.result.valid_p = FALSE;
@@ -402,12 +404,10 @@ fold_binary (etree_type *tree)
 
 	case DATA_SEGMENT_ALIGN:
 	  expld.dataseg.relro = exp_dataseg_relro_start;
-	  if (expld.phase != lang_first_phase_enum
-	      && expld.section == bfd_abs_section_ptr
-	      && (expld.dataseg.phase == exp_dataseg_none
-		  || expld.dataseg.phase == exp_dataseg_adjust
-		  || expld.dataseg.phase == exp_dataseg_relro_adjust
-		  || expld.phase == lang_final_phase_enum))
+	  if (expld.phase == lang_first_phase_enum
+	      || expld.section != bfd_abs_section_ptr)
+	    expld.result.valid_p = FALSE;
+	  else
 	    {
 	      bfd_vma maxpage = lhs.value;
 	      bfd_vma commonpage = expld.result.value;
@@ -415,10 +415,20 @@ fold_binary (etree_type *tree)
 	      expld.result.value = align_n (expld.dot, maxpage);
 	      if (expld.dataseg.phase == exp_dataseg_relro_adjust)
 		expld.result.value = expld.dataseg.base;
-	      else if (expld.dataseg.phase != exp_dataseg_adjust)
+	      else if (expld.dataseg.phase == exp_dataseg_adjust)
+		{
+		  if (commonpage < maxpage)
+		    expld.result.value += ((expld.dot + commonpage - 1)
+					   & (maxpage - commonpage));
+		}
+	      else
 		{
 		  expld.result.value += expld.dot & (maxpage - 1);
-		  if (expld.phase == lang_allocating_phase_enum)
+		  if (expld.dataseg.phase == exp_dataseg_done)
+		    {
+		      /* OK.  */
+		    }
+		  else if (expld.dataseg.phase == exp_dataseg_none)
 		    {
 		      expld.dataseg.phase = exp_dataseg_align_seen;
 		      expld.dataseg.min_base = expld.dot;
@@ -427,22 +437,21 @@ fold_binary (etree_type *tree)
 		      expld.dataseg.maxpagesize = maxpage;
 		      expld.dataseg.relro_end = 0;
 		    }
+		  else
+		    expld.result.valid_p = FALSE;
 		}
-	      else if (commonpage < maxpage)
-		expld.result.value += ((expld.dot + commonpage - 1)
-				       & (maxpage - commonpage));
 	    }
-	  else
-	    expld.result.valid_p = FALSE;
 	  break;
 
 	case DATA_SEGMENT_RELRO_END:
 	  expld.dataseg.relro = exp_dataseg_relro_end;
-	  if (expld.phase != lang_first_phase_enum
-	      && (expld.dataseg.phase == exp_dataseg_align_seen
-		  || expld.dataseg.phase == exp_dataseg_adjust
-		  || expld.dataseg.phase == exp_dataseg_relro_adjust
-		  || expld.phase == lang_final_phase_enum))
+	  if (expld.phase == lang_first_phase_enum
+	      || expld.section != bfd_abs_section_ptr)
+	    expld.result.valid_p = FALSE;
+	  else if (expld.dataseg.phase == exp_dataseg_align_seen
+		   || expld.dataseg.phase == exp_dataseg_adjust
+		   || expld.dataseg.phase == exp_dataseg_relro_adjust
+		   || expld.dataseg.phase == exp_dataseg_done)
 	    {
 	      if (expld.dataseg.phase == exp_dataseg_align_seen
 		  || expld.dataseg.phase == exp_dataseg_relro_adjust)
Index: ld/ldlang.c
===================================================================
RCS file: /cvs/src/src/ld/ldlang.c,v
retrieving revision 1.356
diff -u -p -r1.356 ldlang.c
--- ld/ldlang.c	10 Jan 2011 13:13:32 -0000	1.356
+++ ld/ldlang.c	12 Jan 2011 06:22:27 -0000
@@ -5425,9 +5425,11 @@ lang_size_sections (bfd_boolean *relax, 
 	  lang_reset_memory_regions ();
 	  one_lang_size_sections_pass (relax, check_regions);
 	}
+      else
+	expld.dataseg.phase = exp_dataseg_done;
     }
-
-  expld.phase = lang_final_phase_enum;
+  else
+    expld.dataseg.phase = exp_dataseg_done;
 }
 
 /* Worker function for lang_do_assignments.  Recursiveness goes here.  */
@@ -6520,7 +6522,7 @@ lang_process (void)
 
   /* Do all the assignments, now that we know the final resting places
      of all the symbols.  */
-
+  expld.phase = lang_final_phase_enum;
   lang_do_assignments ();
 
   ldemul_finish ();

-- 
Alan Modra
Australia Development Lab, IBM