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

[Pedro Alves <palves at redhat dot com>]

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