[PATCH v2] Don't elide all inlined frames

[PATCH v2] Don't elide all inlined frames

[Keith Seitz]

This is v2 of (one of) my original patches to fix some inline function
breakpoint problems. The first patch dealing with "info break" has already
been committed.

What remains is the second of the two patches on which Pedro last commented
on Dec. 1. This patch deals with reporting stop locations in inlined functions.
[For more, read the commit log below.]

Changes since v1:
- Removed breakpoint_for_stop entirely
- Removed changes from bpstat_explains_signal (no longer necessary)
- Rewrote skip_inline_frames to walk stop chain and locations
  (no more calling decode_line_full)
- Updated all (code) documentation
- Updated new gdb.ada/bp_inlined_func.exp test (test originally stopped
  in calling frame)
- Updated gdb.dwarf2/implptr.exp (same reason as bp_inlined_func.exp)

Keith

----------

This patch essentially causes GDB to treat inlined frames like "normal"
frames from the user's perspective.  This means, for example, that when a
user hits a breakpoint in an inlined function, GDB will now actually stop
"in" that function.

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

Because skip_inline_frames currently skips every inlined frame, GDB "stops"
in the caller.  This patch adds a new parameter to skip_inline_frames
that allows us to pass in a bpstat stop chain.  The breakpoint locations
on the stop chain can be used to determine if we've stopped inside an inline
function (due to a user breakpoint).  If we have, we do not elide the frame.

With this patch, GDB now 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;

My thanks to Jan and Pedro for guidance on this.

gdb/ChangeLog:

	* breakpoint.c (build_bpstat_chain): New function, moved from
	bpstat_stop_status.
	(bpstat_stop_status): Add optional parameter, `stop_chain'.
	If no stop chain is passed, call build_bpstat_chain to build it.
	* breakpoint.h (build_bpstat_chain): Declare.
	(bpstat_stop_status): Move documentation here from breakpoint.c.
	* infrun.c (handle_signal_stop): Before eliding inlined frames,
	build the stop chain and pass it to skip_inline_frames.
	Pass this stop chain to bpstat_stop_status.
	* inline-frame.c: Include breakpoint.h.
	(skip_inline_frames): Add parameter `stop_chain'.
	Move documention to inline-frame.h.
	If non-NULL, walk all locations to determine if the inlined
	frame caused the stop.  If it did, do not elide it.
	* inline-frame.h (skip_inline_frames): Add parameter `stop-chain'.
	Add moved documentation and update for new parameter.

gdb/testsuite/ChangeLog:

	* gdb.ada/bp_inlined_func.exp: Update inlined frame locations
	in expected breakpoint stop locations.
	* gdb.dwarf2/implptr.exp (implptr_test_baz): Use up/down to
	move to proper scope to test variable values.
	* 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                          | 79 +++++++++++++++----------------
 gdb/breakpoint.h                          | 30 +++++++++++-
 gdb/infrun.c                              |  6 ++-
 gdb/inline-frame.c                        | 37 +++++++++++++--
 gdb/inline-frame.h                        | 11 +++--
 gdb/testsuite/gdb.ada/bp_inlined_func.exp | 20 ++------
 gdb/testsuite/gdb.dwarf2/implptr.exp      |  6 ++-
 gdb/testsuite/gdb.opt/inline-break.c      | 50 +++++++++++++++++++
 gdb/testsuite/gdb.opt/inline-break.exp    | 35 ++++++++++++++
 9 files changed, 207 insertions(+), 67 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index c56084cce3..c4d800ceca 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5342,54 +5342,21 @@ 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
@@ -5408,8 +5375,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
@@ -5434,12 +5401,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)
+      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;
@@ -5448,6 +5418,33 @@ bpstat_stop_status (const address_space *aspace,
 	}
     }
 
+  return bs_head;
+}
+
+/* See breakpoint.h.  */
+
+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 8bb81d8d17..068c798b9d 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -917,9 +917,37 @@ extern void bpstat_clear (bpstat *);
    is part of the bpstat is copied as well.  */
 extern bpstat bpstat_copy (bpstat);
 
+/* Build the (raw) bpstat 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);
+
+/* Get a bpstat associated with having just stopped at address
+   BP_ADDR in thread PTID.  STOP_CHAIN may be supplied as a previously
+   computed stop chain or NULL, in which case the stop chain will be
+   computed using build_bpstat_chain.
+
+   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.  */
+
 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);
 \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 c663908568..f96c3c0374 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5862,6 +5862,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
@@ -5893,7 +5894,8 @@ handle_signal_stop (struct execution_control_state *ecs)
 					     ecs->event_thread->prev_pc,
 					     &ecs->ws)))
 	{
-	  skip_inline_frames (ecs->ptid);
+	  stop_chain = build_bpstat_chain (aspace, stop_pc, &ecs->ws);
+	  skip_inline_frames (ecs->ptid, stop_chain);
 
 	  /* Re-fetch current thread's frame in case that invalidated
 	     the frame cache.  */
@@ -5942,7 +5944,7 @@ handle_signal_stop (struct execution_control_state *ecs)
      handles this event.  */
   ecs->event_thread->control.stop_bpstat
     = bpstat_stop_status (get_current_regcache ()->aspace (),
-			  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 a019619559..68467d0440 100644
--- a/gdb/inline-frame.c
+++ b/gdb/inline-frame.c
@@ -18,6 +18,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "breakpoint.h"
 #include "inline-frame.h"
 #include "addrmap.h"
 #include "block.h"
@@ -296,12 +297,10 @@ block_starting_point_at (CORE_ADDR pc, const struct block *block)
   return 1;
 }
 
-/* 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.  */
+/* See inline-frame.h.  */
 
 void
-skip_inline_frames (ptid_t ptid)
+skip_inline_frames (ptid_t ptid, bpstat stop_chain)
 {
   CORE_ADDR this_pc;
   const struct block *frame_block, *cur_block;
@@ -327,6 +326,36 @@ skip_inline_frames (ptid_t ptid)
 	      if (BLOCK_START (cur_block) == this_pc
 		  || block_starting_point_at (this_pc, cur_block))
 		{
+		  bool skip_this_frame = true;
+
+		  /* Loop over the stop chain and determine if execution
+		     stopped in an inlined frame because of a user breakpoint.
+		     If so do not skip the inlined frame.  */
+		  for (bpstat s = stop_chain; s != NULL; s = s->next)
+		    {
+		      struct breakpoint *bpt = s->breakpoint_at;
+
+		      if (bpt != NULL && user_breakpoint_p (bpt))
+			{
+			  for (bp_location *loc = s->bp_location_at;
+			       loc != NULL; loc = loc->next)
+			    {
+			      enum bp_loc_type t = loc->loc_type;
+
+			      if (loc->address == this_pc
+				  && (t == bp_loc_software_breakpoint
+				      || t == bp_loc_hardware_breakpoint))
+				{
+				  skip_this_frame = false;
+				  break;
+				}
+			    }
+			}
+		    }
+
+		  if (!skip_this_frame)
+		    break;
+
 		  skip_count++;
 		  last_sym = BLOCK_FUNCTION (cur_block);
 		}
diff --git a/gdb/inline-frame.h b/gdb/inline-frame.h
index 1d2e251cb1..d66ad44b7c 100644
--- a/gdb/inline-frame.h
+++ b/gdb/inline-frame.h
@@ -22,16 +22,21 @@
 
 struct frame_info;
 struct frame_unwind;
+struct bpstats;
 
 /* The inline frame unwinder.  */
 
 extern const struct frame_unwind inline_frame_unwind;
 
 /* 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.  */
 
-void skip_inline_frames (ptid_t ptid);
+   If non-NULL, STOP_CHAIN is used to determine whether a stop was caused by
+   a user breakpoint.  In that case, do not skip that inlined frame.  This
+   allows the inlined frame to be treated as if it were non-inlined from the
+   user's perspective.  GDB will stop "in" the inlined frame instead of
+   the caller.  */
+
+void skip_inline_frames (ptid_t ptid, struct bpstats *stop_chain);
 
 /* 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.ada/bp_inlined_func.exp b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
index 37ef6af8ab..c220df7d33 100644
--- a/gdb/testsuite/gdb.ada/bp_inlined_func.exp
+++ b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
@@ -38,21 +38,11 @@ gdb_test "break read_small" \
 # We do not verify each breakpoint info, but use continue commands instead
 # to verify that we properly stop on each expected breakpoint.
 
-gdb_test "continue" \
-         "Breakpoint $decimal, b\\.doit \\(\\).*" \
-         "Hitting first call of read_small"
-
-gdb_test "continue" \
-         "Breakpoint $decimal, foo \\(\\).*" \
-         "Hitting second call of read_small"
-
-gdb_test "continue" \
-         "Breakpoint $decimal, c\\.c_doit \\(\\).*" \
-         "Hitting third call of read_small"
-
-gdb_test "continue" \
-         "Breakpoint $decimal, c\\.c_doit2 \\(\\).*" \
-         "Hitting fourth call of read_small"
+for {set i 0} {$i < 4} {incr i} {
+    gdb_test "continue" \
+	"Breakpoint $decimal, b\\.read_small \\(\\).*" \
+	"Stopped in read_small ($i)"
+}
 
 gdb_test "continue" \
          "Continuing\..*$inferior_exited_re.*" \
diff --git a/gdb/testsuite/gdb.dwarf2/implptr.exp b/gdb/testsuite/gdb.dwarf2/implptr.exp
index 890598c9ff..92ca6d10e7 100644
--- a/gdb/testsuite/gdb.dwarf2/implptr.exp
+++ b/gdb/testsuite/gdb.dwarf2/implptr.exp
@@ -66,9 +66,13 @@ proc implptr_test_baz {} {
     gdb_test "break implptr.c:$line" "Breakpoint 3.*" \
 	"set baz breakpoint for implptr"
     gdb_continue_to_breakpoint "continue to baz breakpoint for implptr"
+
+    # We are breaking in an inlined function.  GDB used to stop in the
+    # calling frame, but it now stops "in" the inlined function.
+    gdb_test "up" "#1  foo .*"
     gdb_test {p p[0].y} " = 92" "sanity check element 0"
     gdb_test {p p[1].y} " = 46" "sanity check element 1"
-    gdb_test "step" "\r\nadd \\(.*" "enter the inlined function"
+    gdb_test "down" "#0  add .*"
     gdb_test "p a->y" " = 92" "check element 0 for the offset"
     gdb_test "p b->y" " = 46" "check element 1 for the offset"
     gdb_continue_to_breakpoint "ignore the second baz breakpoint"
diff --git a/gdb/testsuite/gdb.opt/inline-break.c b/gdb/testsuite/gdb.opt/inline-break.c
index c3d633810a..922102debb 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 c6b037d153..cc8d4bd53d 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

Re: [PATCH v2] Don't elide all inlined frames

[Pedro Alves]

Hi Keith,

Somehow I thought I had already replied to this, but apparently
not.  Here it goes.  This is close, but I'm still not clear why the
skip_inline_frames loop is how it currently is.  See more below.

On 02/28/2018 08:33 PM, Keith Seitz wrote:

>  
>  void
> -skip_inline_frames (ptid_t ptid)
> +skip_inline_frames (ptid_t ptid, bpstat stop_chain)
>  {
>    CORE_ADDR this_pc;
>    const struct block *frame_block, *cur_block;
> @@ -327,6 +326,36 @@ skip_inline_frames (ptid_t ptid)
>  	      if (BLOCK_START (cur_block) == this_pc
>  		  || block_starting_point_at (this_pc, cur_block))
>  		{
> +		  bool skip_this_frame = true;
> +
> +		  /* Loop over the stop chain and determine if execution
> +		     stopped in an inlined frame because of a user breakpoint.
> +		     If so do not skip the inlined frame.  */
> +		  for (bpstat s = stop_chain; s != NULL; s = s->next)
> +		    {
> +		      struct breakpoint *bpt = s->breakpoint_at;
> +
> +		      if (bpt != NULL && user_breakpoint_p (bpt))
> +			{
> +			  for (bp_location *loc = s->bp_location_at;
> +			       loc != NULL; loc = loc->next)
> +			    {
> +			      enum bp_loc_type t = loc->loc_type;
> +
> +			      if (loc->address == this_pc
> +				  && (t == bp_loc_software_breakpoint
> +				      || t == bp_loc_hardware_breakpoint))
> +				{
> +				  skip_this_frame = false;
> +				  break;
> +				}
> +			    }
> +			}
> +		    }
> +
> +		  if (!skip_this_frame)
> +		    break;
> +

It seems my comments from last time still apply here:

 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 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
 [follow] bpstat->bp_location_at->owner though, see comments in breakpoint.h.
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Had you tried that and it didn't work for some reason?

Also, this part of the comment:

 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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

 ?
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The part about breakint out of the outer loop no longer
applies as is, but, AFAICT, the current code is still letting the
outer stop_chain loop continue iterating even 'if (!skip_this_frame)'?


> -gdb_test "continue" \
> -         "Breakpoint $decimal, c\\.c_doit2 \\(\\).*" \
> -         "Hitting fourth call of read_small"
> +for {set i 0} {$i < 4} {incr i} {
> +    gdb_test "continue" \
> +	"Breakpoint $decimal, b\\.read_small \\(\\).*" \
> +	"Stopped in read_small ($i)"

Let's avoid the trailing " (...)" in test messages/names:

 https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

How about for e.g.,:

 "stopped in read_small, $i"

or you can use with_test_prefix, foreach_with_prefix, etc.

> --- a/gdb/testsuite/gdb.dwarf2/implptr.exp
> +++ b/gdb/testsuite/gdb.dwarf2/implptr.exp
> @@ -66,9 +66,13 @@ proc implptr_test_baz {} {
>      gdb_test "break implptr.c:$line" "Breakpoint 3.*" \
>  	"set baz breakpoint for implptr"
>      gdb_continue_to_breakpoint "continue to baz breakpoint for implptr"
> +
> +    # We are breaking in an inlined function.  GDB used to stop in the
> +    # calling frame, but it now stops "in" the inlined function.
> +    gdb_test "up" "#1  foo .*"

I'd suggest avoiding talking about how GDB used to work, because IME that
tends to stay behind for years and years and become more noise
than signal.  I'd suggest something like:

    # We are breaking in an inlined function.  GDB should have stopped
    # "in" the inlined function, not the calling frame.
    gdb_test "up" "#1  foo .*"

>      gdb_test {p p[0].y} " = 92" "sanity check element 0"
>      gdb_test {p p[1].y} " = 46" "sanity check element 1"
> -    gdb_test "step" "\r\nadd \\(.*" "enter the inlined function"
> +    gdb_test "down" "#0  add .*"
>      gdb_test "p a->y" " = 92" "check element 0 for the offset"
>      gdb_test "p b->y" " = 46" "check element 1 for the offset"
>      gdb_continue_to_breakpoint "ignore the second baz breakpoint"
> diff --git a/gdb/testsuite/gdb.opt/inline-break.c b/gdb/testsuite/gdb.opt/inline-break.c


> +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;"

Avoid the / before $srcfile for remote host testing.

Thanks,
Pedro Alves

Re: [PATCH v2] Don't elide all inlined frames

[Keith Seitz]

On 04/09/2018 05:25 AM, Pedro Alves wrote:
> On 02/28/2018 08:33 PM, Keith Seitz wrote:
> 
>>  
>>  void
>> -skip_inline_frames (ptid_t ptid)
>> +skip_inline_frames (ptid_t ptid, bpstat stop_chain)
>>  {
>>    CORE_ADDR this_pc;
>>    const struct block *frame_block, *cur_block;
>> @@ -327,6 +326,36 @@ skip_inline_frames (ptid_t ptid)
>>  	      if (BLOCK_START (cur_block) == this_pc
>>  		  || block_starting_point_at (this_pc, cur_block))
>>  		{
>> +		  bool skip_this_frame = true;
>> +
>> +		  /* Loop over the stop chain and determine if execution
>> +		     stopped in an inlined frame because of a user breakpoint.
>> +		     If so do not skip the inlined frame.  */
>> +		  for (bpstat s = stop_chain; s != NULL; s = s->next)
>> +		    {
>> +		      struct breakpoint *bpt = s->breakpoint_at;
>> +
>> +		      if (bpt != NULL && user_breakpoint_p (bpt))
>> +			{
>> +			  for (bp_location *loc = s->bp_location_at;
>> +			       loc != NULL; loc = loc->next)
>> +			    {
>> +			      enum bp_loc_type t = loc->loc_type;
>> +
>> +			      if (loc->address == this_pc
>> +				  && (t == bp_loc_software_breakpoint
>> +				      || t == bp_loc_hardware_breakpoint))
>> +				{
>> +				  skip_this_frame = false;
>> +				  break;
>> +				}
>> +			    }
>> +			}
>> +		    }
>> +
>> +		  if (!skip_this_frame)
>> +		    break;
>> +
> 
> It seems my comments from last time still apply here:
> 
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  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
>  [follow] bpstat->bp_location_at->owner though, see comments in breakpoint.h.
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 

I'm sorry, I don't follow. The locations are gotten from `s' which is from the bpstat chain. Aside from the breakpoint's type (and whether it is a user breakpoint), the actual breakpoint is not used.

However, the inner for-loop (over the location->next) here can/should be removed. I've changed that.

Otherwise, I'm afraid I just don't understand what you're saying. You might have to hit me with the clue stick. Sorry.

Here's the loop in the current version:

                   for (bpstat s = stop_chain; s != NULL; s = s->next)
                    {
                      struct breakpoint *bpt = s->breakpoint_at;

                      if (bpt != NULL && user_breakpoint_p (bpt))
                        {
                          bp_location *loc = s->bp_location_at;
                          enum bp_loc_type t = loc->loc_type;

                          if (loc->address == this_pc
                              && (t == bp_loc_software_breakpoint
                                  || t == bp_loc_hardware_breakpoint))
                            {
                              skip_this_frame = false;
                              break;
                            }
                        }
                    }

> Also, this part of the comment:
> 
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
>  > +
>  > +		  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);
> 
>  ?
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> The part about breakint out of the outer loop no longer
> applies as is, but, AFAICT, the current code is still letting the
> outer stop_chain loop continue iterating even 'if (!skip_this_frame)'?

The quoted code above (yours) is exactly the code in the patch. Did I send the wrong patch?

>> -gdb_test "continue" \
>> -         "Breakpoint $decimal, c\\.c_doit2 \\(\\).*" \
>> -         "Hitting fourth call of read_small"
>> +for {set i 0} {$i < 4} {incr i} {
>> +    gdb_test "continue" \
>> +	"Breakpoint $decimal, b\\.read_small \\(\\).*" \
>> +	"Stopped in read_small ($i)"
> 
> Let's avoid the trailing " (...)" in test messages/names:
> 
>  https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

Fixed.

>> --- a/gdb/testsuite/gdb.dwarf2/implptr.exp
>> +++ b/gdb/testsuite/gdb.dwarf2/implptr.exp
>> @@ -66,9 +66,13 @@ proc implptr_test_baz {} {
>>      gdb_test "break implptr.c:$line" "Breakpoint 3.*" \
>>  	"set baz breakpoint for implptr"
>>      gdb_continue_to_breakpoint "continue to baz breakpoint for implptr"
>> +
>> +    # We are breaking in an inlined function.  GDB used to stop in the
>> +    # calling frame, but it now stops "in" the inlined function.
>> +    gdb_test "up" "#1  foo .*"
> 
> I'd suggest avoiding talking about how GDB used to work, because IME that
> tends to stay behind for years and years and become more noise
> than signal.  I'd suggest something like
> 
>     # We are breaking in an inlined function.  GDB should have stopped
>     # "in" the inlined function, not the calling frame.
>     gdb_test "up" "#1  foo .*"
> 

Fixed.

>> diff --git a/gdb/testsuite/gdb.opt/inline-break.c b/gdb/testsuite/gdb.opt/inline-break.c
> 
> 
>> +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;"
> 
> Avoid the / before $srcfile for remote host testing.

Fixed.

For ease of discussion, these revisions appear below (hopefully sans editor intervention),

Keith

-----

This patch essentially causes GDB to treat inlined frames like "normal"
frames from the user's perspective.  This means, for example, that when a
user sets a breakpoint in an inlined function, GDB will now actually stop
"in" that function.

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

Because skip_inline_frames currently skips every inlined frame, GDB "stops"
in the caller.  This patch adds a new parameter to skip_inline_frames
that allows us to pass in a bpstat stop chain.  The breakpoint locations
on the stop chain can be used to determine if we've stopped inside an inline
function (due to a user breakpoint).  If we have, we do not elide the frame.

With this patch, GDB now 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;

Many thanks to Jan and Pedro for guidance on this.

gdb/ChangeLog:

	* breakpoint.c (build_bpstat_chain): New function, moved from
	bpstat_stop_status.
	(bpstat_stop_status): Add optional parameter, `stop_chain'.
	If no stop chain is passed, call build_bpstat_chain to build it.
	* breakpoint.h (build_bpstat_chain): Declare.
	(bpstat_stop_status): Move documentation here from breakpoint.c.
	* infrun.c (handle_signal_stop): Before eliding inlined frames,
	build the stop chain and pass it to skip_inline_frames.
	Pass this stop chain to bpstat_stop_status.
	* inline-frame.c: Include breakpoint.h.
	(skip_inline_frames): Add parameter `stop_chain'.
	Move documention to inline-frame.h.
	If non-NULL, walk all locations to determine if the inlined
	frame caused the stop.  If it did, do not elide it.
	* inline-frame.h (skip_inline_frames): Add parameter `stop-chain'.
	Add moved documentation and update for new parameter.

gdb/testsuite/ChangeLog:

	* gdb.ada/bp_inlined_func.exp: Update inlined frame locations
	in expected breakpoint stop locations.
	* gdb.dwarf2/implptr.exp (implptr_test_baz): Use up/down to
	move to proper scope to test variable values.
	* 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                          | 79 +++++++++++++++----------------
 gdb/breakpoint.h                          | 30 +++++++++++-
 gdb/infrun.c                              |  6 ++-
 gdb/inline-frame.c                        | 34 +++++++++++--
 gdb/inline-frame.h                        | 11 +++--
 gdb/testsuite/gdb.ada/bp_inlined_func.exp | 22 +++------
 gdb/testsuite/gdb.dwarf2/implptr.exp      |  6 ++-
 gdb/testsuite/gdb.opt/inline-break.c      | 50 +++++++++++++++++++
 gdb/testsuite/gdb.opt/inline-break.exp    | 35 ++++++++++++++
 9 files changed, 206 insertions(+), 67 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 779240b8f0..e8e08ac92b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5312,54 +5312,21 @@ 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
@@ -5378,8 +5345,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
@@ -5404,12 +5371,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)
+      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;
@@ -5418,6 +5388,33 @@ bpstat_stop_status (const address_space *aspace,
 	}
     }
 
+  return bs_head;
+}
+
+/* See breakpoint.h.  */
+
+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 1b045ae092..400620f885 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -924,9 +924,37 @@ extern void bpstat_clear (bpstat *);
    is part of the bpstat is copied as well.  */
 extern bpstat bpstat_copy (bpstat);
 
+/* Build the (raw) bpstat 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);
+
+/* Get a bpstat associated with having just stopped at address
+   BP_ADDR in thread PTID.  STOP_CHAIN may be supplied as a previously
+   computed stop chain or NULL, in which case the stop chain will be
+   computed using build_bpstat_chain.
+
+   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.  */
+
 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);
 \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 cc12e6910f..df19478ef3 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5862,6 +5862,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
@@ -5893,7 +5894,8 @@ handle_signal_stop (struct execution_control_state *ecs)
 					     ecs->event_thread->prev_pc,
 					     &ecs->ws)))
 	{
-	  skip_inline_frames (ecs->ptid);
+	  stop_chain = build_bpstat_chain (aspace, stop_pc, &ecs->ws);
+	  skip_inline_frames (ecs->ptid, stop_chain);
 
 	  /* Re-fetch current thread's frame in case that invalidated
 	     the frame cache.  */
@@ -5942,7 +5944,7 @@ handle_signal_stop (struct execution_control_state *ecs)
      handles this event.  */
   ecs->event_thread->control.stop_bpstat
     = bpstat_stop_status (get_current_regcache ()->aspace (),
-			  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 2f701e7bce..5e32de9d0b 100644
--- a/gdb/inline-frame.c
+++ b/gdb/inline-frame.c
@@ -18,6 +18,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "breakpoint.h"
 #include "inline-frame.h"
 #include "addrmap.h"
 #include "block.h"
@@ -284,12 +285,10 @@ block_starting_point_at (CORE_ADDR pc, const struct block *block)
   return 1;
 }
 
-/* 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.  */
+/* See inline-frame.h.  */
 
 void
-skip_inline_frames (ptid_t ptid)
+skip_inline_frames (ptid_t ptid, bpstat stop_chain)
 {
   const struct block *frame_block, *cur_block;
   struct symbol *last_sym = NULL;
@@ -313,6 +312,33 @@ skip_inline_frames (ptid_t ptid)
 	      if (BLOCK_START (cur_block) == this_pc
 		  || block_starting_point_at (this_pc, cur_block))
 		{
+		  bool skip_this_frame = true;
+
+		  /* Loop over the stop chain and determine if execution
+		     stopped in an inlined frame because of a user breakpoint.
+		     If so do not skip the inlined frame.  */
+		  for (bpstat s = stop_chain; s != NULL; s = s->next)
+		    {
+		      struct breakpoint *bpt = s->breakpoint_at;
+
+		      if (bpt != NULL && user_breakpoint_p (bpt))
+			{
+			  bp_location *loc = s->bp_location_at;
+			  enum bp_loc_type t = loc->loc_type;
+
+			  if (loc->address == this_pc
+			      && (t == bp_loc_software_breakpoint
+				  || t == bp_loc_hardware_breakpoint))
+			    {
+			      skip_this_frame = false;
+			      break;
+			    }
+			}
+		    }
+
+		  if (!skip_this_frame)
+		    break;
+
 		  skip_count++;
 		  last_sym = BLOCK_FUNCTION (cur_block);
 		}
diff --git a/gdb/inline-frame.h b/gdb/inline-frame.h
index 1d2e251cb1..d66ad44b7c 100644
--- a/gdb/inline-frame.h
+++ b/gdb/inline-frame.h
@@ -22,16 +22,21 @@
 
 struct frame_info;
 struct frame_unwind;
+struct bpstats;
 
 /* The inline frame unwinder.  */
 
 extern const struct frame_unwind inline_frame_unwind;
 
 /* 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.  */
 
-void skip_inline_frames (ptid_t ptid);
+   If non-NULL, STOP_CHAIN is used to determine whether a stop was caused by
+   a user breakpoint.  In that case, do not skip that inlined frame.  This
+   allows the inlined frame to be treated as if it were non-inlined from the
+   user's perspective.  GDB will stop "in" the inlined frame instead of
+   the caller.  */
+
+void skip_inline_frames (ptid_t ptid, struct bpstats *stop_chain);
 
 /* 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.ada/bp_inlined_func.exp b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
index 37ef6af8ab..0f615f5d9b 100644
--- a/gdb/testsuite/gdb.ada/bp_inlined_func.exp
+++ b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
@@ -38,21 +38,13 @@ gdb_test "break read_small" \
 # We do not verify each breakpoint info, but use continue commands instead
 # to verify that we properly stop on each expected breakpoint.
 
-gdb_test "continue" \
-         "Breakpoint $decimal, b\\.doit \\(\\).*" \
-         "Hitting first call of read_small"
-
-gdb_test "continue" \
-         "Breakpoint $decimal, foo \\(\\).*" \
-         "Hitting second call of read_small"
-
-gdb_test "continue" \
-         "Breakpoint $decimal, c\\.c_doit \\(\\).*" \
-         "Hitting third call of read_small"
-
-gdb_test "continue" \
-         "Breakpoint $decimal, c\\.c_doit2 \\(\\).*" \
-         "Hitting fourth call of read_small"
+for {set i 0} {$i < 4} {incr i} {
+    with_test_prefix "iteration $i" {
+	gdb_test "continue" \
+	    "Breakpoint $decimal, b\\.read_small \\(\\).*" \
+	    "stopped in read_small"
+    }
+}
 
 gdb_test "continue" \
          "Continuing\..*$inferior_exited_re.*" \
diff --git a/gdb/testsuite/gdb.dwarf2/implptr.exp b/gdb/testsuite/gdb.dwarf2/implptr.exp
index 890598c9ff..ab9c5276ed 100644
--- a/gdb/testsuite/gdb.dwarf2/implptr.exp
+++ b/gdb/testsuite/gdb.dwarf2/implptr.exp
@@ -66,9 +66,13 @@ proc implptr_test_baz {} {
     gdb_test "break implptr.c:$line" "Breakpoint 3.*" \
 	"set baz breakpoint for implptr"
     gdb_continue_to_breakpoint "continue to baz breakpoint for implptr"
+
+    # We are breaking in an inlined function.  GDB should appear to
+    # have stopped "in" the inlined function.
+    gdb_test "up" "#1  foo .*"
     gdb_test {p p[0].y} " = 92" "sanity check element 0"
     gdb_test {p p[1].y} " = 46" "sanity check element 1"
-    gdb_test "step" "\r\nadd \\(.*" "enter the inlined function"
+    gdb_test "down" "#0  add .*"
     gdb_test "p a->y" " = 92" "check element 0 for the offset"
     gdb_test "p b->y" " = 46" "check element 1 for the offset"
     gdb_continue_to_breakpoint "ignore the second baz breakpoint"
diff --git a/gdb/testsuite/gdb.opt/inline-break.c b/gdb/testsuite/gdb.opt/inline-break.c
index c3d633810a..922102debb 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 c6b037d153..7c21422fad 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

Re: [PATCH v2] Don't elide all inlined frames

[Pedro Alves]

On 05/09/2018 08:17 PM, Keith Seitz wrote:
> On 04/09/2018 05:25 AM, Pedro Alves wrote:


> I'm sorry, I don't follow. The locations are gotten from `s' which is from the bpstat chain. Aside from the breakpoint's type (and whether it is a user breakpoint), the actual breakpoint is not used.

You're right, sorry about that.

> 
> However, the inner for-loop (over the location->next) here can/should be removed. I've changed that.
> 
> Otherwise, I'm afraid I just don't understand what you're saying. You might have to hit me with the clue stick. Sorry.

I was thinking of this comment:

    /* Breakpoint that caused the stop.  This is nullified if the
       breakpoint ends up being deleted.  See comments on
       `bp_location_at' above for why do we need this field instead of
       following the location's owner.  */
    struct breakpoint *breakpoint_at;

and noticed that the code uses "s->breakpoint_at" and somehow misread
it as still following the breakpoint's location list.

>> The part about breakint out of the outer loop no longer
>> applies as is, but, AFAICT, the current code is still letting the
>> outer stop_chain loop continue iterating even 'if (!skip_this_frame)'?
> 
> The quoted code above (yours) is exactly the code in the patch. Did I send the wrong patch?

The point is that the code still continues iterating this middle
loop unnecessarily:

		  for (bpstat s = stop_chain; s != NULL; s = s->next)
		    {

I think the easiest and clearest way to address that is to move
those loops to a separate function, like this:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
From be6bc201c84fcbe94783bdf08557658b824df0d0 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 11 May 2018 16:22:09 +0100
Subject: [PATCH] split

---
 gdb/inline-frame.c | 65 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
index 68467d04406..3e258423099 100644
--- a/gdb/inline-frame.c
+++ b/gdb/inline-frame.c
@@ -297,6 +297,35 @@ block_starting_point_at (CORE_ADDR pc, const struct block *block)
   return 1;
 }
 
+/* Loop over the stop chain and determine if execution stopped in an
+   inlined frame because of a user breakpoint.  THIS_PC is the current
+   frame's PC.  */
+
+static bool
+stopped_by_user_bp_inline_frame (CORE_ADDR this_pc, bpstat stop_chain)
+{
+  for (bpstat s = stop_chain; s != NULL; s = s->next)
+    {
+      struct breakpoint *bpt = s->breakpoint_at;
+
+      if (bpt != NULL && user_breakpoint_p (bpt))
+	{
+	  for (bp_location *loc = s->bp_location_at;
+	       loc != NULL; loc = loc->next)
+	    {
+	      enum bp_loc_type t = loc->loc_type;
+
+	      if (loc->address == this_pc
+		  && (t == bp_loc_software_breakpoint
+		      || t == bp_loc_hardware_breakpoint))
+		return true;
+	    }
+	}
+    }
+
+  return false;
+}
+
 /* See inline-frame.h.  */
 
 void
@@ -326,38 +355,14 @@ skip_inline_frames (ptid_t ptid, bpstat stop_chain)
 	      if (BLOCK_START (cur_block) == this_pc
 		  || block_starting_point_at (this_pc, cur_block))
 		{
-		  bool skip_this_frame = true;
-
-		  /* Loop over the stop chain and determine if execution
-		     stopped in an inlined frame because of a user breakpoint.
-		     If so do not skip the inlined frame.  */
-		  for (bpstat s = stop_chain; s != NULL; s = s->next)
+		  /* Do not skip the inlined frame if execution
+		     stopped in an inlined frame because of a user
+		     breakpoint.  */
+		  if (!stopped_by_user_bp_inline_frame (this_pc, stop_chain))
 		    {
-		      struct breakpoint *bpt = s->breakpoint_at;
-
-		      if (bpt != NULL && user_breakpoint_p (bpt))
-			{
-			  for (bp_location *loc = s->bp_location_at;
-			       loc != NULL; loc = loc->next)
-			    {
-			      enum bp_loc_type t = loc->loc_type;
-
-			      if (loc->address == this_pc
-				  && (t == bp_loc_software_breakpoint
-				      || t == bp_loc_hardware_breakpoint))
-				{
-				  skip_this_frame = false;
-				  break;
-				}
-			    }
-			}
+		      skip_count++;
+		      last_sym = BLOCK_FUNCTION (cur_block);
 		    }
-
-		  if (!skip_this_frame)
-		    break;
-
-		  skip_count++;
-		  last_sym = BLOCK_FUNCTION (cur_block);
 		}
 	      else
 		break;
-- 
2.14.3
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


> +# 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
> +    }
> +}

So that discussion above about "s->breakpoint_at" made me think that
it'd be good to also test all this with "tbreak", in case this frame
skipping code ends up moving around and ends up after the breakpoint
is deleted.

Could you add that to the test?  I'm thinking that you'd basically
add a new outer loop around the new tests, like:

# Also test "tbreak" to make sure that we display the stop in
# the inline frame even the breakpoint is immediately deleted.
foreach_with_prefix cmd "break" "tbreak" {
   #restart
   #test
}

Thanks,
Pedro Alves

Re: [PATCH v2] Don't elide all inlined frames

[Keith Seitz]

On 05/11/2018 08:55 AM, Pedro Alves wrote:
> I was thinking of this comment:
> 
>     /* Breakpoint that caused the stop.  This is nullified if the
>        breakpoint ends up being deleted.  See comments on
>        `bp_location_at' above for why do we need this field instead of
>        following the location's owner.  */
>     struct breakpoint *breakpoint_at;
> 
> and noticed that the code uses "s->breakpoint_at" and somehow misread
> it as still following the breakpoint's location list.

Ah, yes, okay, your comments make much more sense now. ;-)

> I think the easiest and clearest way to address that is to move
> those loops to a separate function, like this:
> 
> diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
> index 68467d04406..3e258423099 100644
> --- a/gdb/inline-frame.c
> +++ b/gdb/inline-frame.c
> @@ -297,6 +297,35 @@ block_starting_point_at (CORE_ADDR pc, const struct block *block)
>    return 1;
>  }
>  
> +/* Loop over the stop chain and determine if execution stopped in an
> +   inlined frame because of a user breakpoint.  THIS_PC is the current
> +   frame's PC.  */
> +
> +static bool
> +stopped_by_user_bp_inline_frame (CORE_ADDR this_pc, bpstat stop_chain)
> +{

[snip]

Yeah, that's a nice clean-up anyway which makes the intent a lot easier to read in skip_inline_frames. I'll just "borrow" that.

Here's the next revision, which includes the recommended changes (at least I think it does) and tbreak tests. Only changes from previous versions are in gdb.opt/inline-break.exp and inline-frames.c, but I'm including everything for reference.

Keith

gdb/ChangeLog:

	* breakpoint.c (build_bpstat_chain): New function, moved from
	bpstat_stop_status.
	(bpstat_stop_status): Add optional parameter, `stop_chain'.
	If no stop chain is passed, call build_bpstat_chain to build it.
	* breakpoint.h (build_bpstat_chain): Declare.
	(bpstat_stop_status): Move documentation here from breakpoint.c.
	* infrun.c (handle_signal_stop): Before eliding inlined frames,
	build the stop chain and pass it to skip_inline_frames.
	Pass this stop chain to bpstat_stop_status.
	* inline-frame.c: Include breakpoint.h.
	(stopped_by_user_bp_inline_frame): New function.
	(skip_inline_frames): Add parameter `stop_chain'.
	Move documention to inline-frame.h.
	If non-NULL, use stopped_by_user_bp_inline_frame to determine
	whether the frame should be elided.
	* inline-frame.h (skip_inline_frames): Add parameter `stop_chain'.
	Add moved documentation and update for new parameter.

gdb/testsuite/ChangeLog:

	* gdb.ada/bp_inlined_func.exp: Update inlined frame locations
	in expected breakpoint stop locations.
	* gdb.dwarf2/implptr.exp (implptr_test_baz): Use up/down to
	move to proper scope to test variable values.
	* 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. Repeat tests for temporary breakpoints.
---
 gdb/breakpoint.c                          | 79 +++++++++++++++----------------
 gdb/breakpoint.h                          | 30 +++++++++++-
 gdb/infrun.c                              |  6 ++-
 gdb/inline-frame.c                        | 43 ++++++++++++++---
 gdb/inline-frame.h                        | 11 +++--
 gdb/testsuite/gdb.ada/bp_inlined_func.exp | 22 +++------
 gdb/testsuite/gdb.dwarf2/implptr.exp      |  6 ++-
 gdb/testsuite/gdb.opt/inline-break.c      | 50 +++++++++++++++++++
 gdb/testsuite/gdb.opt/inline-break.exp    | 46 ++++++++++++++++++
 9 files changed, 224 insertions(+), 69 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 779240b8f0..e8e08ac92b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5312,54 +5312,21 @@ 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
@@ -5378,8 +5345,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
@@ -5404,12 +5371,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)
+      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;
@@ -5418,6 +5388,33 @@ bpstat_stop_status (const address_space *aspace,
 	}
     }
 
+  return bs_head;
+}
+
+/* See breakpoint.h.  */
+
+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 1b045ae092..400620f885 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -924,9 +924,37 @@ extern void bpstat_clear (bpstat *);
    is part of the bpstat is copied as well.  */
 extern bpstat bpstat_copy (bpstat);
 
+/* Build the (raw) bpstat 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);
+
+/* Get a bpstat associated with having just stopped at address
+   BP_ADDR in thread PTID.  STOP_CHAIN may be supplied as a previously
+   computed stop chain or NULL, in which case the stop chain will be
+   computed using build_bpstat_chain.
+
+   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.  */
+
 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);
 \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 cc12e6910f..df19478ef3 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5862,6 +5862,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
@@ -5893,7 +5894,8 @@ handle_signal_stop (struct execution_control_state *ecs)
 					     ecs->event_thread->prev_pc,
 					     &ecs->ws)))
 	{
-	  skip_inline_frames (ecs->ptid);
+	  stop_chain = build_bpstat_chain (aspace, stop_pc, &ecs->ws);
+	  skip_inline_frames (ecs->ptid, stop_chain);
 
 	  /* Re-fetch current thread's frame in case that invalidated
 	     the frame cache.  */
@@ -5942,7 +5944,7 @@ handle_signal_stop (struct execution_control_state *ecs)
      handles this event.  */
   ecs->event_thread->control.stop_bpstat
     = bpstat_stop_status (get_current_regcache ()->aspace (),
-			  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 2f701e7bce..1ac5835438 100644
--- a/gdb/inline-frame.c
+++ b/gdb/inline-frame.c
@@ -18,6 +18,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "breakpoint.h"
 #include "inline-frame.h"
 #include "addrmap.h"
 #include "block.h"
@@ -284,12 +285,36 @@ block_starting_point_at (CORE_ADDR pc, const struct block *block)
   return 1;
 }
 
-/* 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.  */
+/* Loop over the stop chain and determine if execution stopped in an
+   inlined frame because of a user breakpoint.  THIS_PC is the current
+   frame's PC.  */
+
+static bool
+stopped_by_user_bp_inline_frame (CORE_ADDR this_pc, bpstat stop_chain)
+{
+  for (bpstat s = stop_chain; s != NULL; s = s->next)
+    {
+      struct breakpoint *bpt = s->breakpoint_at;
+
+      if (bpt != NULL && user_breakpoint_p (bpt))
+	{
+	  bp_location *loc = s->bp_location_at;
+	  enum bp_loc_type t = loc->loc_type;
+
+	  if (loc->address == this_pc
+	      && (t == bp_loc_software_breakpoint
+		  || t == bp_loc_hardware_breakpoint))
+	    return true;
+	}
+    }
+
+  return false;
+}
+
+/* See inline-frame.h.  */
 
 void
-skip_inline_frames (ptid_t ptid)
+skip_inline_frames (ptid_t ptid, bpstat stop_chain)
 {
   const struct block *frame_block, *cur_block;
   struct symbol *last_sym = NULL;
@@ -313,8 +338,14 @@ skip_inline_frames (ptid_t ptid)
 	      if (BLOCK_START (cur_block) == this_pc
 		  || block_starting_point_at (this_pc, cur_block))
 		{
-		  skip_count++;
-		  last_sym = BLOCK_FUNCTION (cur_block);
+		  /* Do not skip the inlined frame if execution
+		     stopped in an inlined frame because of a user
+		     breakpoint.  */
+		  if (!stopped_by_user_bp_inline_frame (this_pc, stop_chain))
+		    {
+		      skip_count++;
+		      last_sym = BLOCK_FUNCTION (cur_block);
+		    }
 		}
 	      else
 		break;
diff --git a/gdb/inline-frame.h b/gdb/inline-frame.h
index 1d2e251cb1..d66ad44b7c 100644
--- a/gdb/inline-frame.h
+++ b/gdb/inline-frame.h
@@ -22,16 +22,21 @@
 
 struct frame_info;
 struct frame_unwind;
+struct bpstats;
 
 /* The inline frame unwinder.  */
 
 extern const struct frame_unwind inline_frame_unwind;
 
 /* 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.  */
 
-void skip_inline_frames (ptid_t ptid);
+   If non-NULL, STOP_CHAIN is used to determine whether a stop was caused by
+   a user breakpoint.  In that case, do not skip that inlined frame.  This
+   allows the inlined frame to be treated as if it were non-inlined from the
+   user's perspective.  GDB will stop "in" the inlined frame instead of
+   the caller.  */
+
+void skip_inline_frames (ptid_t ptid, struct bpstats *stop_chain);
 
 /* 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.ada/bp_inlined_func.exp b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
index 37ef6af8ab..0f615f5d9b 100644
--- a/gdb/testsuite/gdb.ada/bp_inlined_func.exp
+++ b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
@@ -38,21 +38,13 @@ gdb_test "break read_small" \
 # We do not verify each breakpoint info, but use continue commands instead
 # to verify that we properly stop on each expected breakpoint.
 
-gdb_test "continue" \
-         "Breakpoint $decimal, b\\.doit \\(\\).*" \
-         "Hitting first call of read_small"
-
-gdb_test "continue" \
-         "Breakpoint $decimal, foo \\(\\).*" \
-         "Hitting second call of read_small"
-
-gdb_test "continue" \
-         "Breakpoint $decimal, c\\.c_doit \\(\\).*" \
-         "Hitting third call of read_small"
-
-gdb_test "continue" \
-         "Breakpoint $decimal, c\\.c_doit2 \\(\\).*" \
-         "Hitting fourth call of read_small"
+for {set i 0} {$i < 4} {incr i} {
+    with_test_prefix "iteration $i" {
+	gdb_test "continue" \
+	    "Breakpoint $decimal, b\\.read_small \\(\\).*" \
+	    "stopped in read_small"
+    }
+}
 
 gdb_test "continue" \
          "Continuing\..*$inferior_exited_re.*" \
diff --git a/gdb/testsuite/gdb.dwarf2/implptr.exp b/gdb/testsuite/gdb.dwarf2/implptr.exp
index 890598c9ff..ab9c5276ed 100644
--- a/gdb/testsuite/gdb.dwarf2/implptr.exp
+++ b/gdb/testsuite/gdb.dwarf2/implptr.exp
@@ -66,9 +66,13 @@ proc implptr_test_baz {} {
     gdb_test "break implptr.c:$line" "Breakpoint 3.*" \
 	"set baz breakpoint for implptr"
     gdb_continue_to_breakpoint "continue to baz breakpoint for implptr"
+
+    # We are breaking in an inlined function.  GDB should appear to
+    # have stopped "in" the inlined function.
+    gdb_test "up" "#1  foo .*"
     gdb_test {p p[0].y} " = 92" "sanity check element 0"
     gdb_test {p p[1].y} " = 46" "sanity check element 1"
-    gdb_test "step" "\r\nadd \\(.*" "enter the inlined function"
+    gdb_test "down" "#0  add .*"
     gdb_test "p a->y" " = 92" "check element 0 for the offset"
     gdb_test "p b->y" " = 46" "check element 1 for the offset"
     gdb_continue_to_breakpoint "ignore the second baz breakpoint"
diff --git a/gdb/testsuite/gdb.opt/inline-break.c b/gdb/testsuite/gdb.opt/inline-break.c
index c3d633810a..922102debb 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 c6b037d153..008ff1ac33 100644
--- a/gdb/testsuite/gdb.opt/inline-break.exp
+++ b/gdb/testsuite/gdb.opt/inline-break.exp
@@ -185,4 +185,50 @@ for {set i 1} {$i <= [array size results]} {incr i} {
     gdb_test "info break $i" $results($i)
 }
 
+# Test "permanent" and "temporary" breakpoints.
+foreach_with_prefix cmd [list "break" "tbreak"] {
+
+    # Start with a clean state.
+    delete_breakpoints
+    if {![runto main]} {
+	untested "could not run to main"
+	return -1
+    }
+
+    # Assemble flags to pass to gdb_breakpoint.  Lame but this is just
+    # a test suite!
+    set break_flags "message"
+    if {[string match $cmd "tbreak"]} {
+	lappend break_flags "temporary"
+    }
+
+    # 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"} {
+	    eval gdb_breakpoint "${inline}_func$i" $break_flags
+	}
+    }
+
+    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

Re: [PATCH v2] Don't elide all inlined frames

[Pedro Alves]

On 05/14/2018 08:43 PM, Keith Seitz wrote:
> On 05/11/2018 08:55 AM, Pedro Alves wrote:
>> I was thinking of this comment:
>>
>>     /* Breakpoint that caused the stop.  This is nullified if the
>>        breakpoint ends up being deleted.  See comments on
>>        `bp_location_at' above for why do we need this field instead of
>>        following the location's owner.  */
>>     struct breakpoint *breakpoint_at;
>>
>> and noticed that the code uses "s->breakpoint_at" and somehow misread
>> it as still following the breakpoint's location list.
> 
> Ah, yes, okay, your comments make much more sense now. ;-)

Yeah.  Also thanks for spotting that the earlier comment about not
following the locations of the breakpoint (since the may not be
enabled/inserted) still applied, and fixing the patch.  ;-)

This version is OK.  Please push.

Thanks,
Pedro Alves

Re: [PATCH v2] Don't elide all inlined frames

[Keith Seitz]

On 05/16/2018 06:49 AM, Pedro Alves wrote:
>
> This version is OK.  Please push.

Excellent! Thank you. Pushed.

Keith