[RFA/commit] ia64: incorrect breakpoint save/restore with L-type instruction at slot 1

[Joel Brobecker <brobecker at adacore dot com>]

Hello,

We have a small testcase at AdaCore that just puts a breakpoint
on a line of code, runs to it, and then continues past it. I am
pasting the code at the end of this email.  The initial purpose
of that testcase was to stress GDB with a very large number of
Ada tasks, but this is not relevant to the issue at hand, so I reduced
the number of tasks to 2.

To reproduce the problem, compile the program pasted at the end of
this message, and build it with the following trivial gnatmake:

    % gnatmake -g ct

Then, insert a breakpoint at line 51, run to this breakpoint, and
then try to continue from that breakpoint:

    (gdb) b 51
    Breakpoint 1 at 0x4000000000004bd1: file ct.adb, line 51.
    (gdb) run
    [...]
    Breakpoint 1, ct () at ct.adb:51
    51         delay 1.0;  -- BREAK
    (gdb) cont
    Continuing. 

    Program received signal SIGILL, Illegal instruction.
    ct () at ct.adb:51
    51         delay 1.0;  -- BREAK

The program should just run until it completes.

The problem comes from the fact that the instruction where we
insert the breakpoint is in a slot 1 and is an L type instruction.
However, we save the wrong portion of the instruction bundle before
we realize this fact, and then insert the breakpoint in slot 2:
First we store the piece of bundle assuming slot 1, then reread
the bundle this time with breakpoints inserted, realize that we
have an L-type instruction at slot 1 and thus decide to put the
breakpoint on slot 2.  At this point, the wrong portion of the bundle
has been saved in the shadow contents buffer, and breakpoint removal
is broken.

2009-09-25  Joel Brobecker  <brobecker@adacore.com>

        * ia64-tdep.c (ia64_memory_insert_breakpoint): Check the slotnum
        and the type of instruction before deciding which slot to save
        in the breakpoint shadown contents.

Tested on ia64-linux.  Jan, does this look right to you too?  I would
normally apply immediately, but since we're close to release, a second
pair of eyes is always nice.

Thanks,
-- 
Joel

with Ada.Text_IO;

procedure Ct is

   Number_Of_Tasks_To_Create : constant := 2;

   task type T (Index : Natural; Last : Boolean) is
      entry Finish;
   end T;

   task body T is
   begin

      --  Signal the creation of the task.
      if Last then
         Ada.Text_IO.Put_Line
           ("Starting task number:" & Natural'Image (Index) & " (last)");
      else
         Ada.Text_IO.Put_Line
           ("Starting task number:" & Natural'Image (Index));
      end if;

      --  Wait until told to finish the task. Then print a message
      --  showing that the task is completing.
      accept Finish do
         if Last then
            Ada.Text_IO.Put_Line
              ("End of task number:" & Natural'Image (Index) & " (last)");
         else
            Ada.Text_IO.Put_Line
              ("End of task number:" & Natural'Image (Index));
         end if;
      end Finish;

   end T;

   type TA is access T;

   All_Tasks : array (Positive range 1 .. Number_Of_Tasks_To_Create) of TA;

begin

   for Loop_Index in All_Tasks'Range loop
      All_Tasks (Loop_Index) :=
        new T (Loop_Index, Loop_Index = All_Tasks'Last);
   end loop;

   -- Add a delay to make sure all "Starting task ..." messages are
   -- printed before Finish'ing thee tasks.
   delay 1.0;
   delay 1.0;  -- BREAK

   for Loop_Index in All_Tasks'Range loop
      All_Tasks (Loop_Index).Finish;
   end loop;

end Ct;

commit 1491fbdd98bd0bd63717f03f64c24bdda3917029
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Fri Sep 25 14:51:45 2009 -0400

    Check instrunction slot num and type before deciding which slot to save.

diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
index 674204a..bbde5f6 100644
--- a/gdb/ia64-tdep.c
+++ b/gdb/ia64-tdep.c
@@ -635,6 +635,12 @@ ia64_memory_insert_breakpoint (struct gdbarch *gdbarch,
       return val;
     }
 
+  /* Check for L type instruction in slot 1, if present then bump up the slot
+     number to the slot 2.  */
+  template = extract_bit_field (bundle, 0, 5);
+  if (slotnum == 1 && template_encoding_table[template][slotnum] == L)
+    slotnum = 2;
+
   /* Slot number 2 may skip at most 2 bytes at the beginning.  */
   bp_tgt->shadow_len = BUNDLE_LEN - 2;
 
@@ -657,12 +663,6 @@ ia64_memory_insert_breakpoint (struct gdbarch *gdbarch,
       return val;
     }
 
-  /* Check for L type instruction in slot 1, if present then bump up the slot
-     number to the slot 2.  */
-  template = extract_bit_field (bundle, 0, 5);
-  if (slotnum == 1 && template_encoding_table[template][slotnum] == L)
-    slotnum = 2;
-
   /* Breakpoints already present in the code will get deteacted and not get
      reinserted by bp_loc_is_permanent.  Multiple breakpoints at the same
      location cannot induce the internal error as they are optimized into