Re: [PATCH v2] [gdb] Support frames inlined into the outer frame

[Simon Marchi <simark@simark.ca>]

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

On 2020-06-08 8:00 a.m., Luis Machado wrote:
> 
> I don't see the same, even with the fixup of memcmp. Though gdb.base/break.exp has full passes with the change, the following tests internal error with the patch...
> 
> gdb.mi/mi-nonstop.exp
> gdb.threads/clone-thread_db.exp
> gdb.threads/current-lwp-dead.exp
> gdb.threads/hand-call-in-threads.exp
> gdb.threads/linux-dp.exp
> gdb.threads/local-watch-wrong-thread.exp
> gdb.threads/queue-signal.exp
> gdb.threads/schedlock.exp
> gdb.threads/thread_check.exp
> gdb.threads/tls.exp
> 
> #1  0x0000ffffb7fa1088 in start_thread () from /lib/aarch64-linux-gnu/libpthread.so.0
> ../../../repos/binutils-gdb/gdb/frame.c:551: internal-error: void compute_frame_id(frame_info*): Assertion `frame_id_p (fi->this_id.value)' failed.
> 
> Scott, could you please send a v3 so I can make sure I tested the right version? I was initially slightly confused with what version Simon was talking about since I had already tested v2.

I don't see these tests failing.  Can you please share your test setup?  Maybe we'll find
what's different between us.

I've attached the patch I am testing.  It is just patch v2 with `== 0` added after the
memcmp call.  I am testing on top of commit 7d8b91fda9fed423b91d4d43b19dd068457fe555.  I
am testing on machine gcc117 of the compile farm, which is running Debian 9.12 (stretch).
The gcc version there is 6.3.0.

Simon



[-- Attachment #2: 0001-Support-frames-inlined-into-the-outer-frame.patch --]
[-- Type: text/x-patch, Size: 5753 bytes --]

From 26297062a61b3dd7bd1a3e55005aaa932713c7da Mon Sep 17 00:00:00 2001
From: Scott Linder <scott@scottlinder.com>
Date: Tue, 31 Mar 2020 15:18:56 -0400
Subject: [PATCH] Support frames inlined into the outer frame

Broaden the definition of `outer_frame_id` to effectively create a new
class of "invalid" IDs to represent frames inlined into the outer frame.
These new IDs behave like the outer frame, in that they are "invalid",
yet return true from `frame_id_p` and compare equal to themselves.

2020-03-18  Scott Linder  <scott@scottlinder.com>

	* frame.c (frame_id_p): Consider functions inlined into outer frame
	as valid.
	(frame_id_eq): Consider functions inlined into outer frame with same
	artificial_depth as equal.
	(outer_frame_id_p): New.
	* frame.h (outer_frame_id): Update comment.
	(outer_frame_id_p): New.
	* inline-frame.c (inline_frame_this_id): Remove assert that prevents
	inline frame ids in outer frame.

Change-Id: I8aa129c667dccc31590ffdf426586418493a6ebe
---
 gdb/frame.c        | 41 ++++++++++++++++++++++++++++-------------
 gdb/frame.h        | 12 +++++++++++-
 gdb/inline-frame.c |  4 ----
 3 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index ff27b9f00e..d03d6faed3 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -700,11 +700,7 @@ frame_id_p (struct frame_id l)
 {
   int p;
 
-  /* The frame is valid iff it has a valid stack address.  */
-  p = l.stack_status != FID_STACK_INVALID;
-  /* outer_frame_id is also valid.  */
-  if (!p && memcmp (&l, &outer_frame_id, sizeof (l)) == 0)
-    p = 1;
+  p = l.stack_status != FID_STACK_INVALID || outer_frame_id_p (l);
   if (frame_debug)
     {
       fprintf_unfiltered (gdb_stdlog, "{ frame_id_p (l=");
@@ -728,14 +724,15 @@ frame_id_eq (struct frame_id l, struct frame_id r)
 {
   int eq;
 
-  if (l.stack_status == FID_STACK_INVALID && l.special_addr_p
-      && r.stack_status == FID_STACK_INVALID && r.special_addr_p)
-    /* The outermost frame marker is equal to itself.  This is the
-       dodgy thing about outer_frame_id, since between execution steps
-       we might step into another function - from which we can't
-       unwind either.  More thought required to get rid of
-       outer_frame_id.  */
-    eq = 1;
+  if (outer_frame_id_p (l) && outer_frame_id_p (r))
+    /* The outermost frame marker, and any inline frame markers derived
+       from it (with artificial_depth > 0), are equal to themselves.  The
+       problem with outer_frame_id is that, if between execution steps, we
+       step into a completely separate function (not an inlined function)
+       that also identifies as outer_frame_id, then we can't distinguish
+       between the previous frame and the new frame.  More thought is
+       required to get rid of outer_frame_id.  */
+    eq = l.artificial_depth == r.artificial_depth;
   else if (l.stack_status == FID_STACK_INVALID
 	   || r.stack_status == FID_STACK_INVALID)
     /* Like a NaN, if either ID is invalid, the result is false.
@@ -771,6 +768,24 @@ frame_id_eq (struct frame_id l, struct frame_id r)
   return eq;
 }
 
+int
+outer_frame_id_p (struct frame_id l)
+{
+  int p;
+
+  /* The artificial_depth can vary so we ignore it when checking if this is
+     an outer_frame_id.  */
+  l.artificial_depth = 0;
+  p = memcmp (&l, &outer_frame_id, sizeof (outer_frame_id)) == 0;
+  if (frame_debug)
+    {
+      fprintf_unfiltered (gdb_stdlog, "{ outer_frame_id_p (l=");
+      fprint_frame_id (gdb_stdlog, l);
+      fprintf_unfiltered (gdb_stdlog, ") -> %d }\n", p);
+    }
+  return p;
+}
+
 /* Safety net to check whether frame ID L should be inner to
    frame ID R, according to their stack addresses.
 
diff --git a/gdb/frame.h b/gdb/frame.h
index e835d49f9c..5e6690b2a1 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -195,7 +195,13 @@ extern const struct frame_id sentinel_frame_id;
 
 /* This means "there is no frame ID, but there is a frame".  It should be
    replaced by best-effort frame IDs for the outermost frame, somehow.
-   The implementation is only special_addr_p set.  */
+
+   The implementation has stack_status set to FID_STACK_INVALID,
+   special_addr_p set to 1, artificial_depth set to 0 or greater, and all other
+   members set to 0. For the non-inline outer frame artificial_depth remains
+   set to 0 and for frames inlined into it the artificial_depth is set in the
+   typical way.  Checking if a frame marker is an outer_frame_id should be done
+   with outer_frame_id_p.  */
 extern const struct frame_id outer_frame_id;
 
 /* Flag to control debugging.  */
@@ -254,6 +260,10 @@ extern int frame_id_artificial_p (struct frame_id l);
    either L or R have a zero .func, then the same frame base.  */
 extern int frame_id_eq (struct frame_id l, struct frame_id r);
 
+/* Returns non-zero when L is an outer frame marker or any inline frame marker
+   derived from it.  */
+extern int outer_frame_id_p (struct frame_id l);
+
 /* Write the internal representation of a frame ID on the specified
    stream.  */
 extern void fprint_frame_id (struct ui_file *file, struct frame_id id);
diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
index c650195e57..a187630840 100644
--- a/gdb/inline-frame.c
+++ b/gdb/inline-frame.c
@@ -171,10 +171,6 @@ inline_frame_this_id (struct frame_info *this_frame,
      frame").  This will take work.  */
   gdb_assert (frame_id_p (*this_id));
 
-  /* For now, require we don't match outer_frame_id either (see
-     comment above).  */
-  gdb_assert (!frame_id_eq (*this_id, outer_frame_id));
-
   /* Future work NOTE: Alexandre Oliva applied a patch to GCC 4.3
      which generates DW_AT_entry_pc for inlined functions when
      possible.  If this attribute is available, we should use it
-- 
2.11.0