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

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

[Keith Seitz]

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 --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);
 \f
 /* 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
-- 
2.1.0

[PATCH 1/2] Report call site for inlined functions

[Keith Seitz]

Currently, "info break" can show some (perhaps) unexpected results when
setting a breakpoint on an inlined function:

(gdb) list
1	#include <stdio.h>
2
3	static inline void foo()
4	{
5	        printf("Hello world\n");
6	}
7
8	int main()
9	{
10	        foo();
11	        return 0;
12	}
13
(gdb) b foo
Breakpoint 1 at 0x400434: file 1228556.c, line 5.
(gdb) i b
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   0x0000000000400434 in main at 1228556.c:5

GDB reported that we understood what "foo" was, but we then report that the
breakpoint is actually set in main. While that is literally true, we can
do a little better.

I have chosen to record the actual symbol that we found during the parse
in the SaL.  Later that information is copied into the bp_location.  From
there, print_breakpoint_location can use this information to ascertain
that the symbol is really an inlined function, and it can report the
real symbol (foo) and inform the user of the actual call site (where the
breakpoint is really set):

(gdb) i b
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   0x0000000000400434 in foo at 1228556.c:3
                                                   inlined in main
                                                   at 1228556.c:10

I have reported this information through to MI so that UI writers can inform
their users as well:

(gdb) interpreter-exec mi -break-info
^done,BreakpointTable={nr_rows="1",nr_cols="6",hdr=[{width="7",alignment="-1",col_name="number",colhdr="Num"},{width="14",alignment="-1",col_name="type",colhdr="Type"},{width="4",alignment="-1",col_name="disp",colhdr="Disp"},{width="3",alignment="-1",col_name="enabled",colhdr="Enb"},{width="18",alignment="-1",col_name="addr",colhdr="Address"},{width="40",alignment="2",col_name="what",colhdr="What"}],body=[bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000400434",func="foo",file="1228556.c",fullname="/home/keiths/tmp/1228556.c",line="3",call-site-func="main",call-site-file="1228556.c",call-site-fullname="/home/keiths/tmp/1228556.c",call-site-line="10",thread-groups=["i1"],times="0",original-location="foo"}]}

Here you can see the new call-site-func, call-site-file, call-site-fullname,
and call-site-line.

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

	* breakpoint.c (print_breakpoint_location): Print out call site
	information for inlined functions.
	(add_location_to_breakpoint): Save sal->symbol.
	* breakpoint.h (struct bp_location)<symbol>: New field.
	* dwarf2read.c (dwarf2_file_symtab): New function.
	(read_func_scope): For inlined functions, allocate an inlined_symbol.
	(new_symbol_full): Move call-file/dec_file handling to
	dwarf2_file_symtab and use it.
	* symtab.c (find_function_start_sal): Save the symbol into the SaL.
	(allocate_inlined_symbol): New function.
	* symtab.h (struct inlined_symbol): New structure.
	(struct symtab_and_line)<symbol>: New field.
	(allocate_inlined_symbol): Declare.

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

	* gdb.texinfo (GDB/MI Breakpoint Information): Mention call-site-func,
	call-site-file, call-site-fullname, and call-site-line.

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

	* gdb.opt/inline-break.exp (break_info_1): New procedure.
	Test "info break" for every inlined function breakpoint.
	Test output of -break-info.
---
 gdb/breakpoint.c                       |  70 +++++++++++++++++++----
 gdb/breakpoint.h                       |   5 ++
 gdb/doc/gdb.texinfo                    |   8 +++
 gdb/dwarf2read.c                       |  72 ++++++++++++++++-------
 gdb/symtab.c                           |  14 +++++
 gdb/symtab.h                           |  20 +++++++
 gdb/testsuite/gdb.opt/inline-break.exp | 101 +++++++++++++++++++++++++++++++++
 7 files changed, 260 insertions(+), 30 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 053ccef..c283ec0 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6153,24 +6153,73 @@ print_breakpoint_location (struct breakpoint *b,
     uiout->field_string ("what", event_location_to_string (b->location.get ()));
   else if (loc && loc->symtab)
     {
-      struct symbol *sym 
-	= find_pc_sect_function (loc->address, loc->section);
-      if (sym)
+      /* If a location is associated with an inlined symbol, print out
+	 information about its call site, too.  */
+      if (loc->symbol != NULL && SYMBOL_INLINED (loc->symbol))
 	{
+	  struct inlined_symbol *isym = (struct inlined_symbol *) (loc->symbol);
+	  struct symbol *sym
+	    = find_pc_sect_function (loc->address, loc->section);
+
+	  /* ISYM contains information about the inlined function, and
+	     LOC->SYMBOL describes the call site.  */
 	  uiout->text ("in ");
-	  uiout->field_string ("func", SYMBOL_PRINT_NAME (sym));
+	  uiout->field_string ("func", SYMBOL_PRINT_NAME (loc->symbol));
 	  uiout->text (" ");
 	  uiout->wrap_hint (wrap_indent_at_field (uiout, "what"));
 	  uiout->text ("at ");
+
+	  const char *s = symtab_to_filename_for_display (isym->decl_file);
+          uiout->field_string ("file", s);
+
+	  uiout->text (":");
+	  if (uiout->is_mi_like_p ())
+	    {
+	      uiout->field_string ("fullname",
+				   symtab_to_fullname (isym->decl_file));
+	    }
+	  uiout->field_int ("line", isym->decl_line);
+	  uiout->text (" ");
+	  uiout->wrap_hint (wrap_indent_at_field (uiout, "what"));
+	  uiout->text ("inlined in ");
+	  if (sym != NULL)
+	    uiout->field_string ("call-site-func", SYMBOL_PRINT_NAME (sym));
+	  uiout->text (" ");
+	  uiout->wrap_hint (wrap_indent_at_field (uiout, "what"));
+	  uiout->text ("at ");
+	  s = symtab_to_filename_for_display (symbol_symtab (loc->symbol));
+	  uiout->field_string ("call-site-file", s);
+
+	  uiout->text (":");
+	  if (uiout->is_mi_like_p ())
+	    {
+	      s = symtab_to_fullname (symbol_symtab (loc->symbol));
+	      uiout->field_string ("call-site-fullname", s);
+	    }
+	  uiout->field_int ("call-site-line", SYMBOL_LINE (loc->symbol));
 	}
-      uiout->field_string ("file",
-			   symtab_to_filename_for_display (loc->symtab));
-      uiout->text (":");
+      else
+	{
+	  struct symbol *sym
+	    = find_pc_sect_function (loc->address, loc->section);
 
-      if (uiout->is_mi_like_p ())
-	uiout->field_string ("fullname", symtab_to_fullname (loc->symtab));
+	  if (sym)
+	    {
+	      uiout->text ("in ");
+	      uiout->field_string ("func", SYMBOL_PRINT_NAME (sym));
+	      uiout->text (" ");
+	      uiout->wrap_hint (wrap_indent_at_field (uiout, "what"));
+	      uiout->text ("at ");
+	    }
+	  uiout->field_string ("file",
+			       symtab_to_filename_for_display (loc->symtab));
+	  uiout->text (":");
+
+	  if (uiout->is_mi_like_p ())
+	    uiout->field_string ("fullname", symtab_to_fullname (loc->symtab));
       
-      uiout->field_int ("line", loc->line_number);
+	  uiout->field_int ("line", loc->line_number);
+	}
     }
   else if (loc)
     {
@@ -8986,6 +9035,7 @@ add_location_to_breakpoint (struct breakpoint *b,
   loc->gdbarch = loc_gdbarch;
   loc->line_number = sal->line;
   loc->symtab = sal->symtab;
+  loc->symbol = sal->symbol;
 
   set_breakpoint_location_function (loc,
 				    sal->explicit_pc || sal->explicit_line);
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index d955184..2f10c3b 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -484,6 +484,11 @@ public:
      to find the corresponding source file name.  */
 
   struct symtab *symtab = NULL;
+
+  /* The symbol found by the location parser, if any.  This may be used to
+     ascertain when an event location was set at a different location than
+     the one originally selected by parsing, e.g., inlined symbols.  */
+  const struct symbol *symbol = NULL;
 };
 
 /* The possible return values for print_bpstat, print_it_normal,
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index c167a86..58cdc1a 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -26755,6 +26755,14 @@ known.  If not known, this field is not present.
 The line number at which this breakpoint appears, if known.
 If not known, this field is not present.
 
+@item call-site-func
+@item call-site-file
+@item call-site-fullname
+@item call-site-line
+These fields describe the call site for a breakpoint set on an inlined function.
+The fields are analogous to those fields of the same name for normal breakpoint
+locations.
+
 @item at
 If the source file is not known, this field may be provided.  If
 provided, this holds the address of the breakpoint, possibly followed
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 0fdcd42..3b3193b 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -11470,6 +11470,35 @@ inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu)
   do_cleanups (cleanups);
 }
 
+/* Get the symtab representing the KIND of file of DIE in CU or NULL
+   if no such file exists.  KIND is any valid DWARF file attribute such as
+   DW_AT_decl_file or DW_AT_call_file.  */
+
+static struct symtab *
+dwarf2_file_symtab (unsigned int kind, struct die_info *die,
+		    struct dwarf2_cu *cu)
+{
+  struct attribute *attr = dwarf2_attr (die, kind, cu);
+
+  if (attr != NULL)
+    {
+      file_name_index file_index = (file_name_index) DW_UNSND (attr);
+      struct file_entry *fe;
+
+      if (cu->line_header != NULL)
+	fe = cu->line_header->file_name_at (file_index);
+      else
+	fe = NULL;
+
+      if (fe == NULL)
+	complaint (&symfile_complaints, _("file index out of range"));
+      else
+	return fe->symtab;
+    }
+
+  return NULL;
+}
+
 static void
 read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
 {
@@ -11486,6 +11515,8 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
   int inlined_func = (die->tag == DW_TAG_inlined_subroutine);
   VEC (symbolp) *template_args = NULL;
   struct template_symbol *templ_func = NULL;
+  struct inlined_symbol *isym = NULL;
+  struct symbol *symbol_storage = NULL;
 
   if (inlined_func)
     {
@@ -11540,13 +11571,28 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
 	{
 	  templ_func = allocate_template_symbol (objfile);
 	  templ_func->base.is_cplus_template_function = 1;
+	  symbol_storage = (struct symbol *) templ_func;
 	  break;
 	}
     }
 
+  /* If we have an inlined symbol, we must also allocate a different
+     symbol.  */
+  if (inlined_func)
+    {
+      isym = allocate_inlined_symbol (objfile);
+      isym->decl_file = dwarf2_file_symtab (DW_AT_decl_file, die, cu);
+
+      attr = dwarf2_attr (die, DW_AT_decl_line, cu);
+      if (attr != NULL)
+	isym->decl_line = DW_UNSND (attr);
+
+      symbol_storage = (struct symbol *) isym;
+    }
+
   newobj = push_context (0, lowpc);
   newobj->name = new_symbol_full (die, read_type_die (die, cu), cu,
-			       (struct symbol *) templ_func);
+				  symbol_storage);
 
   /* If there is a location expression for DW_AT_frame_base, record
      it.  */
@@ -18948,25 +18994,11 @@ new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 	  SYMBOL_LINE (sym) = DW_UNSND (attr);
 	}
 
-      attr = dwarf2_attr (die,
-			  inlined_func ? DW_AT_call_file : DW_AT_decl_file,
-			  cu);
-      if (attr)
-	{
-	  file_name_index file_index = (file_name_index) DW_UNSND (attr);
-	  struct file_entry *fe;
-
-	  if (cu->line_header != NULL)
-	    fe = cu->line_header->file_name_at (file_index);
-	  else
-	    fe = NULL;
-
-	  if (fe == NULL)
-	    complaint (&symfile_complaints,
-		       _("file index out of range"));
-	  else
-	    symbol_set_symtab (sym, fe->symtab);
-	}
+      struct symtab *symtab
+	= dwarf2_file_symtab (inlined_func ? DW_AT_call_file : DW_AT_decl_file,
+			      die, cu);
+      if (symtab != NULL)
+	symbol_set_symtab (sym, symtab);
 
       switch (die->tag)
 	{
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 497d520..55bfc93 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3472,6 +3472,7 @@ find_function_start_sal (struct symbol *sym, int funfirstline)
   fixup_symbol_section (sym, NULL);
   section = SYMBOL_OBJ_SECTION (symbol_objfile (sym), sym);
   sal = find_pc_sect_line (BLOCK_START (SYMBOL_BLOCK_VALUE (sym)), section, 0);
+  sal.symbol = sym;
 
   if (funfirstline && sal.symtab != NULL
       && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
@@ -5998,6 +5999,19 @@ allocate_template_symbol (struct objfile *objfile)
   return result;
 }
 
+/* See description in symtab.h.  */
+
+struct inlined_symbol *
+allocate_inlined_symbol (struct objfile *objfile)
+{
+  struct inlined_symbol *result;
+
+  result = OBSTACK_ZALLOC (&objfile->objfile_obstack, struct inlined_symbol);
+  initialize_objfile_symbol_1 (&result->base);
+
+  return result;
+}
+
 /* See symtab.h.  */
 
 struct objfile *
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 341deca..a70ed54 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -910,6 +910,21 @@ struct template_symbol
   struct symbol **template_arguments;
 };
 
+/* A superclass of struct symbol used to represent an inlined symbol.
+   A symbol is really of this type if SYMBOL_INLINED is true.  */
+
+struct inlined_symbol
+{
+  /* The base class.  */
+  struct symbol base;
+
+  /* The inlined symbol's symtab.  */
+  struct symtab *decl_file;
+
+  /* Line number of the inlined symbol.  */
+  int decl_line;
+};
+
 \f
 /* Each item represents a line-->pc (or the reverse) mapping.  This is
    somewhat more wasteful of space than one might wish, but since only
@@ -1436,6 +1451,9 @@ struct symtab_and_line
   /* If PROBE is not NULL, then this is the objfile in which the probe
      originated.  */
   struct objfile *objfile;
+
+  /* The symbol for which this SaL was created.  */
+  struct symbol *symbol;
 };
 
 extern void init_sal (struct symtab_and_line *sal);
@@ -1676,4 +1694,6 @@ void initialize_objfile_symbol (struct symbol *);
 
 struct template_symbol *allocate_template_symbol (struct objfile *);
 
+extern struct inlined_symbol *allocate_inlined_symbol (struct objfile *);
+
 #endif /* !defined(SYMTAB_H) */
diff --git a/gdb/testsuite/gdb.opt/inline-break.exp b/gdb/testsuite/gdb.opt/inline-break.exp
index 7be3a34..91766d4 100644
--- a/gdb/testsuite/gdb.opt/inline-break.exp
+++ b/gdb/testsuite/gdb.opt/inline-break.exp
@@ -24,6 +24,76 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
     return -1
 }
 
+# Return a string that may be used to match the output of "info break NUM".
+#
+# Optional arguments:
+#
+# source    - the name of the source file
+# func      - the name of the function
+# disp      - the event disposition
+# enabled   - enable state
+# locs      - number of locations
+# line      - source line number (ignored without -source)
+# inlined   - add inlined list, argument is a list of regexps to match
+#             *per location* (e.g., [list "" "foo at $source:$line"] will yield
+#             no inline for first location, and "inlined in foo at ..." for
+#             the second location)
+
+proc break_info_1 {num args} {
+    global decimal
+
+    # Column delimiter
+    set c {[\t ]+}
+
+    # Row delimiter
+    set end {[\r\n \t]+}
+
+    # Table header
+    set header "[join [list Num Type Disp Enb Address What] ${c}]"
+
+    # Get/configure any optional parameters.
+    parse_args [list {source ""} {func ".*"} {disp "keep"} \
+		    {enabled "y"} {locs 1} [list line $decimal] \
+		    {inlined {}} {type "breakpoint"}]
+
+    if {$source != ""} {
+	set source "/$source:$line"
+    }
+
+    # Result starts with the standard header.
+    set result "$header${end}"
+
+    # Set up for multi-location breakpoint marker.
+    if {$locs == 1} {
+	set multi ".*"
+    } else {
+	set multi "<MULTIPLE>${end}"
+    }
+    append result "[join [list $num $type $disp $enabled $multi] $c]"
+
+    # Add location info.
+    for {set i 1} {$i <= $locs} {incr i} {
+	if {$locs > 1} {
+	    append result "[join [list $num.$i $enabled] $c].*"
+	}
+
+	#  Add function/source file info.
+	append result "in $func at .*$source${end}"
+
+	# Add inline, if requested.
+	if {$i <= [llength $inlined]} {
+	    set inlined_regexp [lindex $inlined [expr $i - 1]]
+	} else {
+	    set inlined_regexp ""
+	}
+	if {$inlined_regexp != ""} {
+	    append result ".*inlined in $inlined_regexp${end}"
+	}
+    }
+
+    return $result
+}
+
 #
 # func1 is a static inlined function that is called once.
 # The result should be a single-location breakpoint.
@@ -111,3 +181,34 @@ gdb_test "print func1" \
 #
 gdb_test "print func2" \
     "\\\$.* = {int \\(int\\)} .* <func2>"
+
+# Test that "info break" reports the location of the breakpoints "inside"
+# the inlined functions
+
+set results(1) [break_info_1 1 -source $srcfile -func "func1" \
+		    -inlined [list "main at .*"]]
+set results(2) [break_info_1 2 -locs 2 -source $srcfile -func "func2" \
+		    -inlined [list "" "main at .*"]]
+set results(3) [break_info_1 3 -source $srcfile -func "func3b" \
+		    -inlined [list "main at .*"]]
+set results(4) [break_info_1 4 -locs 2 -source $srcfile -func "func4b" \
+		    -inlined [list "func4a at .*" "main at .*"]]
+set results(5) [break_info_1 5 -locs 2 -source $srcfile -func "func5b" \
+		    -inlined [list "" ".*"]]
+set results(6) [break_info_1 6 -locs 3 -source $srcfile -func "func6b" \
+		    -inlined [list "" ".*"]]
+set results(7) [break_info_1 7 -locs 2 -source $srcfile -func "func7b" \
+		    -inlined [list ".*" ".*"]]
+set results(8) [break_info_1 8 -locs 3 -source $srcfile -func "func8b"\
+		    -inlined [list "" ".*" ".*"]]
+
+for {set i 1} {$i <= [llength [array names results]]} {incr i} {
+    send_log "Expecting: $results($i)\n"
+    gdb_test "info break $i" $results($i)
+}
+
+# A quick check that MI is outputting new call-site parameters.
+gdb_test "interpreter-exec mi -break-info 1" \
+    ".*,call-site-func=\"main\",call-site-file=\".*\",call-site-fullname=\".*\",call-site-line=\".*\".*"
+
+unset -nocomplain results
-- 
2.1.0

Re: [PATCH 1/2] Report call site for inlined functions

[Eli Zaretskii]

> From: Keith Seitz <keiths@redhat.com>
> Date: Mon, 10 Jul 2017 19:36:40 -0700
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index c167a86..58cdc1a 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -26755,6 +26755,14 @@ known.  If not known, this field is not present.
>  The line number at which this breakpoint appears, if known.
>  If not known, this field is not present.
>  
> +@item call-site-func
> +@item call-site-file
> +@item call-site-fullname
> +@item call-site-line

All but the first of these should be @itemx.

Does this warrant a NEWS entry?

OK for the documentation part, thanks.

Re: [PATCH 1/2] Report call site for inlined functions

[Jan Kratochvil]

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

Hi Keith,

with attached file on Fedora 25 x86_64 'gcc -g' (not any -O):

------------------------------------------------------------------------------
(gdb) b g
Breakpoint 1 at 0x4004ce: file inlined5.c, line 12.
(gdb) r
Starting program: inlined5 

Breakpoint 1, g (a=1) at inlined5.c:12
12	  f (a);
(gdb) bt
#0  g (a=1) at inlined5.c:12
#1  h (a=1) at inlined5.c:20
#2  main () at inlined5.c:28
(gdb) info breakpoints 
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   0x00000000004004ce in g at inlined5.c:10 inlined in main 
                                                   at inlined5.c:20
	breakpoint already hit 1 time
(gdb) 
------------------------------------------------------------------------------

I think g is inlined into h which is inlined into main.  Personally I think it
would be enough to say "in g" as the rest is a toolchain implementation detail.
But if it tries to describe the code layout then it should match the reality.

The MI attributes would also need to be adjusted for multi-level inlines.


Jan

[-- Attachment #2: inlined5.c --]
[-- Type: text/plain, Size: 281 bytes --]

int e;

static __attribute__ ((__noinline__)) void
f (int a)
{
  e = a;
}

static __attribute__ ((__always_inline__)) void
g (int a)
{
  f (a);

  e = a;
}

static __attribute__ ((__always_inline__)) void
h (int a)
{
  g (a);

  e = a;
}

int
main (void)
{
  h (1);

  return 0;
}

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

[Pedro Alves]

On 07/11/2017 03:36 AM, Keith Seitz wrote:
> 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.

I'm still trying to grok these patches fully, but, shouldn't comparing
the breakpoint's bp_location's addresses work the same?  I.e., with this,
gdb.opt/inline-break.exp still passes cleanly here:

From a7b8eb698d2853fc16bca1b933be5c517dd1d446 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 18 Jul 2017 18:09:50 +0100
Subject: [PATCH] no decode

---
 gdb/breakpoint.c   |  2 +-
 gdb/inline-frame.c | 45 +++++++++++----------------------------------
 2 files changed, 12 insertions(+), 35 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 05eb71e..c149e5e 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7165,7 +7165,7 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 
 */
 
-static int
+int
 breakpoint_address_is_meaningful (struct breakpoint *bpt)
 {
   enum bptype type = bpt->type;
diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
index 006ae0d..9120554 100644
--- a/gdb/inline-frame.c
+++ b/gdb/inline-frame.c
@@ -297,6 +297,8 @@ block_starting_point_at (CORE_ADDR pc, const struct block *block)
   return 1;
 }
 
+int breakpoint_address_is_meaningful (struct breakpoint *bpt);
+
 /* Skip all inlined functions whose call sites are at the current PC.
    Frames for the hidden functions will not appear in the backtrace until the
    user steps into them.  */
@@ -309,27 +311,6 @@ skip_inline_frames (ptid_t ptid, struct breakpoint *bpt)
   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
@@ -349,23 +330,19 @@ skip_inline_frames (ptid_t ptid, struct breakpoint *bpt)
 	      if (BLOCK_START (cur_block) == this_pc
 		  || block_starting_point_at (this_pc, cur_block))
 		{
-		  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++)
+		  if (bpt != NULL
+		      && breakpoint_address_is_meaningful (bpt))
 		    {
-		      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)
+		      for (bp_location *loc = bpt->loc;
+			   loc != NULL;
+			   loc = loc->next)
+			if (this_pc == loc->address)
+			  {
 			    skip_this_frame = false;
-			}
+			    break;
+			  }
 		    }
 
 		  if (skip_this_frame)
-- 
2.5.5

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

[Pedro Alves]

On 07/18/2017 06:16 PM, Pedro Alves wrote:
> On 07/11/2017 03:36 AM, Keith Seitz wrote:

>> 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.
> 
> I'm still trying to grok these patches fully, but, shouldn't comparing
> the breakpoint's bp_location's addresses work the same?  I.e., with this,
> gdb.opt/inline-break.exp still passes cleanly here:
> 

Hmm, and with this direction, we may not even need the
breakpoint_for_stop function.  The location(s) that caused
the stop is/are in the bpstat chain:

  stop_chain->bp_location_at->address

etc.  Using those also implicitly makes sure that you're only
consulting locations that were inserted/enabled, as other
not-enabled/inserted locations won't appear in the bpstat chain.

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?

Thanks,
Pedro Alves

Re: [PATCH 1/2] Report call site for inlined functions

[Pedro Alves]

On 07/11/2017 03:36 AM, Keith Seitz wrote:

> I have chosen to record the actual symbol that we found during the parse
> in the SaL.  Later that information is copied into the bp_location.  

At some point I think we'll just make bp_location hold a SaL.  :-)

> From
> there, print_breakpoint_location can use this information to ascertain
> that the symbol is really an inlined function, and it can report the
> real symbol (foo) and inform the user of the actual call site (where the
> breakpoint is really set):
> 
> (gdb) i b
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y   0x0000000000400434 in foo at 1228556.c:3
>                                                    inlined in main
>                                                    at 1228556.c:10
> 
> I have reported this information through to MI so that UI writers can inform
> their users as well:
> 
> (gdb) interpreter-exec mi -break-info
> ^done,BreakpointTable={nr_rows="1",nr_cols="6",hdr=[{width="7",alignment="-1",col_name="number",colhdr="Num"},{width="14",alignment="-1",col_name="type",colhdr="Type"},{width="4",alignment="-1",col_name="disp",colhdr="Disp"},{width="3",alignment="-1",col_name="enabled",colhdr="Enb"},{width="18",alignment="-1",col_name="addr",colhdr="Address"},{width="40",alignment="2",col_name="what",colhdr="What"}],body=[bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000400434",func="foo",file="1228556.c",fullname="/home/keiths/tmp/1228556.c",line="3",call-site-func="main",call-site-file="1228556.c",call-site-fullname="/home/keiths/tmp/1228556.c",call-site-line="10",thread-groups=["i1"],times="0",original-location="foo"}]}
> 
> Here you can see the new call-site-func, call-site-file, call-site-fullname,
> and call-site-line.

The non-inlined call site seems sufficient info to me, at least for
the  CLI.  I'm not sure whether "call-site" is sufficiently clear that
it's referring to that vs the immediate potential inlined-too
caller though.  I think at least the docs need such a clarification.


> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -6153,24 +6153,73 @@ print_breakpoint_location (struct breakpoint *b,
>      uiout->field_string ("what", event_location_to_string (b->location.get ()));
>    else if (loc && loc->symtab)
>      {
> -      struct symbol *sym 
> -	= find_pc_sect_function (loc->address, loc->section);
> -      if (sym)
> +      /* If a location is associated with an inlined symbol, print out
> +	 information about its call site, too.  */
> +      if (loc->symbol != NULL && SYMBOL_INLINED (loc->symbol))
>  	{
> +	  struct inlined_symbol *isym = (struct inlined_symbol *) (loc->symbol);
> +	  struct symbol *sym
> +	    = find_pc_sect_function (loc->address, loc->section);
> +
> +	  /* ISYM contains information about the inlined function, and
> +	     LOC->SYMBOL describes the call site.  */
>  	  uiout->text ("in ");
> -	  uiout->field_string ("func", SYMBOL_PRINT_NAME (sym));
> +	  uiout->field_string ("func", SYMBOL_PRINT_NAME (loc->symbol));
>  	  uiout->text (" ");
>  	  uiout->wrap_hint (wrap_indent_at_field (uiout, "what"));
>  	  uiout->text ("at ");
> +
> +	  const char *s = symtab_to_filename_for_display (isym->decl_file);
> +          uiout->field_string ("file", s);
> +
> +	  uiout->text (":");
> +	  if (uiout->is_mi_like_p ())
> +	    {
> +	      uiout->field_string ("fullname",
> +				   symtab_to_fullname (isym->decl_file));
> +	    }

Isn't there a good deal of duplication between the above and the
else branch?


> +	  uiout->field_int ("line", isym->decl_line);
> +	  uiout->text (" ");
> +	  uiout->wrap_hint (wrap_indent_at_field (uiout, "what"));
> +	  uiout->text ("inlined in ");
> +	  if (sym != NULL)
> +	    uiout->field_string ("call-site-func", SYMBOL_PRINT_NAME (sym));
> +	  uiout->text (" ");
> +	  uiout->wrap_hint (wrap_indent_at_field (uiout, "what"));
> +	  uiout->text ("at ");
> +	  s = symtab_to_filename_for_display (symbol_symtab (loc->symbol));
> +	  uiout->field_string ("call-site-file", s);
> +
> +	  uiout->text (":");
> +	  if (uiout->is_mi_like_p ())
> +	    {
> +	      s = symtab_to_fullname (symbol_symtab (loc->symbol));
> +	      uiout->field_string ("call-site-fullname", s);
> +	    }
> +	  uiout->field_int ("call-site-line", SYMBOL_LINE (loc->symbol));
>  	}
> -      uiout->field_string ("file",
> -			   symtab_to_filename_for_display (loc->symtab));
> -      uiout->text (":");
> +      else
> +	{
> +	  struct symbol *sym
> +	    = find_pc_sect_function (loc->address, loc->section);
>  

I wonder whether the else branch could use loc->symbol now that you
have it, instead of looking it up.


> -      if (uiout->is_mi_like_p ())
> -	uiout->field_string ("fullname", symtab_to_fullname (loc->symtab));
> +	  if (sym)
> +	    {
> +	      uiout->text ("in ");
> +	      uiout->field_string ("func", SYMBOL_PRINT_NAME (sym));
> +	      uiout->text (" ");
> +	      uiout->wrap_hint (wrap_indent_at_field (uiout, "what"));
> +	      uiout->text ("at ");
> +	    }
> +	  uiout->field_string ("file",
> +			       symtab_to_filename_for_display (loc->symtab));
> +	  uiout->text (":");
> +
> +	  if (uiout->is_mi_like_p ())
> +	    uiout->field_string ("fullname", symtab_to_fullname (loc->symtab));
>        
> -      uiout->field_int ("line", loc->line_number);
> +	  uiout->field_int ("line", loc->line_number);
> +	}
>      }
>    else if (loc)
>      {
> @@ -8986,6 +9035,7 @@ add_location_to_breakpoint (struct breakpoint *b,
>    loc->gdbarch = loc_gdbarch;
>    loc->line_number = sal->line;
>    loc->symtab = sal->symtab;
> +  loc->symbol = sal->symbol;
>  
>    set_breakpoint_location_function (loc,
>  				    sal->explicit_pc || sal->explicit_line);
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index d955184..2f10c3b 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -484,6 +484,11 @@ public:
>       to find the corresponding source file name.  */
>  
>    struct symtab *symtab = NULL;
> +
> +  /* The symbol found by the location parser, if any.  This may be used to
> +     ascertain when an event location was set at a different location than
> +     the one originally selected by parsing, e.g., inlined symbols.  */
> +  const struct symbol *symbol = NULL;
>  };
>  
>  /* The possible return values for print_bpstat, print_it_normal,
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index c167a86..58cdc1a 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -26755,6 +26755,14 @@ known.  If not known, this field is not present.
>  The line number at which this breakpoint appears, if known.
>  If not known, this field is not present.
>  
> +@item call-site-func
> +@item call-site-file
> +@item call-site-fullname
> +@item call-site-line
> +These fields describe the call site for a breakpoint set on an inlined function.
> +The fields are analogous to those fields of the same name for normal breakpoint
> +locations.

IMO, the non-inlined call site is sufficient info, at least for the 
CLI.  I'm not sure whether "call-site" is sufficiently clear that
it's referring to that vs the immediate potential inlined-too
caller though.  I think at least the docs need that clarified.

> +
>  @item at
>  If the source file is not known, this field may be provided.  If
>  provided, this holds the address of the breakpoint, possibly followed
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 0fdcd42..3b3193b 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -11470,6 +11470,35 @@ inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu)
>    do_cleanups (cleanups);
>  }
>  
> +/* Get the symtab representing the KIND of file of DIE in CU or NULL
> +   if no such file exists.  KIND is any valid DWARF file attribute such as
> +   DW_AT_decl_file or DW_AT_call_file.  */
> +
> +static struct symtab *
> +dwarf2_file_symtab (unsigned int kind, struct die_info *die,
> +		    struct dwarf2_cu *cu)
> +{
> +  struct attribute *attr = dwarf2_attr (die, kind, cu);
> +
> +  if (attr != NULL)
> +    {
> +      file_name_index file_index = (file_name_index) DW_UNSND (attr);
> +      struct file_entry *fe;
> +
> +      if (cu->line_header != NULL)
> +	fe = cu->line_header->file_name_at (file_index);
> +      else
> +	fe = NULL;
> +
> +      if (fe == NULL)
> +	complaint (&symfile_complaints, _("file index out of range"));
> +      else
> +	return fe->symtab;
> +    }
> +
> +  return NULL;
> +}
> +
>  static void
>  read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>  {
> @@ -11486,6 +11515,8 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>    int inlined_func = (die->tag == DW_TAG_inlined_subroutine);
>    VEC (symbolp) *template_args = NULL;
>    struct template_symbol *templ_func = NULL;
> +  struct inlined_symbol *isym = NULL;
> +  struct symbol *symbol_storage = NULL;
>  
>    if (inlined_func)
>      {
> @@ -11540,13 +11571,28 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>  	{
>  	  templ_func = allocate_template_symbol (objfile);
>  	  templ_func->base.is_cplus_template_function = 1;
> +	  symbol_storage = (struct symbol *) templ_func;
>  	  break;
>  	}
>      }
>  
> +  /* If we have an inlined symbol, we must also allocate a different
> +     symbol.  */

How does this work when you have an (inlined) template function?
Like e.g.,:

template<typename T>
static T inline_func (T val)
{
  return val * 2;
}

int
not_inline_func (int val)
{
  return inline_func (val * 2);
}


 <1><31>: Abbrev Number: 2 (DW_TAG_subprogram)
    <32>   DW_AT_name        : (indirect string, offset: 0x929): inline_func<int>
    <36>   DW_AT_decl_file   : 1
    <37>   DW_AT_decl_line   : 2
    <38>   DW_AT_type        : <0x54>
    <3c>   DW_AT_inline      : 1        (inlined)
    <3d>   DW_AT_sibling     : <0x54>


> +  if (inlined_func)
> +    {
> +      isym = allocate_inlined_symbol (objfile);
> +      isym->decl_file = dwarf2_file_symtab (DW_AT_decl_file, die, cu);
> +
> +      attr = dwarf2_attr (die, DW_AT_decl_line, cu);
> +      if (attr != NULL)
> +	isym->decl_line = DW_UNSND (attr);
> +
> +      symbol_storage = (struct symbol *) isym;
> +    }
> +
>    newobj = push_context (0, lowpc);
>    newobj->name = new_symbol_full (die, read_type_die (die, cu), cu,
> -			       (struct symbol *) templ_func);
> +				  symbol_storage);
>  
>    /* If there is a location expression for DW_AT_frame_base, record
>       it.  */
> @@ -18948,25 +18994,11 @@ new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>  	  SYMBOL_LINE (sym) = DW_UNSND (attr);
>  	}
>  


>  /* See symtab.h.  */
>  
>  struct objfile *
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 341deca..a70ed54 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -910,6 +910,21 @@ struct template_symbol
>    struct symbol **template_arguments;
>  };
>  
> +/* A superclass of struct symbol used to represent an inlined symbol.
> +   A symbol is really of this type if SYMBOL_INLINED is true.  */
> +
> +struct inlined_symbol
> +{
> +  /* The base class.  */
> +  struct symbol base;

Couldn't this be real C++ inheritance?

  struct inlined_symbol : symbol 
  {
     ...
  };


I ran out of time ... more later.

Thanks,
Pedro Alves

Re: [PATCH 1/2] Report call site for inlined functions

[Keith Seitz]

[My reply seems not to have made it to sourceware, so I'm resending.]

On 07/18/2017 12:05 PM, Pedro Alves wrote:
> On 07/11/2017 03:36 AM, Keith Seitz wrote:
>>
>> Here you can see the new call-site-func, call-site-file, call-site-fullname,
>> and call-site-line.
> 
> The non-inlined call site seems sufficient info to me, at least for
> the  CLI.  I'm not sure whether "call-site" is sufficiently clear that
> it's referring to that vs the immediate potential inlined-too
> caller though.  I think at least the docs need such a clarification.

There seems to be consensus on the (lack of) utility of this call-site info, so in the next revision (to follow), I've chosen to remove it all.  I think that pretty much resolves (by deletion!) most of your concerns/comments. I've also removed the inlined_symbol proposal since that is also no longer necessary. 

>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>> index 0fdcd42..3b3193b 100644
>> --- a/gdb/dwarf2read.c
>> +++ b/gdb/dwarf2read.c
>> @@ -11540,13 +11571,28 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>>  	{
>>  	  templ_func = allocate_template_symbol (objfile);
>>  	  templ_func->base.is_cplus_template_function = 1;
>> +	  symbol_storage = (struct symbol *) templ_func;
>>  	  break;
>>  	}
>>      }
>>  
>> +  /* If we have an inlined symbol, we must also allocate a different
>> +     symbol.  */
> 
> How does this work when you have an (inlined) template function?
> Like e.g.,:
> 
> template<typename T>
> static T inline_func (T val)
> {
>   return val * 2;
> }
> 
> int
> not_inline_func (int val)
> {
>   return inline_func (val * 2);
> }
> 

I realize this was asked in relation to the proposed (and now deleted) inline_symbol, but to be clear, the above example works:

(gdb) b inline_func<int>
Breakpoint 1 at 0x4004c0: file inline-t.cc, line 9.
(gdb) i b
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   0x00000000004004c0 in inline_func<int> 
                                                   at inline-t.cc:9

Keith

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

[Keith Seitz]

On 07/18/2017 10:16 AM, Pedro Alves wrote:
>>
>> 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.
> 
> I'm still trying to grok these patches fully, but, shouldn't comparing
> the breakpoint's bp_location's addresses work the same?  I.e., with this,
> gdb.opt/inline-break.exp still passes cleanly here:

I've played with your suggestion, and I *think* I am getting close. :-)

> diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
> index 006ae0d..9120554 100644
> --- a/gdb/inline-frame.c
> +++ b/gdb/inline-frame.c
> @@ -349,23 +330,19 @@ skip_inline_frames (ptid_t ptid, struct breakpoint *bpt)
>  	      if (BLOCK_START (cur_block) == this_pc
>  		  || block_starting_point_at (this_pc, cur_block))
>  		{
> -		  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++)
> +		  if (bpt != NULL
> +		      && breakpoint_address_is_meaningful (bpt))
>  		    {
> -		      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)
> +		      for (bp_location *loc = bpt->loc;
> +			   loc != NULL;
> +			   loc = loc->next)
> +			if (this_pc == loc->address)
> +			  {
>  			    skip_this_frame = false;
> -			}
> +			    break;
> +			  }
>  		    }
>  
>  		  if (skip_this_frame)
> 

The next version of this patch does this, and it works. One small addition I had to make was to /not/ skip inline frames for non-user breakpoints. step-resume breakpoints, IIRC, were otherwise broken.

Keith

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

[Keith Seitz]

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

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

[Pedro Alves]

On 10/20/2017 08:21 PM, Keith Seitz wrote:
> 
> *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)

Can you send me these as full patches, or send me the
patch they apply on top of too?  I'll need to play with
it a bit to understand it all better.

Thanks,
Pedro Alves

Re: [PATCH 1/2] Report call site for inlined functions

[Pedro Alves]

On 10/20/2017 07:46 PM, Keith Seitz wrote:
> [My reply seems not to have made it to sourceware, so I'm resending.]
> 
> On 07/18/2017 12:05 PM, Pedro Alves wrote:
>> On 07/11/2017 03:36 AM, Keith Seitz wrote:
>>>
>>> Here you can see the new call-site-func, call-site-file, call-site-fullname,
>>> and call-site-line.
>>
>> The non-inlined call site seems sufficient info to me, at least for
>> the  CLI.  I'm not sure whether "call-site" is sufficiently clear that
>> it's referring to that vs the immediate potential inlined-too
>> caller though.  I think at least the docs need such a clarification.
> 
> There seems to be consensus on the (lack of) utility of this call-site info, so in the next revision (to follow), I've chosen to remove it all.  I think that pretty much resolves (by deletion!) most of your concerns/comments. I've also removed the inlined_symbol proposal since that is also no longer necessary. 

I think my observations in
<https://sourceware.org/ml/gdb-patches/2017-03/msg00382.html>
still make sense, and I don't know about consensus, but OK, let's
move on.  I'll take a look at the new version.

> 
>>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>>> index 0fdcd42..3b3193b 100644
>>> --- a/gdb/dwarf2read.c
>>> +++ b/gdb/dwarf2read.c
>>> @@ -11540,13 +11571,28 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>>>  	{
>>>  	  templ_func = allocate_template_symbol (objfile);
>>>  	  templ_func->base.is_cplus_template_function = 1;
>>> +	  symbol_storage = (struct symbol *) templ_func;
>>>  	  break;
>>>  	}
>>>      }
>>>  
>>> +  /* If we have an inlined symbol, we must also allocate a different
>>> +     symbol.  */
>>
>> How does this work when you have an (inlined) template function?
>> Like e.g.,:
>>
>> template<typename T>
>> static T inline_func (T val)
>> {
>>   return val * 2;
>> }
>>
>> int
>> not_inline_func (int val)
>> {
>>   return inline_func (val * 2);
>> }
>>
> 
> I realize this was asked in relation to the proposed (and now deleted) inline_symbol, but to be clear, the above example works:
> 
> (gdb) b inline_func<int>
> Breakpoint 1 at 0x4004c0: file inline-t.cc, line 9.
> (gdb) i b
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y   0x00000000004004c0 in inline_func<int> 
>                                                    at inline-t.cc:9

OK, it seems like a DIE can't be both DW_TAG_inlined_subroutine
and a template (kind of makes sense), so you couldn't ever end up
calling both allocate_template_symbol and allocate_inlined_symbol
for the same symbol (which is what I was worrying about, because
the code didn't consider/guard against that).

Thanks,
Pedro Alves

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

[Keith Seitz]

On 10/27/2017 05:37 AM, Pedro Alves wrote:
> 
> Can you send me these as full patches, or send me the
> patch they apply on top of too?  I'll need to play with
> it a bit to understand it all better.

Sure, here they are. The two are from my stgit sandbox so #2 applies on top
of #1. [That allowed me to easily flip between the two and maintain tests.]

First up is the more "complex" solution, which does /not/ move the call
to bpstat_stop_status, but instead uses (a new function) build_bpstat_chain.
This is used to get at the current breakpoint (if any) to pass to
skip_inline_frames.

---
 gdb/breakpoint.c                       | 114 ++++++++++++++++-----------------
 gdb/breakpoint.h                       |  28 +++++++-
 gdb/infrun.c                           |  11 +++-
 gdb/inline-frame.c                     |  22 ++++++-
 gdb/inline-frame.h                     |   2 +-
 gdb/testsuite/gdb.opt/inline-break.c   |  50 +++++++++++++++
 gdb/testsuite/gdb.opt/inline-break.exp |  35 ++++++++++
 7 files changed, 199 insertions(+), 63 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 10fccb8d2d..8d0a1cd2b7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5401,58 +5401,25 @@ need_moribund_for_location_type (struct bp_location *loc)
 	      && !target_supports_stopped_by_hw_breakpoint ()));
 }
 
-
-/* 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.  */
+/* See breakpoint.h.  */
 
 bpstat
-bpstat_stop_status (const address_space *aspace,
-		    CORE_ADDR bp_addr, ptid_t ptid,
+build_bpstat_chain (const address_space *aspace, CORE_ADDR bp_addr,
 		    const struct target_waitstatus *ws)
 {
-  struct breakpoint *b = NULL;
-  struct bp_location *bl;
-  struct bp_location *loc;
-  /* First item of allocated bpstat's.  */
+  struct breakpoint *b;
   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)
     {
       if (!breakpoint_enabled (b))
 	continue;
 
-      for (bl = b->loc; bl != NULL; bl = bl->next)
+      for (bp_location *bl = b->loc; bl != NULL; bl = bl->next)
 	{
 	  /* For hardware watchpoints, we look only at the first
 	     location.  The watchpoint_check function will work on the
-	     entire expression, not the individual locations.  For
+
 	     read watchpoints, the watchpoints_triggered function has
 	     checked all locations already.  */
 	  if (b->type == bp_hardware_watchpoint && bl != b->loc)
@@ -5467,8 +5434,8 @@ bpstat_stop_status (const address_space *aspace,
 	  /* Come here if it's a watchpoint, or if the break address
 	     matches.  */
 
-	  bs = new bpstats (bl, &bs_link);	/* Alloc a bpstat to
-						   explain stop.  */
+	  bpstat bs = new bpstats (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
@@ -5493,12 +5460,15 @@ bpstat_stop_status (const 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 = new bpstats (loc, &bs_link);
+	      bpstat bs = new bpstats (loc, &bs_link);
 	      /* For hits of moribund locations, we should just proceed.  */
 	      bs->stop = 0;
 	      bs->print = 0;
@@ -5507,6 +5477,49 @@ bpstat_stop_status (const address_space *aspace,
 	}
     }
 
+  return bs_head;
+}
+
+/* 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 (const address_space *aspace,
+		    CORE_ADDR bp_addr, ptid_t ptid,
+		    const struct target_waitstatus *ws,
+		    bpstat stop_chain)
+{
+  struct breakpoint *b = NULL;
+  /* First item of allocated bpstat's.  */
+  bpstat bs_head = stop_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)
+    bs_head = build_bpstat_chain (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
      unloaded libraries are correct before we handle "catch load" and
@@ -6827,22 +6840,9 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 }
 \f
 
-/* Return true iff it is meaningful to use the address member of
-   BPT locations.  For some breakpoint types, the locations' address members
-   are irrelevant and it makes no sense to attempt to compare them to other
-   addresses (or use them for any other purpose either).
-
-   More specifically, each of the following breakpoint types will
-   always have a zero valued location address and we don't want to mark
-   breakpoints of any of these types to be a duplicate of an actual
-   breakpoint location at address zero:
-
-      bp_watchpoint
-      bp_catchpoint
-
-*/
+/* See breakpoint.h.  */
 
-static int
+bool
 breakpoint_address_is_meaningful (struct breakpoint *bpt)
 {
   enum bptype type = bpt->type;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 9a9433bd66..df23d442e9 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -917,9 +917,35 @@ extern void bpstat_clear (bpstat *);
    is part of the bpstat is copied as well.  */
 extern bpstat bpstat_copy (bpstat);
 
+/* Build the bstat chain for the stop information given by ASPACE,
+   BP_ADDR, and WS.  Returns the head of the bpstat chain.  */
+
+extern bpstat build_bpstat_chain (const address_space *aspace,
+				  CORE_ADDR bp_addr,
+				  const struct target_waitstatus *ws);
+
 extern bpstat bpstat_stop_status (const address_space *aspace,
 				  CORE_ADDR pc, ptid_t ptid,
-				  const struct target_waitstatus *ws);
+				  const struct target_waitstatus *ws,
+				  bpstat stop_chain = NULL);
+
+/* Return true iff it is meaningful to use the address member of
+   BPT locations.  For some breakpoint types, the locations' address members
+   are irrelevant and it makes no sense to attempt to compare them to other
+   addresses (or use them for any other purpose either).
+
+   More specifically, each of the following breakpoint types will
+   always have a zero valued location address and we don't want to mark
+   breakpoints of any of these types to be a duplicate of an actual
+   breakpoint location at address zero:
+
+      bp_watchpoint
+      bp_catchpoint
+
+*/
+
+extern bool breakpoint_address_is_meaningful (struct breakpoint *bpt);
+
 \f
 /* This bpstat_what stuff tells wait_for_inferior what to do with a
    breakpoint (a challenging task).
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 858ffa1175..5f009334fd 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5865,6 +5865,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
@@ -5896,7 +5897,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.  */
@@ -5945,7 +5952,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.  */
diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
index b6154cdcc5..f06ecf61a6 100644
--- a/gdb/inline-frame.c
+++ b/gdb/inline-frame.c
@@ -301,7 +301,7 @@ 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;
@@ -327,7 +327,25 @@ skip_inline_frames (ptid_t ptid)
 	      if (BLOCK_START (cur_block) == this_pc
 		  || block_starting_point_at (this_pc, cur_block))
 		{
-		  skip_count++;
+		  bool skip_this_frame = true;
+
+		  if (bpt != NULL
+		      && user_breakpoint_p (bpt)
+		      && breakpoint_address_is_meaningful (bpt))
+		    {
+		      for (bp_location *loc = bpt->loc; loc != NULL;
+			   loc = loc->next)
+			{
+			  if (loc->address == this_pc)
+			    {
+			      skip_this_frame = false;
+			      break;
+			    }
+			}
+		    }
+
+		  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 e6fe29db1d..8afaf6c945 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 2616a0e3d7..a42bc0bb44 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 20bae071b3..0a990fbfd5 100644
--- a/gdb/testsuite/gdb.opt/inline-break.exp
+++ b/gdb/testsuite/gdb.opt/inline-break.exp
@@ -185,4 +185,39 @@ for {set i 1} {$i <= [array size results]} {incr i} {
     gdb_test "info break $i" $results($i)
 }
 
+# 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
-- 
2.13.6


The second "simpler" patch does move the call to bpstat_stop_status, but
since this will call bpstat_check_breakpoint_conditions, a call to
clear_inline_frame_state needed to be inserted (as previously described).

---
 gdb/breakpoint.c | 99 +++++++++++++++++++++++++-------------------------------
 gdb/breakpoint.h | 11 +------
 gdb/infrun.c     | 18 +++++------
 3 files changed, 53 insertions(+), 75 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8d0a1cd2b7..8510d4bda0 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");
 	    }
@@ -5401,25 +5403,58 @@ need_moribund_for_location_type (struct bp_location *loc)
 	      && !target_supports_stopped_by_hw_breakpoint ()));
 }
 
-/* See breakpoint.h.  */
+
+/* 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
-build_bpstat_chain (const address_space *aspace, CORE_ADDR bp_addr,
+bpstat_stop_status (const address_space *aspace,
+		    CORE_ADDR bp_addr, ptid_t ptid,
 		    const struct target_waitstatus *ws)
 {
-  struct breakpoint *b;
+  struct breakpoint *b = NULL;
+  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)
     {
       if (!breakpoint_enabled (b))
 	continue;
 
-      for (bp_location *bl = b->loc; bl != NULL; bl = bl->next)
+      for (bl = b->loc; bl != NULL; bl = bl->next)
 	{
 	  /* For hardware watchpoints, we look only at the first
 	     location.  The watchpoint_check function will work on the
-
+	     entire expression, not the individual locations.  For
 	     read watchpoints, the watchpoints_triggered function has
 	     checked all locations already.  */
 	  if (b->type == bp_hardware_watchpoint && bl != b->loc)
@@ -5434,8 +5469,8 @@ build_bpstat_chain (const address_space *aspace, CORE_ADDR bp_addr,
 	  /* Come here if it's a watchpoint, or if the break address
 	     matches.  */
 
-	  bpstat bs = new bpstats (bl, &bs_link);	/* Alloc a bpstat to
-							   explain stop.  */
+	  bs = new bpstats (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
@@ -5460,15 +5495,12 @@ build_bpstat_chain (const address_space *aspace, CORE_ADDR bp_addr,
   if (!target_supports_stopped_by_sw_breakpoint ()
       || !target_supports_stopped_by_hw_breakpoint ())
     {
-      struct bp_location *loc;
-
-      for (int ix = 0;
-	   VEC_iterate (bp_location_p, moribund_locations, ix, loc); ++ix)
+      for (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))
 	    {
-	      bpstat bs = new bpstats (loc, &bs_link);
+	      bs = new bpstats (loc, &bs_link);
 	      /* For hits of moribund locations, we should just proceed.  */
 	      bs->stop = 0;
 	      bs->print = 0;
@@ -5477,49 +5509,6 @@ build_bpstat_chain (const address_space *aspace, CORE_ADDR bp_addr,
 	}
     }
 
-  return bs_head;
-}
-
-/* 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 (const address_space *aspace,
-		    CORE_ADDR bp_addr, ptid_t ptid,
-		    const struct target_waitstatus *ws,
-		    bpstat stop_chain)
-{
-  struct breakpoint *b = NULL;
-  /* First item of allocated bpstat's.  */
-  bpstat bs_head = stop_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)
-    bs_head = build_bpstat_chain (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
      unloaded libraries are correct before we handle "catch load" and
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index df23d442e9..495b961bd3 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -917,17 +917,9 @@ extern void bpstat_clear (bpstat *);
    is part of the bpstat is copied as well.  */
 extern bpstat bpstat_copy (bpstat);
 
-/* Build the bstat chain for the stop information given by ASPACE,
-   BP_ADDR, and WS.  Returns the head of the bpstat chain.  */
-
-extern bpstat build_bpstat_chain (const address_space *aspace,
-				  CORE_ADDR bp_addr,
-				  const struct target_waitstatus *ws);
-
 extern bpstat bpstat_stop_status (const address_space *aspace,
 				  CORE_ADDR pc, ptid_t ptid,
-				  const struct target_waitstatus *ws,
-				  bpstat stop_chain = NULL);
+				  const struct target_waitstatus *ws);
 
 /* Return true iff it is meaningful to use the address member of
    BPT locations.  For some breakpoint types, the locations' address members
@@ -943,7 +935,6 @@ extern bpstat bpstat_stop_status (const address_space *aspace,
       bp_catchpoint
 
 */
-
 extern bool breakpoint_address_is_meaningful (struct breakpoint *bpt);
 
 \f
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 5f009334fd..0071f7e7b4 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5865,7 +5865,12 @@ 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;
+
+  /* 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
@@ -5899,9 +5904,8 @@ handle_signal_stop (struct execution_control_state *ecs)
 	{
 	  struct breakpoint *bpt = NULL;
 
-	  stop_chain = build_bpstat_chain (aspace, stop_pc, &ecs->ws);
-	  if (stop_chain != NULL)
-	    bpt = stop_chain->breakpoint_at;
+	  if (ecs->event_thread->control.stop_bpstat != NULL)
+	    bpt = ecs->event_thread->control.stop_bpstat->breakpoint_at;
 
 	  skip_inline_frames (ecs->ptid, bpt);
 
@@ -5948,12 +5952,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, stop_chain);
-
   /* Following in case break condition called a
      function.  */
   stop_print_frame = 1;
-- 
2.13.6

As I mentioned in my previous mail, I have a slight preference for the former
solution, but infrun is very unfamiliar to me, so I'm soliciting more advice.

Keith

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

[Pedro Alves]

Hi Keith,

On 10/30/2017 09:18 PM, Keith Seitz wrote:
> On 10/27/2017 05:37 AM, Pedro Alves wrote:
>>
>> Can you send me these as full patches, or send me the
>> patch they apply on top of too?  I'll need to play with
>> it a bit to understand it all better.
> 
> Sure, here they are. The two are from my stgit sandbox so #2 applies on top
> of #1. [That allowed me to easily flip between the two and maintain tests.]

Finally managed to play a bit with this.

> 
> First up is the more "complex" solution, which does /not/ move the call
> to bpstat_stop_status, but instead uses (a new function) build_bpstat_chain.
> This is used to get at the current breakpoint (if any) to pass to
> skip_inline_frames.

Yes, let's use this one.  The "simpler" one is too ad hoc.

>  	{
> -	  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);

I think you should pass down the stop_chain directly to
skip_inline_frames.  This is because a stop can be explained
by more than one breakpoint (that's why it's a chain), and
the user breakpoint may not be the head of the chain.

>  
>  	  /* Re-fetch current thread's frame in case that invalidated
>  	     the frame cache.  */
> @@ -5945,7 +5952,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.  */
> diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
> index b6154cdcc5..f06ecf61a6 100644
> --- a/gdb/inline-frame.c
> +++ b/gdb/inline-frame.c
> @@ -301,7 +301,7 @@ 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;
> @@ -327,7 +327,25 @@ skip_inline_frames (ptid_t ptid)
>  	      if (BLOCK_START (cur_block) == this_pc
>  		  || block_starting_point_at (this_pc, cur_block))
>  		{
> -		  skip_count++;
> +		  bool skip_this_frame = true;
> +
> +		  if (bpt != NULL
> +		      && user_breakpoint_p (bpt)
> +		      && breakpoint_address_is_meaningful (bpt))
> +		    {
> +		      for (bp_location *loc = bpt->loc; loc != NULL;
> +			   loc = loc->next)
> +			{
> +			  if (loc->address == this_pc)
> +			    {
> +			      skip_this_frame = false;
> +			      break;
> +			    }
> +			}
> +		    }

So here you'd check each breakpoint in the bpstat chain, not
just the head.

Also, to look at the locations, you should look at
bpstat->bp_location_at, not at the locations of the breakpoint,
because some of the locations of the breakpoint may be
disabled/not-inserted, for example.  There's one bpstat entry for
each _location_ that actually caused a stop, so checking bp_location_at
directly saves one loop.  (Though you'll add one loop back to walk
the bpstat chain, so it's a wash).  Careful to not
bpstat->bp_location_at->owner though, see comments in breakpoint.h.

(I think you could look at bpstat->bp_location_at->loc_type,
match against bp_loc_software_breakpoint / bp_loc_hardware_breakpoint,
instead of exposing breakpoint_address_is_meaningful.)

> +
> +		  if (skip_this_frame)
> +		    skip_count++;
>  		  last_sym = BLOCK_FUNCTION (cur_block);

Couldn't this break out of the outer loop if !skip_this_frame ?

Like:

		  if (!skip_this_frame)
		    break;

		  skip_count++;
		  last_sym = BLOCK_FUNCTION (cur_block);

?

Thanks,
Pedro Alves