[RFA] Fix frame-issue with watchpoints...

[RFA] Fix frame-issue with watchpoints...

[Joel Brobecker]

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

Hello,

A coworker of mine noticed an issue with watchpoints. To reproduce,
he provided the following Ada program (on x86-linux for instance):

     1  procedure Watch is
     2
     3     procedure Foo (X : access Integer) is
     4     begin
     5        delay 1.0;
     6     end Foo;
     7
     8     X : aliased Integer := 1;
     9
    10  begin
    11     Foo (X'Access);
    12     X := 2;  -- BREAK
    13  end Watch;

This program needs to be compiled using the usual trivial command:

        % gnatmake -g watch

This will produce a small program that can be debugged as follow:

        % gdb watch
        (gdb) b foo.adb:5
        (gdb) b foo.adb:12
        (gdb) run

At this point, the debugger will stop inside procedure Foo, where
we insert a watchpoint on X, before continuing to the next breakpoint
on line 12:

        (gdb) watch x
        Hardware watchpoint 3: x
        (gdb) cont
        Continuing.
        No frame is currently executing in block watch.foo.

Here is what happens:

The first thing we do is notice that we just stopped at a location
where we have a breakpoint, so we do a single-step, which worked out
just fine, and then do the real continue. Just before resuming the
execution of the inferior, we try to re-insert the breakpoints, and
that means we re-evaluate all breakpoint locations, and this is where
things start to break...

The watchpoint location evaluation eventually leads us to
value_of_variable, and in particular the error message we see:

        else if (symbol_read_needs_frame (var))
          {
            frame = block_innermost_frame (b);
            if (!frame)
              {   
                if (BLOCK_FUNCTION (b)
                    && SYMBOL_PRINT_NAME (BLOCK_FUNCTION (b)))
                  error (_("No frame is currently executing in block %s."),

What happens is that block_innermost_frame(b) returned NULL, and
the reason is fairly simple:

        frame = NULL;
        while (1)
          {
            frame = get_prev_frame (frame);

There is a note from Andrew besides the part of get_prev_frame that
says:

  if (this_frame == NULL)
    {
      /* NOTE: cagney/2002-11-09: There was a code segment here that
         would error out when CURRENT_FRAME was NULL.  The comment
         that went with it made the claim ...

         ``This screws value_of_variable, which just wants a nice
         clean NULL return from block_innermost_frame if there are no
         frames.  I don't think I've ever seen this message happen
         otherwise.  And returning NULL here is a perfectly legitimate
         thing to do.''

         Per the above, this code shouldn't even be called with a NULL
         THIS_FRAME.  */
      frame_debug_got_null_frame (gdb_stdlog, this_frame, "this_frame NULL");
      return current_frame;
    }

The issue in our case is that "current_frame" is NULL too, probably
because we never needed it before in our case (just finished off
single-stepping out of the breakpoint and immediately getting ready
to resume) and therefore never set it to a proper value.

One way to bandaid this, probably along the lines that Andrew was
trying to do (try to recover from a situation that should not happen),
is to replace "current_frame" by get_current_frame().
However, I'm thinking that the real culprit is block_innermost_frame
which called this function with a NULL frame, which it's not supposed
to do. So I rewrote the loop to start from the outermost frame instead
of a NULL frame, and then work its way up the frame stack...

This fixes the problem without introducing any regression.

In addition to this patch, I think we should investigate the option
of removing the check above and replace it with an assertion that
THIS_FRAME is not null. Or maybe if we are afraid of breaking something,
then at least change "current_frame" along the lines of what I suggested,
and print an unconditional warning. Let's make it visible.

2006-10-05  Joel Brobecker  <brobecker@adacore.com>

        * blockframe.c (block_innermost_frame): Rewrite frame search logic.

Tested on x86-linux, no regression. A new testcase to be submitted soon.
OK to apply?

Thanks,
-- 
Joel

[-- Attachment #2: blockframe.c.diff --]
[-- Type: text/plain, Size: 774 bytes --]

Index: blockframe.c
===================================================================
RCS file: /cvs/src/src/gdb/blockframe.c,v
retrieving revision 1.110
diff -u -p -r1.110 blockframe.c
--- blockframe.c	19 Jul 2006 02:17:23 -0000	1.110
+++ blockframe.c	6 Oct 2006 00:47:59 -0000
@@ -358,14 +358,15 @@ block_innermost_frame (struct block *blo
   start = BLOCK_START (block);
   end = BLOCK_END (block);
 
-  frame = NULL;
-  while (1)
+  frame = get_current_frame ();
+  while (frame != NULL)
     {
-      frame = get_prev_frame (frame);
-      if (frame == NULL)
-	return NULL;
       calling_pc = get_frame_address_in_block (frame);
       if (calling_pc >= start && calling_pc < end)
 	return frame;
+
+      frame = get_prev_frame (frame);
     }
+
+  return NULL;
 }

Re: [RFA] Fix frame-issue with watchpoints...

[Daniel Jacobowitz]

On Thu, Oct 05, 2006 at 05:50:09PM -0700, Joel Brobecker wrote:
> The issue in our case is that "current_frame" is NULL too, probably
> because we never needed it before in our case (just finished off
> single-stepping out of the breakpoint and immediately getting ready
> to resume) and therefore never set it to a proper value.

This is 100% bogus.  If we leave this code at all, we should absolutely
do this:

> One way to bandaid this, probably along the lines that Andrew was
> trying to do (try to recover from a situation that should not happen),
> is to replace "current_frame" by get_current_frame().

Or just kill it.  As far as I can see though, this:

> However, I'm thinking that the real culprit is block_innermost_frame
> which called this function with a NULL frame, which it's not supposed
> to do. So I rewrote the loop to start from the outermost frame instead
> of a NULL frame, and then work its way up the frame stack...

is also right.  Except that you really scared me here: you are actually
starting from the _innermost_ frame, not the _outermost_.

> 2006-10-05  Joel Brobecker  <brobecker@adacore.com>
> 
>         * blockframe.c (block_innermost_frame): Rewrite frame search logic.
> 
> Tested on x86-linux, no regression. A new testcase to be submitted soon.
> OK to apply?

OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Fix frame-issue with watchpoints...

[Joel Brobecker]

> > The issue in our case is that "current_frame" is NULL too, probably
> > because we never needed it before in our case (just finished off
> > single-stepping out of the breakpoint and immediately getting ready
> > to resume) and therefore never set it to a proper value.
> 
> This is 100% bogus.  If we leave this code at all, we should absolutely
> do this:
> 
> > One way to bandaid this, probably along the lines that Andrew was
> > trying to do (try to recover from a situation that should not happen),
> > is to replace "current_frame" by get_current_frame().
> 
> Or just kill it.

Let me do a testsuite run without this code, and see what comes out
of it. Perhaps it's time to kill it.

> is also right.  Except that you really scared me here: you are actually
> starting from the _innermost_ frame, not the _outermost_.

Argh, yes, of course! I keep thinking the wrong way when I don't pay
attention. Sorry.

> > 2006-10-05  Joel Brobecker  <brobecker@adacore.com>
> > 
> >         * blockframe.c (block_innermost_frame): Rewrite frame search logic.
> > 
> > Tested on x86-linux, no regression. A new testcase to be submitted soon.
> > OK to apply?
> 
> OK.

Thanks, now applied. (and thanks for the lightning-fast review, it is
always very much appreciated)

-- 
Joel

Re: [RFA] Fix frame-issue with watchpoints...

[Joel Brobecker]

> > > One way to bandaid this, probably along the lines that Andrew was
> > > trying to do (try to recover from a situation that should not happen),
> > > is to replace "current_frame" by get_current_frame().
> > 
> > Or just kill it.
> 
> Let me do a testsuite run without this code, and see what comes out
> of it. Perhaps it's time to kill it.

Hum, unfortunately we had one testcase where the assertion failed:

-var-create a2 0xbffff6b8 a^M
~"frame.c:1238: internal-error: get_prev_frame: Assertion `this_frame != NULL' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit this debugging session? "^M
~"(y or n) "^M
ERROR: Got interactive prompt.
UNRESOLVED: gdb.mi/mi-var-display.exp: create variable a2 in different scope

-- 
Joel