Re: [PATCH 2/2] Report stop locations in inlined functions.

[Keith Seitz <keiths@redhat.com>]

On 07/18/2017 10:46 AM, Pedro Alves wrote:

> Maybe we need to move this bit in infrun.c:
> 
>   /* See if there is a breakpoint/watchpoint/catchpoint/etc. that
>      handles this event.  */
>   ecs->event_thread->control.stop_bpstat
>     = bpstat_stop_status (get_regcache_aspace (get_current_regcache ()),
> 			  stop_pc, ecs->ptid, &ecs->ws, stop_chain);
> 
> a bit above, before the skip_inline_frames call, and then
> you don't even need to pass down the bpstat to skip_inline_frames,
> as you can then access it from the thread directly?

I've tried this approach, and it causes a single test regression in gdb.python/py-finish-breakpoint.exp.

It's taken me quite some time to figure out what was happening, but the key is that the regressing test involved a breakpoint condition that was an inferior function call (which failed). In that case, the global inline frame state was left with a stray inline frame and the assertion in skip_inline_frames was triggered.

There are two ways to fix this. The easy way is to call clear_inline_frame_state sometime after the exception occurred, such as where the exception is caught in bpstat_check_breakpoint_conditions(*1). I'm not entirely confident that doing this every time is safe, so I'm leaning toward option #2.

The "more" complex option is to only create the stop_chain, later passing it to bpstat_stop_status(*2). This avoids checking the breakpoint condition before calling skip_inline_frames, avoiding the exception problem altogether.

If you've a preference, I'd like to hear it.

Keith

*1 The "simpler" solution (for illustration only)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 32ceea7..4d2ebd8 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -68,6 +68,7 @@
 #include "format.h"
 #include "thread-fsm.h"
 #include "tid-parse.h"
+#include "inline-frame.h"
 
 /* readline include files */
 #include "readline/readline.h"
@@ -5359,6 +5360,7 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
            }
          CATCH (ex, RETURN_MASK_ALL)
            {
+             clear_inline_frame_state (ptid);
              exception_fprintf (gdb_stderr, ex,
                                 "Error in testing breakpoint condition:\n");
            }
diff --git a/gdb/infrun.c b/gdb/infrun.c
index d00c5f6..60fd166 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5864,6 +5864,12 @@ handle_signal_stop (struct execution_control_state *ecs)
   stop_print_frame = 1;
   stopped_by_random_signal = 0;
 
+  /* See if there is a breakpoint/watchpoint/catchpoint/etc. that
+     handles this event.  */
+  ecs->event_thread->control.stop_bpstat
+    = bpstat_stop_status (get_regcache_aspace (get_current_regcache ()),
+			  stop_pc, ecs->ptid, &ecs->ws);
+
   /* Hide inlined functions starting here, unless we just performed stepi or
      nexti.  After stepi and nexti, always show the innermost frame (not any
      inline function call sites).  */
@@ -5894,7 +5900,12 @@ handle_signal_stop (struct execution_control_state *ecs)
 					     ecs->event_thread->prev_pc,
 					     &ecs->ws)))
 	{
-	  skip_inline_frames (ecs->ptid);
+	  struct breakpoint *bpt = NULL;
+
+	  if (ecs->event_thread->control.stop_bpstat != NULL)
+	    bpt = ecs->event_thread->control.stop_bpstat->breakpoint_at;
+
+	  skip_inline_frames (ecs->ptid, bpt);
 
 	  /* Re-fetch current thread's frame in case that invalidated
 	     the frame cache.  */
@@ -5939,12 +5950,6 @@ handle_signal_stop (struct execution_control_state *ecs)
 	}
     }
 
-  /* See if there is a breakpoint/watchpoint/catchpoint/etc. that
-     handles this event.  */
-  ecs->event_thread->control.stop_bpstat
-    = bpstat_stop_status (get_regcache_aspace (get_current_regcache ()),
-			  stop_pc, ecs->ptid, &ecs->ws);
-
   /* Following in case break condition called a
      function.  */
   stop_print_frame = 1;



*2 The more "complex," safer solution (for illustration only)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index d00c5f6..8c70aba 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5863,6 +5863,7 @@ handle_signal_stop (struct execution_control_state *ecs)
   ecs->event_thread->control.stop_step = 0;
   stop_print_frame = 1;
   stopped_by_random_signal = 0;
+  bpstat stop_chain = NULL;
 
   /* Hide inlined functions starting here, unless we just performed stepi or
      nexti.  After stepi and nexti, always show the innermost frame (not any
@@ -5894,7 +5895,13 @@ handle_signal_stop (struct execution_control_state *ecs)
                                             ecs->event_thread->prev_pc,
                                             &ecs->ws)))
        {
-         skip_inline_frames (ecs->ptid);
+         struct breakpoint *bpt = NULL;
+
+         stop_chain = build_bpstat_chain (aspace, stop_pc, &ecs->ws);
+         if (stop_chain != NULL)
+           bpt = stop_chain->breakpoint_at;
+
+         skip_inline_frames (ecs->ptid, bpt);
 
          /* Re-fetch current thread's frame in case that invalidated
             the frame cache.  */
@@ -5943,7 +5950,7 @@ handle_signal_stop (struct execution_control_state *ecs)
      handles this event.  */
   ecs->event_thread->control.stop_bpstat
     = bpstat_stop_status (get_regcache_aspace (get_current_regcache ()),
-                         stop_pc, ecs->ptid, &ecs->ws);
+                         stop_pc, ecs->ptid, &ecs->ws, stop_chain);
 
   /* Following in case break condition called a
      function.  */