[RFC/RFA/i386] pb reading insns if breakpoints still inserted

[Joel Brobecker <brobecker@adacore.com>]

[-- Attachment #1: Type: text/plain, Size: 1420 bytes --]

Hello,

following a discussion that started in:

        http://sources.redhat.com/ml/gdb-patches/2006-04/msg00345.html

Here is a patch that fixes the problem. In summary: When we do next/step
operations, we end up parsing the top frame function prologue and
creating a frame_info with it. Unfortunately, at that point, the
breakpoints are still inserted and that causes the prologue analyzer
to misinterpret the function prologue and consequently breaks unwinding
a bit.

The fix that has been suggested is to fix the i386 prologue analyzer
to handle inserted breakpoints. This is what the new read_insn()
function does. I've always been very bad with names, so suggestions
are more than welcome.

I also think that read_insn() might be better located in a more general
area, such as beside the various read_memory routines for instance. But
before moving this function, I thought I'd ask...

2006-04-28  Joel Brobecker  <brobecker@adacore.com>

        * i386-tdep.c (read_insn): New function.
        (i386_follow_jump): Use read_insn to read instructions.
        (i386_analyze_struct_return): Likewise.
        (i386_skip_probe): Likewise.
        (i386_match_insn): Likewise.
        (i386_analyze_frame_setup): Likewise.
        (i386_analyze_register_saves): Likewise.
        (i386_skip_prologue): Likewise.

Tested on i686-pc-cygwin. No regression. Testcase to follow shortly.
OK to apply? 

Thanks,
-- 
Joel

[-- Attachment #2: bt.diff --]
[-- Type: text/plain, Size: 4728 bytes --]

Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.224
diff -u -p -r1.224 i386-tdep.c
--- i386-tdep.c	17 Mar 2006 00:14:24 -0000	1.224
+++ i386-tdep.c	28 Apr 2006 17:07:00 -0000
@@ -80,6 +80,34 @@ static char *i386_mmx_names[] =
 
 static const int i386_num_mmx_regs = ARRAY_SIZE (i386_mmx_names);
 
+/* Read LEN bytes of inferior memory at PC, and store them in BUF.
+   This function works even if breakpoints are still inserted in
+   memory.
+
+   Note that the breakpoint instruction on i386 takes only one byte,
+   so being careful of inserted breakpoints is only necessary if the
+   first byte of the instruction is being read. This function introduces
+   a small overhead that can be avoided by doing a direct memory read if
+   the bytes being read do not contain the first byte of an instruction.  */
+   
+static void
+read_insn (CORE_ADDR pc, gdb_byte *buf, int len)
+{
+  struct frame_info *current_frame = NULL;
+
+  if (target_has_registers && target_has_stack && target_has_memory)
+    current_frame = get_current_frame ();
+
+  if (current_frame != NULL)
+    safe_frame_unwind_memory (current_frame, pc, buf, len);
+  else
+    {
+      /* If we don't have a frame, then breakpoints cannot have been
+         inserted, and hence a direct memory read will work.  */
+      read_memory (pc, buf, len);
+    }
+}
+
 static int
 i386_mmx_regnum_p (struct gdbarch *gdbarch, int regnum)
 {
@@ -354,7 +382,7 @@ i386_follow_jump (CORE_ADDR pc)
   long delta = 0;
   int data16 = 0;
 
-  op = read_memory_unsigned_integer (pc, 1);
+  read_insn (pc, &op, 1);
   if (op == 0x66)
     {
       data16 = 1;
@@ -420,12 +448,12 @@ i386_analyze_struct_return (CORE_ADDR pc
   if (current_pc <= pc)
     return pc;
 
-  op = read_memory_unsigned_integer (pc, 1);
+  read_insn (pc, &op, 1);
 
   if (op != 0x58)		/* popl %eax */
     return pc;
 
-  read_memory (pc + 1, buf, 4);
+  read_insn (pc + 1, buf, 4);
   if (memcmp (buf, proto1, 3) != 0 && memcmp (buf, proto2, 4) != 0)
     return pc;
 
@@ -464,7 +492,7 @@ i386_skip_probe (CORE_ADDR pc)
   gdb_byte buf[8];
   gdb_byte op;
 
-  op = read_memory_unsigned_integer (pc, 1);
+  read_insn (pc, &op, 1);
 
   if (op == 0x68 || op == 0x6a)
     {
@@ -535,7 +563,7 @@ i386_match_insn (CORE_ADDR pc, struct i3
   struct i386_insn *insn;
   gdb_byte op;
 
-  op = read_memory_unsigned_integer (pc, 1);
+  read_insn (pc, &op, 1);
 
   for (insn = skip_insns; insn->len > 0; insn++)
     {
@@ -548,7 +576,7 @@ i386_match_insn (CORE_ADDR pc, struct i3
 	  gdb_assert (insn->len > 1);
 	  gdb_assert (insn->len <= I386_MAX_INSN_LEN);
 
-	  read_memory (pc + 1, buf, insn->len - 1);
+	  read_insn (pc + 1, buf, insn->len - 1);
 	  for (i = 1; i < insn->len; i++)
 	    {
 	      if ((buf[i - 1] & insn->mask[i]) != insn->insn[i])
@@ -634,7 +662,7 @@ i386_analyze_frame_setup (CORE_ADDR pc, 
   if (limit <= pc)
     return limit;
 
-  op = read_memory_unsigned_integer (pc, 1);
+  read_insn (pc, &op, 1);
 
   if (op == 0x55)		/* pushl %ebp */
     {
@@ -669,7 +697,7 @@ i386_analyze_frame_setup (CORE_ADDR pc, 
       if (limit <= pc + skip)
 	return limit;
 
-      op = read_memory_unsigned_integer (pc + skip, 1);
+      read_insn (pc + skip, &op, 1);
 
       /* Check for `movl %esp, %ebp' -- can be written in two ways.  */
       switch (op)
@@ -703,7 +731,7 @@ i386_analyze_frame_setup (CORE_ADDR pc, 
 
 	 NOTE: You can't subtract a 16-bit immediate from a 32-bit
 	 reg, so we don't have to worry about a data16 prefix.  */
-      op = read_memory_unsigned_integer (pc, 1);
+      read_insn (pc, &op, 1);
       if (op == 0x83)
 	{
 	  /* `subl' with 8-bit immediate.  */
@@ -759,7 +787,7 @@ i386_analyze_register_saves (CORE_ADDR p
     offset -= cache->locals;
   for (i = 0; i < 8 && pc < current_pc; i++)
     {
-      op = read_memory_unsigned_integer (pc, 1);
+      read_insn (pc, &op, 1);
       if (op < 0x50 || op > 0x57)
 	break;
 
@@ -848,7 +876,7 @@ i386_skip_prologue (CORE_ADDR start_pc)
 
   for (i = 0; i < 6; i++)
     {
-      op = read_memory_unsigned_integer (pc + i, 1);
+      read_insn (pc + i, &op, 1);
       if (pic_pat[i] != op)
 	break;
     }
@@ -856,7 +884,7 @@ i386_skip_prologue (CORE_ADDR start_pc)
     {
       int delta = 6;
 
-      op = read_memory_unsigned_integer (pc + delta, 1);
+      read_insn (pc + delta, &op, 1);
 
       if (op == 0x89)		/* movl %ebx, x(%ebp) */
 	{
@@ -869,7 +897,7 @@ i386_skip_prologue (CORE_ADDR start_pc)
 	  else			/* Unexpected instruction.  */
 	    delta = 0;
 
-	  op = read_memory_unsigned_integer (pc + delta, 1);
+          read_insn (pc + delta, &op, 1);
 	}
 
       /* addl y,%ebx */