diff mbox

[2/2] Report stop locations in inlined functions.

Message ID 1499740601-15957-2-git-send-email-keiths@redhat.com
State New
Headers show

Commit Message

Keith Seitz July 11, 2017, 2:36 a.m. UTC
This is a patch for a very related inline function problem.  Using the
test case from breakpoints/17534,

3	static inline void NVIC_EnableIRQ(int IRQn)
4	{
5	  volatile int y;
6	  y = IRQn;
7	}
8
9	__attribute__( ( always_inline ) ) static inline void __WFI(void)
10	{
11	    __asm volatile ("nop");
12	}
13
14	int main(void) {
15
16	    x= 42;
17
18	    if (x)
19	      NVIC_EnableIRQ(16);
20	    else
21	      NVIC_EnableIRQ(18);
(gdb) b NVIC_EnableIRQ
Breakpoint 1 at 0x4003e4: NVIC_EnableIRQ. (2 locations)
(gdb) r
Starting program: 17534

Breakpoint 1, main () at 17534.c:19
19	      NVIC_EnableIRQ(16);

This happens because skip_inline_frames automatically skips every inlined
frame.  Based on a suggestion by Jan, this patch introduces a new function,
breakpoint_for_stop, which attempts to ascertain which breakpoint, if any,
caused a particular stop in the inferior.  That breakpoint is then passed
to skip_inline_frames so that it can decide if a particular inlined frame
should be skipped.

I've had to separate the bpstat chain building from bpstat_stop_status --
py-finish-breakpoint.exp did not like me calling bpstat_stop_status multiple
times.  So I've added the ability to allocate the chain separately and
optionally pass it to bpstat_stop_status, which remains otherwise unchanged.

With this patch, GDB now correctly reports that the inferior has stopped
inside the inlined function:

(gdb) r
Starting program: 17534

Breakpoint 1, NVIC_EnableIRQ (IRQn=16) at 17534.c:6
6	  y = IRQn;

I don't quite like this, though.  This solution involves calling
decode_line_full, and that is really expensive, so I would be grateful if
maintaienrs could offer advice on how to better tackle this.

gdb/ChangeLog:
2017-MM-DD  Keith Seitz  <keiths@redhat.com>

	* breakpoint.c (bpstat_explains_signal): Add output parameter for
	breakpoint and save the breakpoint if one is found to explain
	the signal.
	All callers updated.
	(build_bpstat_chain): New function, moved from bpstat_stop_status.
	(breakpoint_for_stop): New function.
	(bpstat_stop_status): Add new optional parameter for the bpstat chain.
	If this new parameter is NULL, call build_bpstat_chain.
	All callers updated.
	* breakpoint.h (breakpoint_for_stop): Declare.
	(bpstat_explains_signal): Update declaration.
	* infrun.c (handle_signal_stop): Before calling skip_inline_frames,
	use breakpoint_for_stop to find the breakpoint that caused us
	to stop.
	Save the bpstat chain for later invocation of bpstat_stop_status.
	* inline-frame.c: Include linespec.h.
	(skip_inline_frames): Add struct breakpoint parameter.
	Re-parse the location of the breakpoint causing the stop, if any,
	and only skip frames that did not cause the stop.
	* inline-frame.h (skip_inline_frames): Update declaration.

gdb/testsuite/ChangeLog:
2017-MM-DD  Keith Seitz  <keiths@redhat.com>

	* gdb.opt/inline-break.c (inline_func1, not_inline_func1)
	(inline_func2, not_inline_func2, inline_func3, not_inline_func3):
	New functions.
	(main): Call not_inline_func3.
	* gdb.opt/inline-break.exp: Start inferior and set breakpoints at
	inline_func1, inline_func2, and inline_func3.  Test that when each
	breakpoint is hit, GDB properly reports both the stop location
	and the backtrace.
---
 gdb/breakpoint.c                       | 128 +++++++++++++++++++++------------
 gdb/breakpoint.h                       |  19 ++++-
 gdb/infrun.c                           |  22 +++---
 gdb/inline-frame.c                     |  46 +++++++++++-
 gdb/inline-frame.h                     |   2 +-
 gdb/testsuite/gdb.opt/inline-break.c   |  50 +++++++++++++
 gdb/testsuite/gdb.opt/inline-break.exp |  35 +++++++++
 7 files changed, 244 insertions(+), 58 deletions(-)
diff mbox

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index c283ec0..0a087ec 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4547,7 +4547,7 @@  bpstat_find_breakpoint (bpstat bsp, struct breakpoint *breakpoint)
 /* See breakpoint.h.  */
 
 int
-bpstat_explains_signal (bpstat bsp, enum gdb_signal sig)
+bpstat_explains_signal (bpstat bsp, enum gdb_signal sig, struct breakpoint **bp)
 {
   for (; bsp != NULL; bsp = bsp->next)
     {
@@ -4556,13 +4556,21 @@  bpstat_explains_signal (bpstat bsp, enum gdb_signal sig)
 	  /* A moribund location can never explain a signal other than
 	     GDB_SIGNAL_TRAP.  */
 	  if (sig == GDB_SIGNAL_TRAP)
-	    return 1;
+	    {
+	      if (bp != NULL)
+		*bp = NULL;
+	      return 1;
+	    }
 	}
       else
 	{
 	  if (bsp->breakpoint_at->ops->explains_signal (bsp->breakpoint_at,
 							sig))
-	    return 1;
+	    {
+	      if (bp != NULL)
+		*bp = bsp->breakpoint_at;;
+	      return 1;
+	    }
 	}
     }
 
@@ -5597,47 +5605,16 @@  need_moribund_for_location_type (struct bp_location *loc)
 	      && !target_supports_stopped_by_hw_breakpoint ()));
 }
 
+/* Build the bstat chain for the stop information given by ASPACE,
+   BP_ADDR, and WS.  BS_LINK should point to storage which may be subsequently
+   passed to bpstat_stop_status to avoid rebuilding the stop chain.  */
 
-/* Get a bpstat associated with having just stopped at address
-   BP_ADDR in thread PTID.
-
-   Determine whether we stopped at a breakpoint, etc, or whether we
-   don't understand this stop.  Result is a chain of bpstat's such
-   that:
-
-   if we don't understand the stop, the result is a null pointer.
-
-   if we understand why we stopped, the result is not null.
-
-   Each element of the chain refers to a particular breakpoint or
-   watchpoint at which we have stopped.  (We may have stopped for
-   several reasons concurrently.)
-
-   Each element of the chain has valid next, breakpoint_at,
-   commands, FIXME??? fields.  */
-
-bpstat
-bpstat_stop_status (struct address_space *aspace,
-		    CORE_ADDR bp_addr, ptid_t ptid,
-		    const struct target_waitstatus *ws)
+static void
+build_bpstat_chain (bpstat *bs_link, struct address_space *aspace,
+		    CORE_ADDR bp_addr, const struct target_waitstatus *ws)
 {
-  struct breakpoint *b = NULL;
+  struct breakpoint *b;
   struct bp_location *bl;
-  struct bp_location *loc;
-  /* First item of allocated bpstat's.  */
-  bpstat bs_head = NULL, *bs_link = &bs_head;
-  /* Pointer to the last thing in the chain currently.  */
-  bpstat bs;
-  int ix;
-  int need_remove_insert;
-  int removed_any;
-
-  /* First, build the bpstat chain with locations that explain a
-     target stop, while being careful to not set the target running,
-     as that may invalidate locations (in particular watchpoint
-     locations are recreated).  Resuming will happen here with
-     breakpoint conditions or watchpoint expressions that include
-     inferior function calls.  */
 
   ALL_BREAKPOINTS (b)
     {
@@ -5663,8 +5640,8 @@  bpstat_stop_status (struct address_space *aspace,
 	  /* Come here if it's a watchpoint, or if the break address
 	     matches.  */
 
-	  bs = bpstat_alloc (bl, &bs_link);	/* Alloc a bpstat to
-						   explain stop.  */
+	  bpstat bs = bpstat_alloc (bl, &bs_link);	/* Alloc a bpstat to
+							   explain stop.  */
 
 	  /* Assume we stop.  Should we find a watchpoint that is not
 	     actually triggered, or if the condition of the breakpoint
@@ -5689,12 +5666,15 @@  bpstat_stop_status (struct address_space *aspace,
   if (!target_supports_stopped_by_sw_breakpoint ()
       || !target_supports_stopped_by_hw_breakpoint ())
     {
-      for (ix = 0; VEC_iterate (bp_location_p, moribund_locations, ix, loc); ++ix)
+      struct bp_location *loc;
+
+      for (int ix = 0;
+	   VEC_iterate (bp_location_p, moribund_locations, ix, loc); ++ix)
 	{
 	  if (breakpoint_location_address_match (loc, aspace, bp_addr)
 	      && need_moribund_for_location_type (loc))
 	    {
-	      bs = bpstat_alloc (loc, &bs_link);
+	      bpstat bs = bpstat_alloc (loc, &bs_link);
 	      /* For hits of moribund locations, we should just proceed.  */
 	      bs->stop = 0;
 	      bs->print = 0;
@@ -5702,6 +5682,64 @@  bpstat_stop_status (struct address_space *aspace,
 	    }
 	}
     }
+}
+
+/* See breakpoint.h.  */
+
+struct breakpoint *
+breakpoint_for_stop (struct address_space *aspace, CORE_ADDR stop_pc,
+		     enum gdb_signal stop_signal,
+		     const struct target_waitstatus *ws,
+		     bpstat *storage)
+{
+  build_bpstat_chain (storage, aspace, stop_pc, ws);
+
+  struct breakpoint *bpt = NULL;
+  if (bpstat_explains_signal (*storage, stop_signal, &bpt))
+    return bpt;
+
+  return NULL;
+}
+
+/* Get a bpstat associated with having just stopped at address
+   BP_ADDR in thread PTID.
+
+   Determine whether we stopped at a breakpoint, etc, or whether we
+   don't understand this stop.  Result is a chain of bpstat's such
+   that:
+
+   if we don't understand the stop, the result is a null pointer.
+
+   if we understand why we stopped, the result is not null.
+
+   Each element of the chain refers to a particular breakpoint or
+   watchpoint at which we have stopped.  (We may have stopped for
+   several reasons concurrently.)
+
+   Each element of the chain has valid next, breakpoint_at,
+   commands, FIXME??? fields.  */
+
+bpstat
+bpstat_stop_status (struct address_space *aspace,
+		    CORE_ADDR bp_addr, ptid_t ptid,
+		    const struct target_waitstatus *ws,
+		    bpstat opt_chain)
+{
+  struct breakpoint *b = NULL;
+  /* First item of allocated bpstat's.  */
+  bpstat bs_head = opt_chain;
+  bpstat bs;
+  int need_remove_insert;
+  int removed_any;
+
+  /* First, build the bpstat chain with locations that explain a
+     target stop, while being careful to not set the target running,
+     as that may invalidate locations (in particular watchpoint
+     locations are recreated).  Resuming will happen here with
+     breakpoint conditions or watchpoint expressions that include
+     inferior function calls.  */
+  if (bs_head == NULL)
+    build_bpstat_chain (&bs_head, aspace, bp_addr, ws);
 
   /* A bit of special processing for shlib breakpoints.  We need to
      process solib loading here, so that the lists of loaded and
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 2f10c3b..1d7ed48 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -918,7 +918,21 @@  extern bpstat bpstat_copy (bpstat);
 
 extern bpstat bpstat_stop_status (struct address_space *aspace,
 				  CORE_ADDR pc, ptid_t ptid,
-				  const struct target_waitstatus *ws);
+				  const struct target_waitstatus *ws,
+				  bpstat opt_chain);
+
+/* If the stop described by ASPACE, STOP_PC, STOP_SIGNAL, and WS was
+   the result of a breakpoint, return that breakpoint;  otherwise return
+   NULL.
+
+   STORAGE will hold the bpstat chain and may be passed to bpstat_stop_status
+   to avoid rebuilding the entire stop chain multiple times.  */
+
+extern struct breakpoint *
+  breakpoint_for_stop (struct address_space *aspace, CORE_ADDR stop_pc,
+		       enum gdb_signal stop_signal,
+		       const struct target_waitstatus *ws,
+		       bpstat *storage);
 
 /* This bpstat_what stuff tells wait_for_inferior what to do with a
    breakpoint (a challenging task).
@@ -1028,7 +1042,8 @@  bpstat bpstat_find_breakpoint (bpstat, struct breakpoint *);
 /* Nonzero if a signal that we got in target_wait() was due to
    circumstances explained by the bpstat; the signal is therefore not
    random.  */
-extern int bpstat_explains_signal (bpstat, enum gdb_signal);
+extern int bpstat_explains_signal (bpstat, enum gdb_signal,
+				   struct breakpoint **);
 
 /* Nonzero is this bpstat causes a stop.  */
 extern int bpstat_causes_stop (bpstat);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 5e4cd51..7efafa6 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4276,7 +4276,7 @@  handle_syscall_event (struct execution_control_state *ecs)
 
       ecs->event_thread->control.stop_bpstat
 	= bpstat_stop_status (get_regcache_aspace (regcache),
-			      stop_pc, ecs->ptid, &ecs->ws);
+			      stop_pc, ecs->ptid, &ecs->ws, NULL);
 
       if (handle_stop_requested (ecs))
 	return 0;
@@ -4979,7 +4979,7 @@  handle_inferior_event_1 (struct execution_control_state *ecs)
 
 	  ecs->event_thread->control.stop_bpstat
 	    = bpstat_stop_status (get_regcache_aspace (regcache),
-				  stop_pc, ecs->ptid, &ecs->ws);
+				  stop_pc, ecs->ptid, &ecs->ws, NULL);
 
 	  if (handle_stop_requested (ecs))
 	    return;
@@ -5233,7 +5233,7 @@  Cannot fill $_exitsignal with the correct signal number.\n"));
 
       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, NULL);
 
       if (handle_stop_requested (ecs))
 	return;
@@ -5345,7 +5345,7 @@  Cannot fill $_exitsignal with the correct signal number.\n"));
 
       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, NULL);
 
       /* Note that this may be referenced from inside
 	 bpstat_stop_status above, through inferior_has_execd.  */
@@ -5884,6 +5884,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
@@ -5915,7 +5916,12 @@  handle_signal_stop (struct execution_control_state *ecs)
 					     ecs->event_thread->prev_pc,
 					     &ecs->ws)))
 	{
-	  skip_inline_frames (ecs->ptid);
+	  struct breakpoint *bpt
+	    = breakpoint_for_stop (aspace, stop_pc,
+				   ecs->event_thread->suspend.stop_signal,
+				   &ecs->ws, &stop_chain);
+
+	  skip_inline_frames (ecs->ptid, bpt);
 
 	  /* Re-fetch current thread's frame in case that invalidated
 	     the frame cache.  */
@@ -5964,7 +5970,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.  */
@@ -5981,7 +5987,7 @@  handle_signal_stop (struct execution_control_state *ecs)
   if (debug_infrun
       && ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP
       && !bpstat_explains_signal (ecs->event_thread->control.stop_bpstat,
-				  GDB_SIGNAL_TRAP)
+				  GDB_SIGNAL_TRAP, NULL)
       && stopped_by_watchpoint)
     fprintf_unfiltered (gdb_stdlog,
 			"infrun: no user watchpoint explains "
@@ -6010,7 +6016,7 @@  handle_signal_stop (struct execution_control_state *ecs)
   /* See if the breakpoints module can explain the signal.  */
   random_signal
     = !bpstat_explains_signal (ecs->event_thread->control.stop_bpstat,
-			       ecs->event_thread->suspend.stop_signal);
+			       ecs->event_thread->suspend.stop_signal, NULL);
 
   /* Maybe this was a trap for a software breakpoint that has since
      been removed.  */
diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
index b6154cd..006ae0d 100644
--- a/gdb/inline-frame.c
+++ b/gdb/inline-frame.c
@@ -27,6 +27,7 @@ 
 #include "symtab.h"
 #include "vec.h"
 #include "frame.h"
+#include "linespec.h"
 
 /* We need to save a few variables for every thread stopped at the
    virtual call site of an inlined function.  If there was always a
@@ -301,13 +302,34 @@  block_starting_point_at (CORE_ADDR pc, const struct block *block)
    user steps into them.  */
 
 void
-skip_inline_frames (ptid_t ptid)
+skip_inline_frames (ptid_t ptid, struct breakpoint *bpt)
 {
   CORE_ADDR this_pc;
   const struct block *frame_block, *cur_block;
   struct symbol *last_sym = NULL;
   int skip_count = 0;
   struct inline_state *state;
+  struct linespec_result canonical;
+
+  canonical.sals = NULL;
+  if (bpt != NULL)
+    {
+      const struct event_location *location = bpt->location.get ();
+
+      if (location != NULL && event_location_type (location) != PROBE_LOCATION)
+	{
+	  TRY
+	    {
+	      decode_line_full (location, DECODE_LINE_FUNFIRSTLINE, bpt->pspace,
+				NULL, 0, &canonical, multiple_symbols_all,
+				NULL);
+	    }
+	  CATCH (e, RETURN_MASK_ERROR)
+	    {
+	    }
+	  END_CATCH
+	}
+    }
 
   /* This function is called right after reinitializing the frame
      cache.  We try not to do more unwinding than absolutely
@@ -327,7 +349,27 @@  skip_inline_frames (ptid_t ptid)
 	      if (BLOCK_START (cur_block) == this_pc
 		  || block_starting_point_at (this_pc, cur_block))
 		{
-		  skip_count++;
+		  int lsal_i;
+		  struct linespec_sals *lsal;
+		  bool skip_this_frame = true;
+
+		  for (lsal_i = 0;
+		       VEC_iterate (linespec_sals, canonical.sals,
+				    lsal_i, lsal); lsal_i++)
+		    {
+		      struct symtabs_and_lines &sals = lsal->sals;
+
+		      for (int sals_i = 0; sals_i < sals.nelts; sals_i++)
+			{
+			  struct symtab_and_line &sal = sals.sals[sals_i];
+
+			  if (sal.pc == this_pc)
+			    skip_this_frame = false;
+			}
+		    }
+
+		  if (skip_this_frame)
+		    skip_count++;
 		  last_sym = BLOCK_FUNCTION (cur_block);
 		}
 	      else
diff --git a/gdb/inline-frame.h b/gdb/inline-frame.h
index e6fe29d..8afaf6c 100644
--- a/gdb/inline-frame.h
+++ b/gdb/inline-frame.h
@@ -31,7 +31,7 @@  extern const struct frame_unwind inline_frame_unwind;
    Frames for the hidden functions will not appear in the backtrace until the
    user steps into them.  */
 
-void skip_inline_frames (ptid_t ptid);
+void skip_inline_frames (ptid_t ptid, struct breakpoint *bpt);
 
 /* Forget about any hidden inlined functions in PTID, which is new or
    about to be resumed.  If PTID is minus_one_ptid, forget about all
diff --git a/gdb/testsuite/gdb.opt/inline-break.c b/gdb/testsuite/gdb.opt/inline-break.c
index 2616a0e..a42bc0b 100644
--- a/gdb/testsuite/gdb.opt/inline-break.c
+++ b/gdb/testsuite/gdb.opt/inline-break.c
@@ -128,6 +128,54 @@  func8a (int x)
   return func8b (x * 31);
 }
 
+static inline ATTR int
+inline_func1 (int x)
+{
+  int y = 1;			/* inline_func1  */
+
+  return y + x;
+}
+
+static int
+not_inline_func1 (int x)
+{
+  int y = 2;			/* not_inline_func1  */
+
+  return y + inline_func1 (x);
+}
+
+inline ATTR int
+inline_func2 (int x)
+{
+  int y = 3;			/* inline_func2  */
+
+  return y + not_inline_func1 (x);
+}
+
+int
+not_inline_func2 (int x)
+{
+  int y = 4;			/* not_inline_func2  */
+
+  return y + inline_func2 (x);
+}
+
+static inline ATTR int
+inline_func3 (int x)
+{
+  int y = 5;			/* inline_func3  */
+
+  return y + not_inline_func2 (x);
+}
+
+static int
+not_inline_func3 (int x)
+{
+  int y = 6;			/* not_inline_func3  */
+
+  return y + inline_func3 (x);
+}
+
 /* Entry point.  */
 
 int
@@ -155,5 +203,7 @@  main (int argc, char *argv[])
 
   x = func8a (x) + func8b (x);
 
+  x = not_inline_func3 (-21);
+
   return x;
 }
diff --git a/gdb/testsuite/gdb.opt/inline-break.exp b/gdb/testsuite/gdb.opt/inline-break.exp
index 91766d4..7965c77 100644
--- a/gdb/testsuite/gdb.opt/inline-break.exp
+++ b/gdb/testsuite/gdb.opt/inline-break.exp
@@ -211,4 +211,39 @@  for {set i 1} {$i <= [llength [array names results]]} {incr i} {
 gdb_test "interpreter-exec mi -break-info 1" \
     ".*,call-site-func=\"main\",call-site-file=\".*\",call-site-fullname=\".*\",call-site-line=\".*\".*"
 
+# Start us running.
+if {![runto main]} {
+    untested "could not run to main"
+    return -1
+}
+
+# Insert breakpoints for all inline_func? and not_inline_func? and check
+# that we actually stop where we think we should.
+
+for {set i 1} {$i < 4} {incr i} {
+    foreach inline {"not_inline" "inline"} {
+	gdb_breakpoint "${inline}_func$i" message
+    }
+}
+
+set ws {[\r\n\t ]+}
+set backtrace [list "(in|at)? main"]
+for {set i 3} {$i > 0} {incr i -1} {
+
+    foreach inline {"not_inline" "inline"} {
+
+	# Check that we stop at the correct location and print out
+	# the (possibly) inlined frames.
+	set num [gdb_get_line_number "/* ${inline}_func$i  */"]
+	set pattern ".*/$srcfile:$num${ws}.*$num${ws}int y = $decimal;"
+	append pattern "${ws}/\\\* ${inline}_func$i  \\\*/"
+	send_log "Expecting $pattern\n"
+	gdb_continue_to_breakpoint "${inline}_func$i" $pattern
+
+	# Also check for the correct backtrace.
+	set backtrace [linsert $backtrace 0 "(in|at)?${ws}${inline}_func$i"]
+	gdb_test_sequence "bt" "bt stopped in ${inline}_func$i" $backtrace
+    }
+}
+
 unset -nocomplain results