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

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

[Joel Brobecker]

[-- 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 */

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

[Jim Blandy]

What would folks say to simply un-deprecating
deprecated_read_memory_nobpt?  We don't have an acceptable replacement
for it yet.  Joel's read_insn function is the logical thing to do, but
it defeats the purpose of deprecating the function in the first place
--- encouraging people to pass context information explicitly --- to
depend on current_frame.  There isn't any way at present to pull it
off.  So I think the deprecation was premature.

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

[Mark Kettenis]

> Date: Fri, 28 Apr 2006 10:54:47 -0700
> From: "Jim Blandy" <jimb@red-bean.com>
> 
> What would folks say to simply un-deprecating
> deprecated_read_memory_nobpt?  We don't have an acceptable replacement
> for it yet.  Joel's read_insn function is the logical thing to do, but
> it defeats the purpose of deprecating the function in the first place
> --- encouraging people to pass context information explicitly --- to
> depend on current_frame.  There isn't any way at present to pull it
> off.  So I think the deprecation was premature.

I don't completely disagree with you here, but, a different way to
view the problem is putting the blame with the fact that we (ab)use
the prologue analyzer for skipping the prologue when trying to place a
breakpoint at the start of a function, where we really should be able
to use the debug info for doing this.

Currently the code is in a pretty bad shape here; some targets try to
use the debug info, some use the prologue scanner and some do both.

Mark

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

[Joel Brobecker]

> I don't completely disagree with you here, but, a different way to
> view the problem is putting the blame with the fact that we (ab)use
> the prologue analyzer for skipping the prologue when trying to place a
> breakpoint at the start of a function, where we really should be able
> to use the debug info for doing this.

It would be nice to live in a world when prologue information was always
available, but this is not the case, and I think we're very far from it
on certain platforms. So I disagree that we abuse the prologue analyzer.
Sometimes, we don't have the choice.

-- 
Joel

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

[Jim Blandy]

On 4/28/06, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> I don't completely disagree with you here, but, a different way to
> view the problem is putting the blame with the fact that we (ab)use
> the prologue analyzer for skipping the prologue when trying to place a
> breakpoint at the start of a function, where we really should be able
> to use the debug info for doing this.

I certainly agree that debug info is preferable to pig-nosing through
machine code.  Perhaps there should be generic code that does what
find_function_start_sal does, and everybody should be using that
instead of calling SKIP_PROLOGUE directly.

But sometimes we don't have debugging information.  I had thought that
prologue analysis was pretty much dead, given that .debug_frame does a
much better job, and puts the problem in the hands of somebody who can
solve it (the compiler).  But it still seems to come up fairly often.

The other argument is just from first principles: dad gum it, even if
we only know the object file, we should be able to read its
instructions.  If the user has said "break foo" and we're not going to
give an error message or mark it pending on shared library load, then
that means we're alleging we know what "foo" refers to.  So we should
be able to look at its code.

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

[Mark Kettenis]

> Date: Fri, 28 Apr 2006 13:58:39 -0700
> From: "Jim Blandy" <jimb@red-bean.com>
> 
> On 4/28/06, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > I don't completely disagree with you here, but, a different way to
> > view the problem is putting the blame with the fact that we (ab)use
> > the prologue analyzer for skipping the prologue when trying to place a
> > breakpoint at the start of a function, where we really should be able
> > to use the debug info for doing this.
> 
> I certainly agree that debug info is preferable to pig-nosing through
> machine code.  Perhaps there should be generic code that does what
> find_function_start_sal does, and everybody should be using that
> instead of calling SKIP_PROLOGUE directly.
> 
> But sometimes we don't have debugging information.  I had thought that
> prologue analysis was pretty much dead, given that .debug_frame does a
> much better job, and puts the problem in the hands of somebody who can
> solve it (the compiler).  But it still seems to come up fairly often.

But if we don't have debug information, what's the point in trying to
skip the prologue in order to put a breakpoint on ... eh what exactly?
Isn't it better to just punt prologue skipping in that case and place
the breakpoint on the first instruction of the code?

Mark

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

[Daniel Jacobowitz]

On Fri, Apr 28, 2006 at 11:09:19PM +0200, Mark Kettenis wrote:
> But if we don't have debug information, what's the point in trying to
> skip the prologue in order to put a breakpoint on ... eh what exactly?
> Isn't it better to just punt prologue skipping in that case and place
> the breakpoint on the first instruction of the code?

Yes, I agree.

I'd like to note, though, that the way we skip prologues based on debug
information is completely wrong.  It only works with GCC and with other
compilers that have chosen to be bug-compatible with GCC.  DWARF does
support a "this is the end of the prologue" flag, but I don't know if
GCC emits it, and I'm pretty positive GDB doesn't know how to consume
it.

[GCC just emits a line note before and after the prologue - even if
they're on the same line.]

-- 
Daniel Jacobowitz
CodeSourcery

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

[Jim Blandy]

On 4/28/06, Daniel Jacobowitz <drow@false.org> wrote:
> On Fri, Apr 28, 2006 at 11:09:19PM +0200, Mark Kettenis wrote:
> > But if we don't have debug information, what's the point in trying to
> > skip the prologue in order to put a breakpoint on ... eh what exactly?
> > Isn't it better to just punt prologue skipping in that case and place
> > the breakpoint on the first instruction of the code?
>
> Yes, I agree.

So you guys would like to push the deprecation boundary further the
other way, and deprecate SKIP_PROLOGUE itself?  That's an idea.

> I'd like to note, though, that the way we skip prologues based on debug
> information is completely wrong.  It only works with GCC and with other
> compilers that have chosen to be bug-compatible with GCC.  DWARF does
> support a "this is the end of the prologue" flag, but I don't know if
> GCC emits it, and I'm pretty positive GDB doesn't know how to consume
> it.

I think the argument was that, in the long run, GCC should be emitting
location lists so that the debugging information accurately describes
the location of the arguments even within the prologues.  Setting a
breakpoint on a function would actually set the breakpoint ... at the
function's entry point.  We could delete SKIP_PROLOGUE altogether. 
Then, there'd be no point in producing or consuming an "end of
prologue" marker whose meaning is kind of vague anyway.

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

[Joel Brobecker]

> So you guys would like to push the deprecation boundary further the
> other way, and deprecate SKIP_PROLOGUE itself?  That's an idea.

I agree with idea, but in my humble opinion, it's premature. We're
still stuck with many platforms that are still in the dark ages and
don't provide the necessary frame info in the debugging info for us
to be able to correctly locate function arguments, unwind the call
stack, etc.

> I think the argument was that, in the long run, GCC should be emitting
> location lists so that the debugging information accurately describes
> the location of the arguments even within the prologues.  Setting a
> breakpoint on a function would actually set the breakpoint ... at the
> function's entry point.  We could delete SKIP_PROLOGUE altogether. 
> Then, there'd be no point in producing or consuming an "end of
> prologue" marker whose meaning is kind of vague anyway.

I agree, especially at higher levels of optimization, the notion
of prologue becomes fuzzier and fuzzier. But not all architectures
offer this level of information, and sometimes the system libraries
offer nothing at all but the symbol table.

-- 
Joel

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

[Joel Brobecker]

Hello,

I am slightly concerned that the side-discussions about the necessity
of frame debugging info, or prologue skipping when inserting a breakpoint
ended up burying the patch I just submitted.

The case presented here is different from the topics later discussed
in this thread, so I'm not sure I can see how any modification made
from these discussions is going to solve the case at hand.

Soooooo... Is somebody going to review the patch? No deadline expected,
I just want to make sure that it's not been rejected without me knowing
it. If it has been rejected, then I need to understand the reasons so
that I can work on a better version.

Thanks,
-- 
Joel

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

[Jim Blandy]

On 4/28/06, Joel Brobecker <brobecker@adacore.com> wrote:
> Soooooo... Is somebody going to review the patch? No deadline expected,
> I just want to make sure that it's not been rejected without me knowing
> it. If it has been rejected, then I need to understand the reasons so
> that I can work on a better version.

It's Mark's call, but as far as I'm concerned, your patch is a
reasonable way to deal with the situation.

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

[Eli Zaretskii]

> Date: Fri, 28 Apr 2006 10:11:54 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> 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.

Isn't this a general problem, not limited to x86?  That is, if there's
a breakpoint inserted in the prologue, wouldn't other prologue
analyzers be fooled?

If this is indeed a general problem, I suggest we fix it in a general
fashion, not for some specific platform.

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

[Daniel Jacobowitz]

On Sat, Apr 29, 2006 at 05:20:42PM +0300, Eli Zaretskii wrote:
> Isn't this a general problem, not limited to x86?  That is, if there's
> a breakpoint inserted in the prologue, wouldn't other prologue
> analyzers be fooled?
> 
> If this is indeed a general problem, I suggest we fix it in a general
> fashion, not for some specific platform.

That's why many prologue analyzers already use the memory reading
functions which ignore breakpoints; this just improves our consistency.

Now, you could make a reasonable argument that the default when reading
memory ought to be to ignore breakpoints.  But that would be a much
harder change to test; we'd need to catch all the places which do need
to see breakpoints - I can only think of one off the top of my head,
but I'm sure there are more.  So, I would suggest making it separately.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Eli Zaretskii]

> Date: Sat, 29 Apr 2006 10:28:12 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> Now, you could make a reasonable argument that the default when reading
> memory ought to be to ignore breakpoints.  But that would be a much
> harder change to test; we'd need to catch all the places which do need
> to see breakpoints

We don't need to catch all of them at once, if that's hard.  We can
catch them one by one and fix them as we do.

> I can only think of one off the top of my head, but I'm sure there
> are more.  So, I would suggest making it separately.

It's fine with me, but I still don't understand why it has to be in an
architecture-specific file, not in some more general place.  Am I
missing something?

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

[Daniel Jacobowitz]

On Sat, Apr 29, 2006 at 05:51:28PM +0300, Eli Zaretskii wrote:
> > Now, you could make a reasonable argument that the default when reading
> > memory ought to be to ignore breakpoints.  But that would be a much
> > harder change to test; we'd need to catch all the places which do need
> > to see breakpoints
> 
> We don't need to catch all of them at once, if that's hard.  We can
> catch them one by one and fix them as we do.

Well, this one's been there for years.  I'd rather have slightly more
confidence that we won't introduce a whole new set of such bugs...

> > I can only think of one off the top of my head, but I'm sure there
> > are more.  So, I would suggest making it separately.
> 
> It's fine with me, but I still don't understand why it has to be in an
> architecture-specific file, not in some more general place.  Am I
> missing something?

Right now, there are many ways to read memory; two of them skip
inserted breakpoints, and the others don't.  So, to fix this sort of
problem, you have to deliberately use the functions which do skip
breakpoints.  Thus the change has to be at the site of the memory
read, which is in this case in a target-specific file.  We don't want
to remove breakpoints before invoking the prologue analyzer; the
removal may be unnecessary if a "step" command is in progress and not
done yet.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Eli Zaretskii]

> Date: Sat, 29 Apr 2006 11:05:57 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> Right now, there are many ways to read memory; two of them skip
> inserted breakpoints, and the others don't.

What will break if we make them all skip breakpoints?

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

[Daniel Jacobowitz]

On Sat, Apr 29, 2006 at 07:47:45PM +0300, Eli Zaretskii wrote:
> > Date: Sat, 29 Apr 2006 11:05:57 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > Right now, there are many ways to read memory; two of them skip
> > inserted breakpoints, and the others don't.
> 
> What will break if we make them all skip breakpoints?

I don't know.  The only one I know of offhand is ppc-linux-tdep.c's
shared library support, which provides an alternate memory remove
breakpoint method.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Mark Kettenis]

> Date: Fri, 28 Apr 2006 10:11:54 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> 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.

Hmm, I'm not entirely happy with the name, and I agree with jimb that
using current_frame here doesn't make sense.  I was planning to look
into getting rid of the prologue skipper, but that isn't really going
to solve the problems.

After another day of thinking I came to the conclusion that jimb is
probably right about the premature deprecation of memory_read_nobpt.
If we undeprecate it, you wouldn't really need your new read_insn
function, since it really would be the same as memory_read_nobpt.

What do the other (global) maintainers think?

Mark

> 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.

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

[Joel Brobecker]

Thanks for the really quick review, Mark.

> After another day of thinking I came to the conclusion that jimb is
> probably right about the premature deprecation of memory_read_nobpt.
> If we undeprecate it, you wouldn't really need your new read_insn
> function, since it really would be the same as memory_read_nobpt.

FWIW, I like this approach too. I can submit a new set of patches
as soon as this becomes the consensus.

-- 
Joel

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

[Daniel Jacobowitz]

On Mon, May 01, 2006 at 06:36:24PM +0200, Mark Kettenis wrote:
> Hmm, I'm not entirely happy with the name, and I agree with jimb that
> using current_frame here doesn't make sense.  I was planning to look
> into getting rid of the prologue skipper, but that isn't really going
> to solve the problems.
> 
> After another day of thinking I came to the conclusion that jimb is
> probably right about the premature deprecation of memory_read_nobpt.
> If we undeprecate it, you wouldn't really need your new read_insn
> function, since it really would be the same as memory_read_nobpt.
> 
> What do the other (global) maintainers think?

I see two choices.

1.  Undeprecate deprecated_read_memory_nobpt.  Use it.

2.  Make all memory reads skip breakpoints by default, and provide a
    different function which doesn't.

I think (1) is the best choice for now.  (2) is going to require a lot
of poking around dark corners and testing on strange targets.  I'd
recommend anyone interested start by cutting down on the number of
memory reading functions - there's quite a lot of them!

So, I'm in favor.

-- 
Daniel Jacobowitz
CodeSourcery