Re: PR gas/13024: internal error with branch swapping and double .locs

[Richard Sandiford <rsandifo at linux dot vnet dot ibm dot com>]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:
> Richard Sandiford wrote:
>> This patch instead creates a list of pending line entries.  When
>> emitting the current .loc for a given label, we do the same for all
>> previous pending .locs.  When we consume the current .loc, we emit all
>> earlier pending ones at the end of the current frag.  This preserves the
>> current behaviour of the external interface, as used by ia64 and xtensa
>> among others.  (I wonder whether they'd actually want to take a list of
>> pending .locs, but I'll leave that for the port maintainers to decide.)
>
> This seems to have introduced a regression in handling DWARF3
> attributes for double .loc directives.  With code like:
>
>         .file   1 "test.cpp"
>
>         .text
>         .loc    1 1 0 prologue_end
>         .loc    1 2 0
>         nop
>
> we now get a line table:
>
>  Line Number Statements:
>   Set prologue_end to true
>   Extended opcode 2: set Address to 0x0
>   Copy
>   Set prologue_end to true
>   Special opcode 6: advance Address by 0 to 0x0 and Line by 1 to 2
>   Advance PC by 4 to 0x4
>   Extended opcode 1: End of Sequence
>
> Note that *both* line table entries are marked as prologue_end,
> even though only the first .loc directive has the keyword.
>
> Before your patch, we got instead:
>
>  Line Number Statements:
>   Set prologue_end to true
>   Extended opcode 2: set Address to 0x0
>   Copy
>   Special opcode 6: advance Address by 0 to 0x0 and Line by 1 to 2
>   Advance PC by 4 to 0x4
>   Extended opcode 1: End of Sequence
>
>
> This seems to be caused by this part of the patch:
>
>> @@ -576,7 +609,7 @@ dwarf2_directive_loc (int dummy ATTRIBUT
>>    /* If we see two .loc directives in a row, force the first one to be
>>       output now.  */
>>    if (dwarf2_loc_directive_seen)
>> -    dwarf2_emit_insn (0);
>> +    dwarf2_push_line (&current);
>
> The old call to dwarf2_emit_insn used to call down to
> dwarf2_consume_line_info which did:
>
>   current.flags &= ~(DWARF2_FLAG_BASIC_BLOCK
>                      | DWARF2_FLAG_PROLOGUE_END
>                      | DWARF2_FLAG_EPILOGUE_BEGIN);
>
> The new call to dwarf2_push_line doesn't do that any more,
> so the DWARF2_FLAG_PROLOGUE_END stays active in current.flags
> for the second directive.  Was this effect deliberate?

No, it's a bug.  The line you quoted was the crux of the patch though.

Alan also pointed out some potential problems with this approach in:

  http://sourceware.org/ml/binutils/2012-05/msg00364.html

Here's an alternative way of fixing the original bug that shouldn't
have these problems.  Does it look OK?  I'll add a test for the
regression too.

Thanks,
Richard


gcc/
	Revert:

	2011-09-05  Richard Sandiford  <rdsandiford@googlemail.com>

	PR gas/13024
	* dwarf2dbg.c (pending_lines, pending_lines_tail): New variables.
	(dwarf2_gen_line_info_1): Delete.
	(dwarf2_push_line, dwarf2_flush_pending_lines): New functions.
	(dwarf2_gen_line_info, dwarf2_emit_label): Use them.
	(dwarf2_consume_line_info): Call dwarf2_flush_pending_lines.
	(dwarf2_directive_loc): Push previous .locs instead of generating
	them immediately.

Index: gas/dwarf2dbg.c
===================================================================
--- gas/dwarf2dbg.c	2013-06-10 16:31:04.000000000 +0100
+++ gas/dwarf2dbg.c	2013-06-10 16:33:06.787932472 +0100
@@ -216,10 +216,6 @@ static struct dwarf2_line_info current =
   0
 };
 
-/* Lines that are at the same location as CURRENT, and which are waiting
-   for a label.  */
-static struct line_entry *pending_lines, **pending_lines_tail = &pending_lines;
-
 /* The size of an address on the target.  */
 static unsigned int sizeof_address;
 
@@ -293,47 +289,22 @@ get_line_subseg (segT seg, subsegT subse
   return lss;
 }
 
-/* Push LOC onto the pending lines list.  */
+/* Record an entry for LOC occurring at LABEL.  */
 
 static void
-dwarf2_push_line (struct dwarf2_line_info *loc)
+dwarf2_gen_line_info_1 (symbolS *label, struct dwarf2_line_info *loc)
 {
+  struct line_subseg *lss;
   struct line_entry *e;
 
   e = (struct line_entry *) xmalloc (sizeof (*e));
   e->next = NULL;
-  e->label = NULL;
+  e->label = label;
   e->loc = *loc;
 
-  *pending_lines_tail = e;
-  pending_lines_tail = &(*pending_lines_tail)->next;
-}
-
-/* Emit all pending line information.  LABEL is the label with which the
-   lines should be associated, or null if they should be associated with
-   the current position.  */
-
-static void
-dwarf2_flush_pending_lines (symbolS *label)
-{
-  if (pending_lines)
-    {
-      struct line_subseg *lss;
-      struct line_entry *e;
-
-      if (!label)
-	label = symbol_temp_new_now ();
-
-      for (e = pending_lines; e; e = e->next)
-	e->label = label;
-
-      lss = get_line_subseg (now_seg, now_subseg);
-      *lss->ptail = pending_lines;
-      lss->ptail = pending_lines_tail;
-
-      pending_lines = NULL;
-      pending_lines_tail = &pending_lines;
-    }
+  lss = get_line_subseg (now_seg, now_subseg);
+  *lss->ptail = e;
+  lss->ptail = &e->next;
 }
 
 /* Record an entry for LOC occurring at OFS within the current fragment.  */
@@ -344,6 +315,8 @@ dwarf2_gen_line_info (addressT ofs, stru
   static unsigned int line = -1;
   static unsigned int filenum = -1;
 
+  symbolS *sym;
+
   /* Early out for as-yet incomplete location information.  */
   if (loc->filenum == 0 || loc->line == 0)
     return;
@@ -359,7 +332,6 @@ dwarf2_gen_line_info (addressT ofs, stru
   line = loc->line;
   filenum = loc->filenum;
 
-  dwarf2_push_line (loc);
   if (linkrelax)
     {
       char name[120];
@@ -367,10 +339,11 @@ dwarf2_gen_line_info (addressT ofs, stru
       /* Use a non-fake name for the line number location,
 	 so that it can be referred to by relocations.  */
       sprintf (name, ".Loc.%u.%u", line, filenum);
-      dwarf2_flush_pending_lines (symbol_new (name, now_seg, ofs, frag_now));
+      sym = symbol_new (name, now_seg, ofs, frag_now);
     }
   else
-    dwarf2_flush_pending_lines (symbol_temp_new (now_seg, ofs, frag_now));
+    sym = symbol_temp_new (now_seg, ofs, frag_now);
+  dwarf2_gen_line_info_1 (sym, loc);
 }
 
 /* Returns the current source information.  If .file directives have
@@ -431,11 +404,6 @@ dwarf2_emit_insn (int size)
 void
 dwarf2_consume_line_info (void)
 {
-  /* If the consumer has stashed the current location away for later use,
-     assume that any earlier location information should be associated
-     with ".".  */
-  dwarf2_flush_pending_lines (NULL);
-
   /* Unless we generate DWARF2 debugging information for each
      assembler line, we only emit one line symbol for one LOC.  */
   dwarf2_loc_directive_seen = FALSE;
@@ -467,8 +435,7 @@ dwarf2_emit_label (symbolS *label)
 
   loc.flags |= DWARF2_FLAG_BASIC_BLOCK;
 
-  dwarf2_push_line (&loc);
-  dwarf2_flush_pending_lines (label);
+  dwarf2_gen_line_info_1 (label, &loc);
   dwarf2_consume_line_info ();
 }
 
@@ -628,7 +595,7 @@ dwarf2_directive_loc (int dummy ATTRIBUT
   /* If we see two .loc directives in a row, force the first one to be
      output now.  */
   if (dwarf2_loc_directive_seen)
-    dwarf2_push_line (&current);
+    dwarf2_emit_insn (0);
 
   filenum = get_absolute_expression ();
   SKIP_WHITESPACE ();

gas/
	* dwarf2dbg.h (dwarf2_move_insn): Declare.
	* dwarf2dbg.c (line_subseg): Add pmove_tail.
	(get_line_subseg): Add create_p argument.  Initialize pmove_tail.
	(dwarf2_gen_line_info_1): Update call accordingly.
	(dwarf2_move_insn): New function.
	* config/tc-mips.c (append_insn): Use dwarf2_move_insn.

Index: gas/dwarf2dbg.h
===================================================================
--- gas/dwarf2dbg.h	2013-06-10 16:31:04.501559312 +0100
+++ gas/dwarf2dbg.h	2013-06-10 16:33:44.932736663 +0100
@@ -74,6 +74,8 @@ extern void dwarf2_gen_line_info (addres
 /* Must be called for each generated instruction.  */
 extern void dwarf2_emit_insn (int);
 
+void dwarf2_move_insn (int);
+
 /* Reset the state of the line number information to reflect that
    it has been used.  */
 extern void dwarf2_consume_line_info (void);
Index: gas/dwarf2dbg.c
===================================================================
--- gas/dwarf2dbg.c	2013-06-10 16:33:06.787932472 +0100
+++ gas/dwarf2dbg.c	2013-06-10 16:37:18.047640482 +0100
@@ -169,6 +169,7 @@ struct line_subseg {
   subsegT subseg;
   struct line_entry *head;
   struct line_entry **ptail;
+  struct line_entry **pmove_tail;
 };
 
 struct line_seg {
@@ -238,10 +239,10 @@ generic_dwarf2_emit_offset (symbolS *sym
 }
 #endif
 
-/* Find or create an entry for SEG+SUBSEG in ALL_SEGS.  */
+/* Find or create (if CREATE_P) an entry for SEG+SUBSEG in ALL_SEGS.  */
 
 static struct line_subseg *
-get_line_subseg (segT seg, subsegT subseg)
+get_line_subseg (segT seg, subsegT subseg, bfd_boolean create_p)
 {
   static segT last_seg;
   static subsegT last_subseg;
@@ -256,6 +257,9 @@ get_line_subseg (segT seg, subsegT subse
   s = (struct line_seg *) hash_find (all_segs_hash, seg->name);
   if (s == NULL)
     {
+      if (!create_p)
+	return NULL;
+
       s = (struct line_seg *) xmalloc (sizeof (*s));
       s->next = NULL;
       s->seg = seg;
@@ -279,6 +283,7 @@ get_line_subseg (segT seg, subsegT subse
   lss->subseg = subseg;
   lss->head = NULL;
   lss->ptail = &lss->head;
+  lss->pmove_tail = &lss->head;
   *pss = lss;
 
  found_subseg:
@@ -302,7 +307,7 @@ dwarf2_gen_line_info_1 (symbolS *label,
   e->label = label;
   e->loc = *loc;
 
-  lss = get_line_subseg (now_seg, now_subseg);
+  lss = get_line_subseg (now_seg, now_subseg, TRUE);
   *lss->ptail = e;
   lss->ptail = &e->next;
 }
@@ -396,6 +401,33 @@ dwarf2_emit_insn (int size)
   dwarf2_consume_line_info ();
 }
 
+/* Move all previously-emitted line entries for the current position by
+   DELTA bytes.  This function cannot be used to move the same entries
+   twice.  */
+
+void
+dwarf2_move_insn (int delta)
+{
+  struct line_subseg *lss;
+  struct line_entry *e;
+  valueT now;
+
+  if (delta == 0)
+    return;
+
+  lss = get_line_subseg (now_seg, now_subseg, FALSE);
+  if (!lss)
+    return;
+
+  now = frag_now_fix ();
+  while ((e = *lss->pmove_tail))
+    {
+      if (S_GET_VALUE (e->label) == now)
+	S_SET_VALUE (e->label, now + delta);
+      lss->pmove_tail = &e->next;
+    }
+}
+
 /* Called after the current line information has been either used with
    dwarf2_gen_line_info or saved with a machine instruction for later use.
    This resets the state of the line number information to reflect that
Index: gas/config/tc-mips.c
===================================================================
--- gas/config/tc-mips.c	2013-06-10 16:31:04.501559312 +0100
+++ gas/config/tc-mips.c	2013-06-10 16:55:43.148936439 +0100
@@ -4370,22 +4370,18 @@ append_insn (struct mips_cl_insn *ip, ex
   branch_disp = method == APPEND_SWAP ? insn_length (history) : 0;
 
 #ifdef OBJ_ELF
-  /* The value passed to dwarf2_emit_insn is the distance between
-     the beginning of the current instruction and the address that
-     should be recorded in the debug tables.  This is normally the
-     current address.
+  dwarf2_emit_insn (0);
+  /* We want MIPS16 and microMIPS debug info to use ISA-encoded addresses,
+     so "move" the instruction accordingly.
 
-     For MIPS16/microMIPS debug info we want to use ISA-encoded
-     addresses, so we use -1 for an address higher by one than the
-     current one.
-
-     If the instruction produced is a branch that we will swap with
-     the preceding instruction, then we add the displacement by which
-     the branch will be moved backwards.  This is more appropriate
-     and for MIPS16/microMIPS code also prevents a debugger from
-     placing a breakpoint in the middle of the branch (and corrupting
-     code if software breakpoints are used).  */
-  dwarf2_emit_insn ((HAVE_CODE_COMPRESSION ? -1 : 0) + branch_disp);
+     Also, it doesn't seem appropriate for the assembler to reorder
+     .loc entries.  If this instruction is a branch that we are going to
+     swap with the previous instruction, the two instructions should be
+     treated as a unit, and the debug information for both instructions
+     should refer to the start of the branch sequence.  This also stops
+     a debugger from corrupting the code by trying to insert a 32-bit
+     breakpoint instruction into a 16-bit MIPS16 or microMIPS delay slot.  */
+  dwarf2_move_insn ((HAVE_CODE_COMPRESSION ? 1 : 0) - branch_disp);
 #endif
 
   relax32 = (mips_relax_branch