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

[Joel Brobecker <brobecker@adacore.com>]

[-- 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;
 }