Patchwork [v2] Don't elide all inlined frames

login
register
mail settings
Submitter Pedro Alves
Date May 11, 2018, 3:55 p.m.
Message ID <a4327229-77f3-b915-3d8b-65248c81b59f@redhat.com>
Download mbox | patch
Permalink /patch/27244/
State New
Headers show

Comments

Pedro Alves - May 11, 2018, 3:55 p.m.
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(-)

Patch

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;