Re: [PATCH] MIPS/gas: Redefined label alignment assertion failure fix

[Richard Sandiford <rdsandiford at googlemail dot com>]

"Maciej W. Rozycki" <macro@linux-mips.org> writes:
>  As noted with the previous submission, there is an internal 
> error/assertion failure seen in the "is already defined" test case.  The 
> reason is a label has been (incorrectly) redefined and as a result it 
> resides in a different segment to the current one, which the alignment 
> operation that fails applies to.  I believe it is safe to convert the 
> assertion failure to a condition around the alignment operation; as the 
> redefinition has been incorrect, GAS will have failed regardless.

Good catch!

>  The fix below removes the assertion failure making the "is already 
> defined" test case behave as expected.
>
> 2009-09-12  Maciej W. Rozycki  <macro@linux-mips.org>
>
> 	* config/tc-mips.c (mips_align): Don't align the label if from 
> 	another segment; remove the assertion failure for same.
>
>  Regression tested for the mipsel-linux and mips64-linux targets.  OK to 
> apply to the trunk?  I suggest to propagate it to 2.20; we shouldn't be 
> nourishing invalid assertion failures.

I suppose we first of all need to decide whether the target-independent
code is doing the right thing by adding the faulty label to the
segment's list in the first place, even though the label doesn't
belong to that segment.  I've no opinion either way, just thought
it needed to be asked.

Assuming the target-independent code is OK, then I agree your change is
correct.  I'm just worried that, in order to see it's correct, you first
need to go looking through the callers to see how they provide "label".
I think it'd be cleaner to move the code inside mips_align itself, so
that the logic is more obvious.  I realise that, with the branch in mind,
you were probably wanting to be as non-invasive as possible, but what do
you think of the following completely-untested alternative?

Richard


gas/
2009-09-13  Maciej W. Rozycki  <macro@linux-mips.org>
	    Richard Sandiford  <rdsandiford@googlemail.com>

	* config/tc-mips.c (mips_align): Remove label argument and
	calculate it internally instead.  Don't align the label if from
	another segment; remove the assertion failure for same.
	(s_align): Update call accordingly.
	(s_cons): Likewise.
	(s_float_cons): Likewise.
	(s_gpword): Likewise.
	(s_gpdword): Likewise.

Index: gas/config/tc-mips.c
===================================================================
--- gas/config/tc-mips.c	2009-09-13 11:22:51.000000000 +0100
+++ gas/config/tc-mips.c	2009-09-13 11:27:01.000000000 +0100
@@ -12439,8 +12439,10 @@ get_symbol (void)
    label.  */
 
 static void
-mips_align (int to, int *fill, symbolS *label)
+mips_align (int to, int *fill)
 {
+  struct insn_label_list *l;
+
   mips_emit_delays ();
   mips_record_mips16_mode ();
   if (fill == NULL && subseg_text_p (now_seg))
@@ -12448,11 +12450,11 @@ mips_align (int to, int *fill, symbolS *
   else
     frag_align (to, fill ? *fill : 0, 0);
   record_alignment (now_seg, to);
-  if (label != NULL)
+  l = seg_info (now_seg)->label_list;
+  if (l && l->label && S_GET_SEGMENT (l->label) == now_seg)
     {
-      gas_assert (S_GET_SEGMENT (label) == now_seg);
-      symbol_set_frag (label, frag_now);
-      S_SET_VALUE (label, (valueT) frag_now_fix ());
+      symbol_set_frag (l->label, frag_now);
+      S_SET_VALUE (l->label, (valueT) frag_now_fix ());
     }
 }
 
@@ -12491,11 +12493,9 @@ s_align (int x ATTRIBUTE_UNUSED)
     fill_ptr = 0;
   if (temp)
     {
-      segment_info_type *si = seg_info (now_seg);
-      struct insn_label_list *l = si->label_list;
       /* Auto alignment should be switched on by next section change.  */
       auto_align = 1;
-      mips_align (temp, fill_ptr, l != NULL ? l->label : NULL);
+      mips_align (temp, fill_ptr);
     }
   else
     {
@@ -12650,14 +12650,9 @@ mips_enable_auto_align (void)
 static void
 s_cons (int log_size)
 {
-  segment_info_type *si = seg_info (now_seg);
-  struct insn_label_list *l = si->label_list;
-  symbolS *label;
-
-  label = l != NULL ? l->label : NULL;
   mips_emit_delays ();
   if (log_size > 0 && auto_align)
-    mips_align (log_size, 0, label);
+    mips_align (log_size, 0);
   mips_clear_insn_labels ();
   cons (1 << log_size);
 }
@@ -12665,20 +12660,14 @@ s_cons (int log_size)
 static void
 s_float_cons (int type)
 {
-  segment_info_type *si = seg_info (now_seg);
-  struct insn_label_list *l = si->label_list;
-  symbolS *label;
-
-  label = l != NULL ? l->label : NULL;
-
   mips_emit_delays ();
 
   if (auto_align)
     {
       if (type == 'd')
-	mips_align (3, 0, label);
+	mips_align (3, 0);
       else
-	mips_align (2, 0, label);
+	mips_align (2, 0);
     }
 
   mips_clear_insn_labels ();
@@ -13429,9 +13418,6 @@ s_gpvalue (int ignore ATTRIBUTE_UNUSED)
 static void
 s_gpword (int ignore ATTRIBUTE_UNUSED)
 {
-  segment_info_type *si;
-  struct insn_label_list *l;
-  symbolS *label;
   expressionS ex;
   char *p;
 
@@ -13442,12 +13428,9 @@ s_gpword (int ignore ATTRIBUTE_UNUSED)
       return;
     }
 
-  si = seg_info (now_seg);
-  l = si->label_list;
-  label = l != NULL ? l->label : NULL;
   mips_emit_delays ();
   if (auto_align)
-    mips_align (2, 0, label);
+    mips_align (2, 0);
   mips_clear_insn_labels ();
 
   expression (&ex);
@@ -13469,9 +13452,6 @@ s_gpword (int ignore ATTRIBUTE_UNUSED)
 static void
 s_gpdword (int ignore ATTRIBUTE_UNUSED)
 {
-  segment_info_type *si;
-  struct insn_label_list *l;
-  symbolS *label;
   expressionS ex;
   char *p;
 
@@ -13482,12 +13462,9 @@ s_gpdword (int ignore ATTRIBUTE_UNUSED)
       return;
     }
 
-  si = seg_info (now_seg);
-  l = si->label_list;
-  label = l != NULL ? l->label : NULL;
   mips_emit_delays ();
   if (auto_align)
-    mips_align (3, 0, label);
+    mips_align (3, 0);
   mips_clear_insn_labels ();
 
   expression (&ex);