[RFC][PATCH] Support frames inlined into the outer frame

[RFC][PATCH] Support frames inlined into the outer frame

[scott]

AMD is working on a port of GDB for our GPUs based on the ROCm stack
(https://rocm.github.io/). We recently open-sourced the fork at
https://github.com/ROCm-Developer-Tools/ROCgdb. We hope to begin 
upstreaming
patches where possible,
and https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/767 is the 
first
such patch.

We frequently have functions inlined into the outermost frame. The 
current
implementation which introduced `outer_frame_id` as a "valid" ID 
distinct from
`null_frame_id` does not support this, asserting the need for both a 
valid
and non-`outer_frame_id` ID to base the ID of the inlined frame on.

This patch changes the definition of `outer_frame_id` slightly to 
effectively
represent a class of IDs which identify a frame inlined into the outer 
frame.
These differ in `artificial_depth`, but otherwise behave just as
`outer_frame_id` in that they are `FID_STACK_INVALID`, yet `frame_id_p` 
returns
`true` and they compare equal to each other.

Running the testsuite both with and without the patch doesn't yield any 
obvious
regressions, although I have not come up with a test case to prove this 
out on
e.g. x86.

Does this seem reasonable? It is a bit of a hack on a hack, considering 
the
existing issues with `outer_frame_id` and the obvious desire to remove 
it
completely. At the same time, there is a fair amount of thought and 
effort
involved in making that change, and I think it can/should be done 
independently
of fixing this bug. My feeling is this patch is a pretty non-invasive 
change
that doesn't make the situation fundamentally worse, but any feedback is
appreciated.

Cheers,
Scott

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

[scott]

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

It was pointed out that posting patches directly to the list is 
preferred over Gerrit, so I abandoned the Gerrit review and I'm 
attaching the patch here. The wiki says a git-format-patch'd attachment 
is OK, but let me know if it isn't the norm!

Scott

On 2020-03-18 16:43, scott@scottlinder.com wrote:
> AMD is working on a port of GDB for our GPUs based on the ROCm stack
> (https://rocm.github.io/). We recently open-sourced the fork at
> https://github.com/ROCm-Developer-Tools/ROCgdb. We hope to begin 
> upstreaming
> patches where possible,
> and https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/767 is the 
> first
> such patch.
> 
> We frequently have functions inlined into the outermost frame. The 
> current
> implementation which introduced `outer_frame_id` as a "valid" ID 
> distinct from
> `null_frame_id` does not support this, asserting the need for both a 
> valid
> and non-`outer_frame_id` ID to base the ID of the inlined frame on.
> 
> This patch changes the definition of `outer_frame_id` slightly to 
> effectively
> represent a class of IDs which identify a frame inlined into the outer 
> frame.
> These differ in `artificial_depth`, but otherwise behave just as
> `outer_frame_id` in that they are `FID_STACK_INVALID`, yet `frame_id_p` 
> returns
> `true` and they compare equal to each other.
> 
> Running the testsuite both with and without the patch doesn't yield any 
> obvious
> regressions, although I have not come up with a test case to prove this 
> out on
> e.g. x86.
> 
> Does this seem reasonable? It is a bit of a hack on a hack, considering 
> the
> existing issues with `outer_frame_id` and the obvious desire to remove 
> it
> completely. At the same time, there is a fair amount of thought and 
> effort
> involved in making that change, and I think it can/should be done 
> independently
> of fixing this bug. My feeling is this patch is a pretty non-invasive 
> change
> that doesn't make the situation fundamentally worse, but any feedback 
> is
> appreciated.
> 
> Cheers,
> Scott

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gdb-Support-frames-inlined-into-the-outer-frame.patch --]
[-- Type: text/x-diff; name=0001-gdb-Support-frames-inlined-into-the-outer-frame.patch, Size: 4267 bytes --]

From 983baad2e87f1d7bb7905859b24a6a9fb7d103f3 Mon Sep 17 00:00:00 2001
From: Scott Linder <scott@scottlinder.com>
Date: Wed, 18 Mar 2020 15:41:53 -0400
Subject: [PATCH] [gdb] 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.

	* frame.h (outer_frame_id): Update comment.
	(frame_id_p): Update comment.

	* inline-frame.c (inline_frame_this_id): Remove assert that prevents
	inline frame ids in outer frame.

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

diff --git a/gdb/frame.c b/gdb/frame.c
index d74d1d5c7c..b62d68f12a 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -694,8 +694,8 @@ frame_id_p (struct frame_id l)
 
   /* 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)
+  /* outer_frame_id and functions inlined into it are also valid.  */
+  if (!p && l.special_addr_p)
     p = 1;
   if (frame_debug)
     {
@@ -722,12 +722,13 @@ frame_id_eq (struct frame_id l, struct frame_id r)
 
   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
+    /* The outermost frame marker, and any inline frame markers
+       derived from it, are equal to themselves.  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;
+    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.
diff --git a/gdb/frame.h b/gdb/frame.h
index cfc15022ed..d394382903 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -195,7 +195,8 @@ 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 is only special_addr_p, and possibly
+   artificial_depth, set.  */
 extern const struct frame_id outer_frame_id;
 
 /* Flag to control debugging.  */
@@ -237,8 +238,8 @@ extern struct frame_id
 extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
 
 /* Returns non-zero when L is a valid frame (a valid frame has a
-   non-zero .base).  The outermost frame is valid even without an
-   ID.  */
+   non-zero .base).  The outermost frame and any frames inlined into it
+   are valid even without an ID.  */
 extern int frame_id_p (struct frame_id l);
 
 /* Returns non-zero when L is a valid frame representing a frame made up by GDB
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.17.1

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

[Simon Marchi]

On 2020-03-18 5:17 p.m., scott@scottlinder.com wrote:
> It was pointed out that posting patches directly to the list is 
> preferred over Gerrit, so I abandoned the Gerrit review and I'm 
> attaching the patch here. The wiki says a git-format-patch'd attachment 
> is OK, but let me know if it isn't the norm!

Hi Scott,

Doing git-format-patch and attaching the patch is fine, since it pretty much
ensures the patch is not mangled by your email client.  But really the preferred
way is to use "git-send-email".  This sends the content of the patch in the body
of the email, so it makes it easier to reply, quoting parts of the patch.  It also
makes it easy to send multiple patches in series.

Simon

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

[scott]

Hi Simon,

Makes sense, I'll use git-send-email in the future. Thank you for 
bearing with
me as I get set up, and I'm sorry for the spam. I also just noticed I 
neglected
to delete Gerrit's Change-Id from the commit message, so I may send an 
updated
diff with git-send-email once I have it set up, as a bit of a smoke 
test.

Scott

On 2020-03-18 17:27, Simon Marchi wrote:
> On 2020-03-18 5:17 p.m., scott@scottlinder.com wrote:
>> It was pointed out that posting patches directly to the list is
>> preferred over Gerrit, so I abandoned the Gerrit review and I'm
>> attaching the patch here. The wiki says a git-format-patch'd 
>> attachment
>> is OK, but let me know if it isn't the norm!
> 
> Hi Scott,
> 
> Doing git-format-patch and attaching the patch is fine, since it pretty 
> much
> ensures the patch is not mangled by your email client.  But really the 
> preferred
> way is to use "git-send-email".  This sends the content of the patch in 
> the body
> of the email, so it makes it easier to reply, quoting parts of the
> patch.  It also
> makes it easy to send multiple patches in series.
> 
> Simon

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

[Simon Marchi]

On 2020-03-18 5:42 p.m., scott@scottlinder.com wrote:
> Hi Simon,
> 
> Makes sense, I'll use git-send-email in the future. Thank you for 
> bearing with
> me as I get set up, and I'm sorry for the spam. I also just noticed I 
> neglected
> to delete Gerrit's Change-Id from the commit message, so I may send an 
> updated
> diff with git-send-email once I have it set up, as a bit of a smoke 
> test.
> 
> Scott

No worries, I'm happy to help people get the right tools, it helps everybody
in the long run.  Don't sweat about Gerrit's change id.  In fact, we might
even try to use it in the future to track patch versions on Patchwork (we
are supposed to get an instance running soon).

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

[Scott Linder]

Including the patch inline this time. I apologize again for all the
noise.

[PATCH] [gdb] Support frames inlined into the outer frame

[Scott Linder]

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.

	* frame.h (outer_frame_id): Update comment.
	(frame_id_p): Update comment.

	* inline-frame.c (inline_frame_this_id): Remove assert that prevents
	inline frame ids in outer frame.

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

diff --git a/gdb/frame.c b/gdb/frame.c
index d74d1d5c7c..b62d68f12a 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -694,8 +694,8 @@ frame_id_p (struct frame_id l)
 
   /* 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)
+  /* outer_frame_id and functions inlined into it are also valid.  */
+  if (!p && l.special_addr_p)
     p = 1;
   if (frame_debug)
     {
@@ -722,12 +722,13 @@ frame_id_eq (struct frame_id l, struct frame_id r)
 
   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
+    /* The outermost frame marker, and any inline frame markers
+       derived from it, are equal to themselves.  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;
+    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.
diff --git a/gdb/frame.h b/gdb/frame.h
index cfc15022ed..d394382903 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -195,7 +195,8 @@ 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 is only special_addr_p, and possibly
+   artificial_depth, set.  */
 extern const struct frame_id outer_frame_id;
 
 /* Flag to control debugging.  */
@@ -237,8 +238,8 @@ extern struct frame_id
 extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
 
 /* Returns non-zero when L is a valid frame (a valid frame has a
-   non-zero .base).  The outermost frame is valid even without an
-   ID.  */
+   non-zero .base).  The outermost frame and any frames inlined into it
+   are valid even without an ID.  */
 extern int frame_id_p (struct frame_id l);
 
 /* Returns non-zero when L is a valid frame representing a frame made up by GDB
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.17.1

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

[Andrew Burgess]

Scott,

Thanks for looking into this.

* Scott Linder <scott@scottlinder.com> [2020-03-18 18:11:19 -0400]:

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

I'm curious as to which target you're working on seeing this issue.

> 
> 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.
> 
> 	* frame.h (outer_frame_id): Update comment.
> 	(frame_id_p): Update comment.
> 
> 	* inline-frame.c (inline_frame_this_id): Remove assert that prevents
> 	inline frame ids in outer frame.

ChangeLog entries don't have blank lines between entries.

> 
> Change-Id: I8aa129c667dccc31590ffdf426586418493a6ebe
> ---
>  gdb/frame.c        | 11 ++++++-----
>  gdb/frame.h        |  7 ++++---
>  gdb/inline-frame.c |  4 ----
>  3 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/gdb/frame.c b/gdb/frame.c
> index d74d1d5c7c..b62d68f12a 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -694,8 +694,8 @@ frame_id_p (struct frame_id l)
>  
>    /* 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)
> +  /* outer_frame_id and functions inlined into it are also valid.  */
> +  if (!p && l.special_addr_p)

It's not immediately obvious how the comment relates to the code
below.  I wonder if changing the code to:

  if (!p && is_outer_frame_p (l))

would be better, where is_outer_frame_p is a new function.  My
motivation here is that we previously checked all fields of l, not
just the special_addr_p field - I wonder if we should restore more of
that checking?  Specifically, the value in the special_addr field?  My
understanding is that if (!p) then (special_addr == 0) - so maybe we
can assert that.

>      p = 1;
>    if (frame_debug)
>      {
> @@ -722,12 +722,13 @@ frame_id_eq (struct frame_id l, struct frame_id r)
>  
>    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
> +    /* The outermost frame marker, and any inline frame markers
> +       derived from it, are equal to themselves.  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.  */

I think we need to rewrite more of this comment.  The part that says
"...since between execution steps we might step into another
function..." now seems ambiguous.  The point this is trying to make is
that if we step into a totally different function that also identifies
as outer_frame_id then we can't tell the difference between the
original function and the new one.

The problem is if we step into an inline function then we can (now)
tell them apart.  Maybe something like:

  The outermost frame marker, and any inilne 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 = 1;
> +    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.
> diff --git a/gdb/frame.h b/gdb/frame.h
> index cfc15022ed..d394382903 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -195,7 +195,8 @@ 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 is only special_addr_p, and possibly
> +   artificial_depth, set.  */
>  extern const struct frame_id outer_frame_id;

I'd drop the "possibly" from this comment, as the value is always
valid, right? 0 means no inline frames, and is just as set as a value
of 1 or more.  Saying possible risks that someone might think they
need to figure out if artificial_depth is set or not, when this is not
the case.

  The implementation has special_addr set to 0, special_addr_p set
  to 1, and artificial_depth set to 0 or greater.

>  
>  /* Flag to control debugging.  */
> @@ -237,8 +238,8 @@ extern struct frame_id
>  extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
>  
>  /* Returns non-zero when L is a valid frame (a valid frame has a
> -   non-zero .base).  The outermost frame is valid even without an
> -   ID.  */
> +   non-zero .base).  The outermost frame and any frames inlined into it
> +   are valid even without an ID.  */
>  extern int frame_id_p (struct frame_id l);

I personally wouldn't make this change.  You're broadening the
definition of outer_frame_id, so the original comment is still valid,
anything else is detail.

>  
>  /* Returns non-zero when L is a valid frame representing a frame made up by GDB
> 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.17.1
> 

It would be great if we could come up for some tests for this new
code, but I can't immediately see how you might setup something like
this on any of the targets I'm most familiar with.

Also do you have a copyright assignment in place as I think one would
be need to take this patch.

Thanks,
Andrew

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

[scott]

Andrew,

Thank you for the review! I'm sorry for taking so long to respond.

I also apologize if my email client munges anything; I had intended to 
set up and learn how to use Mutt or something that would play nicer with 
lists, but I haven't had the chance yet.

On 2020-03-24 06:22, Andrew Burgess wrote:
> Scott,
> 
> Thanks for looking into this.
> 
> * Scott Linder <scott@scottlinder.com> [2020-03-18 18:11:19 -0400]:
> 
>> 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.
> 
> I'm curious as to which target you're working on seeing this issue.
> 

I tried to give a little more exposition in the first email in the 
thread, but in short we encounter this with AMD GPU code objects.  The 
entry function really is at the bottom of the device stack, there is no 
equivalent to e.g. a crt _start.

>> 
>> 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.
>> 
>> 	* frame.h (outer_frame_id): Update comment.
>> 	(frame_id_p): Update comment.
>> 
>> 	* inline-frame.c (inline_frame_this_id): Remove assert that prevents
>> 	inline frame ids in outer frame.
> 
> ChangeLog entries don't have blank lines between entries.
> 

I will fix this, I had misunderstood what the pages the wiki referenced 
were saying.  Thank you for bearing with me!

>> 
>> Change-Id: I8aa129c667dccc31590ffdf426586418493a6ebe
>> ---
>>  gdb/frame.c        | 11 ++++++-----
>>  gdb/frame.h        |  7 ++++---
>>  gdb/inline-frame.c |  4 ----
>>  3 files changed, 10 insertions(+), 12 deletions(-)
>> 
>> diff --git a/gdb/frame.c b/gdb/frame.c
>> index d74d1d5c7c..b62d68f12a 100644
>> --- a/gdb/frame.c
>> +++ b/gdb/frame.c
>> @@ -694,8 +694,8 @@ frame_id_p (struct frame_id l)
>> 
>>    /* 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)
>> +  /* outer_frame_id and functions inlined into it are also valid.  */
>> +  if (!p && l.special_addr_p)
> 
> It's not immediately obvious how the comment relates to the code
> below.  I wonder if changing the code to:
> 
>   if (!p && is_outer_frame_p (l))
> 
> would be better, where is_outer_frame_p is a new function.  My
> motivation here is that we previously checked all fields of l, not
> just the special_addr_p field - I wonder if we should restore more of
> that checking?  Specifically, the value in the special_addr field?  My
> understanding is that if (!p) then (special_addr == 0) - so maybe we
> can assert that.
> 

I agree that a new is_outer_frame_p makes sense, although at that point 
I question whether the comment is really useful.  I also don't think the 
comment above is very helpful, and even before the patch the use of 
`iff` was a lie because of the exception for outer frames.  Would it 
make sense to just delete these and let the comment for frame_id_p in 
frame.h guide the reader, especially considering how short the body of 
the function is?

>>      p = 1;
>>    if (frame_debug)
>>      {
>> @@ -722,12 +722,13 @@ frame_id_eq (struct frame_id l, struct frame_id 
>> r)
>> 
>>    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
>> +    /* The outermost frame marker, and any inline frame markers
>> +       derived from it, are equal to themselves.  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.  */
> 
> I think we need to rewrite more of this comment.  The part that says
> "...since between execution steps we might step into another
> function..." now seems ambiguous.  The point this is trying to make is
> that if we step into a totally different function that also identifies
> as outer_frame_id then we can't tell the difference between the
> original function and the new one.
> 
> The problem is if we step into an inline function then we can (now)
> tell them apart.  Maybe something like:
> 
>   The outermost frame marker, and any inilne 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.
> 

Isn't it still the case that we can get confused if we step into another 
function that is outer_frame_id *and* we end up in a different inline 
frame of the same depth?  Or is your point that we will always stop in 
the non-inlined frame first, so we can't ever hit this?  I don't know 
that I understand how one could construct any of these cases, though; 
how could you step from a function that is the "outer frame" into 
another function that is also the "outer frame"?

>> -    eq = 1;
>> +    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.
>> diff --git a/gdb/frame.h b/gdb/frame.h
>> index cfc15022ed..d394382903 100644
>> --- a/gdb/frame.h
>> +++ b/gdb/frame.h
>> @@ -195,7 +195,8 @@ 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 is only special_addr_p, and possibly
>> +   artificial_depth, set.  */
>>  extern const struct frame_id outer_frame_id;
> 
> I'd drop the "possibly" from this comment, as the value is always
> valid, right? 0 means no inline frames, and is just as set as a value
> of 1 or more.  Saying possible risks that someone might think they
> need to figure out if artificial_depth is set or not, when this is not
> the case.
> 
>   The implementation has special_addr set to 0, special_addr_p set
>   to 1, and artificial_depth set to 0 or greater.
> 

That makes sense to me, I'll update this to be more precise.  I think 
the original comment here is similarly incorrect, depending on how you 
define "set", because all the fields of the outer_frame_id must have 
been required to be set for the use of memcmp elsewhere to work.

>> 
>>  /* Flag to control debugging.  */
>> @@ -237,8 +238,8 @@ extern struct frame_id
>>  extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
>> 
>>  /* Returns non-zero when L is a valid frame (a valid frame has a
>> -   non-zero .base).  The outermost frame is valid even without an
>> -   ID.  */
>> +   non-zero .base).  The outermost frame and any frames inlined into 
>> it
>> +   are valid even without an ID.  */
>>  extern int frame_id_p (struct frame_id l);
> 
> I personally wouldn't make this change.  You're broadening the
> definition of outer_frame_id, so the original comment is still valid,
> anything else is detail.
> 

I changed this because in my understanding the outer_frame_id 
implementation detail is distinct from the term "outermost frame".  The 
patch broadens the definition of outer_frame_id to represent a class of 
IDs, but to me the phrasing "outermost frame" still refers only to the 
non-artificial frame that is actually at the bottom of the call stack.  
Maybe I could just change this to say "outer_frame_id is valid even 
without an ID"?

>> 
>>  /* Returns non-zero when L is a valid frame representing a frame made 
>> up by GDB
>> 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.17.1
>> 
> 
> It would be great if we could come up for some tests for this new
> code, but I can't immediately see how you might setup something like
> this on any of the targets I'm most familiar with.
> 

I can try to come up with something, but I'm also not familiar with any 
of the existing targets supported by GDB well enough to know off-hand 
how to construct a test.

> Also do you have a copyright assignment in place as I think one would
> be need to take this patch.
> 

AMD owns the copyright; would you be able to check if AMD has an 
appropriate copyright assignment on file, and if not let me know so I 
can make sure we get one filed?

> Thanks,
> Andrew

Thanks,
Scott

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

[Scott Linder]

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
---
Changes since v1:
* Fix ChangeLog formatting.
* Add outer_frame_id_p to capture new definition of outer_frame_id in
  one place and to restore checks for all members.
* Reword some comments to make them more precise, borrowing a lot of
  wording from Andrew Burgess.
* Remove some comments describing what is now obvious.
* Undo update to frame_id_p comment which exposes implementation details.

 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 d74d1d5c7c..c6154c2d9c 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -692,11 +692,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=");
@@ -720,14 +716,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.
@@ -763,6 +760,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));
+  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 cfc15022ed..66f19c91dc 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.  */
@@ -250,6 +256,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.17.1

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

[Pedro Alves]

On 3/30/20 11:22 PM, scott@scottlinder.com wrote:
> 
> AMD owns the copyright; would you be able to check if AMD has an appropriate copyright assignment on file, and if not let me know so > I can make sure we get one filed?

I checked -- indeed there's a blanket copyright assignment in place for GDB.
I.e., we can accept all contributions from AMD from all employees/contractors.

Thanks,
Pedro Alves

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

[Andrew Burgess]

* Scott Linder <scott@scottlinder.com> [2020-03-31 15:18:56 -0400]:

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

Thanks, this looks much great.  I have a couple of tiny suggestions,
described inline.

Thanks,
Andrew


> 
> Change-Id: I8aa129c667dccc31590ffdf426586418493a6ebe
> ---
> Changes since v1:
> * Fix ChangeLog formatting.
> * Add outer_frame_id_p to capture new definition of outer_frame_id in
>   one place and to restore checks for all members.
> * Reword some comments to make them more precise, borrowing a lot of
>   wording from Andrew Burgess.
> * Remove some comments describing what is now obvious.
> * Undo update to frame_id_p comment which exposes implementation details.
> 
>  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 d74d1d5c7c..c6154c2d9c 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -692,11 +692,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=");
> @@ -720,14 +716,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.  */

In a previous email, about this comment you wrote:

  Isn't it still the case that we can get confused if we step into another
  function that is outer_frame_id *and* we end up in a different inline
  frame of the same depth?  Or is your point that we will always stop in
  the non-inlined frame first, so we can't ever hit this?  I don't know
  that I understand how one could construct any of these cases, though;
  how could you step from a function that is the "outer frame" into
  another function that is also the "outer frame"?

Yes, I agree with you, and I hadn't considered this case.  The problem
with outer_frame_id before was that if you stepped into a different
function that was also outer_frame_id then you couldn't tell.  After
your patch if you step into another function that is outer_frame_id
*and* the artificial_depth is the same, then you can't tell.

Do feel free to rewrite the above as you see fit.  I agree that it's a
pretty unlikely case, but if we're going to document a known
limitation we might as well try to be accurate.

> +    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.
> @@ -763,6 +760,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));

This function should can be static within this file, and should return
a bool (and p should change type to match).

> +  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 cfc15022ed..66f19c91dc 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.  */
> @@ -250,6 +256,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.17.1
> 

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

[Luis Machado]

Hi Scott,

I was giving this a try on aarch64-linux to see if it fixed an existing 
problem handling inline frames, but it seems to completely break GDB for 
this architecture.

The testsuite runs into a number of internal errors of this kind:

gdb/frame.c:1841: internal-error: frame_info* 
get_next_frame_sentinel_okay(frame_info*): Assertion `this_frame != 
sentinel_frame' failed.

Even basic tests like gdb.base/break.exp run into this.

I'd recommend running this through the buildbot/tryserver to catch 
regressions. This is a very delicate area that is known to break things.

On 3/31/20 4:18 PM, Scott Linder wrote:
> 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
> ---
> Changes since v1:
> * Fix ChangeLog formatting.
> * Add outer_frame_id_p to capture new definition of outer_frame_id in
>    one place and to restore checks for all members.
> * Reword some comments to make them more precise, borrowing a lot of
>    wording from Andrew Burgess.
> * Remove some comments describing what is now obvious.
> * Undo update to frame_id_p comment which exposes implementation details.
> 
>   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 d74d1d5c7c..c6154c2d9c 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -692,11 +692,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=");
> @@ -720,14 +716,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.
> @@ -763,6 +760,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));
> +  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 cfc15022ed..66f19c91dc 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.  */
> @@ -250,6 +256,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
> 

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

[Scott Linder]

On Thu, Apr 02, 2020 at 08:30:19PM +0100, Pedro Alves wrote:
> On 3/30/20 11:22 PM, scott@scottlinder.com wrote:
> > 
> > AMD owns the copyright; would you be able to check if AMD has an appropriate copyright assignment on file, and if not let me know so > I can make sure we get one filed?
> 
> I checked -- indeed there's a blanket copyright assignment in place for GDB.
> I.e., we can accept all contributions from AMD from all employees/contractors.
> 
> Thanks,
> Pedro Alves
> 

Perfect, thank you for checking!

Thanks,
Scott

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

[Scott Linder]

On Fri, Apr 03, 2020 at 06:00:31PM +0100, Andrew Burgess wrote:
> * Scott Linder <scott@scottlinder.com> [2020-03-31 15:18:56 -0400]:
> 
> > 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.
> 
> Thanks, this looks much great.  I have a couple of tiny suggestions,
> described inline.
> 
> Thanks,
> Andrew
> 
> 
> > 
> > Change-Id: I8aa129c667dccc31590ffdf426586418493a6ebe
> > ---
> > Changes since v1:
> > * Fix ChangeLog formatting.
> > * Add outer_frame_id_p to capture new definition of outer_frame_id in
> >   one place and to restore checks for all members.
> > * Reword some comments to make them more precise, borrowing a lot of
> >   wording from Andrew Burgess.
> > * Remove some comments describing what is now obvious.
> > * Undo update to frame_id_p comment which exposes implementation details.
> > 
> >  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 d74d1d5c7c..c6154c2d9c 100644
> > --- a/gdb/frame.c
> > +++ b/gdb/frame.c
> > @@ -692,11 +692,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=");
> > @@ -720,14 +716,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.  */
> 
> In a previous email, about this comment you wrote:
> 
>   Isn't it still the case that we can get confused if we step into another
>   function that is outer_frame_id *and* we end up in a different inline
>   frame of the same depth?  Or is your point that we will always stop in
>   the non-inlined frame first, so we can't ever hit this?  I don't know
>   that I understand how one could construct any of these cases, though;
>   how could you step from a function that is the "outer frame" into
>   another function that is also the "outer frame"?
> 
> Yes, I agree with you, and I hadn't considered this case.  The problem
> with outer_frame_id before was that if you stepped into a different
> function that was also outer_frame_id then you couldn't tell.  After
> your patch if you step into another function that is outer_frame_id
> *and* the artificial_depth is the same, then you can't tell.
> 
> Do feel free to rewrite the above as you see fit.  I agree that it's a
> pretty unlikely case, but if we're going to document a known
> limitation we might as well try to be accurate.
> 
Thank you for the clarification, I will try to document all the
possibilities without being too verbose.
> > +    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.
> > @@ -763,6 +760,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));
> 
> This function should can be static within this file, and should return
> a bool (and p should change type to match).
> 
Ok, will do.
> > +  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 cfc15022ed..66f19c91dc 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.  */
> > @@ -250,6 +256,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.17.1
> > 

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

[Scott Linder]

On Fri, Apr 03, 2020 at 04:37:10PM -0300, Luis Machado wrote:
> Hi Scott,
> 
> I was giving this a try on aarch64-linux to see if it fixed an existing
> problem handling inline frames, but it seems to completely break GDB for
> this architecture.
> 
> The testsuite runs into a number of internal errors of this kind:
> 
> gdb/frame.c:1841: internal-error: frame_info*
> get_next_frame_sentinel_okay(frame_info*): Assertion `this_frame !=
> sentinel_frame' failed.
> 
> Even basic tests like gdb.base/break.exp run into this.
> 
> I'd recommend running this through the buildbot/tryserver to catch
> regressions. This is a very delicate area that is known to break things.
> 

Hi Luis,

Thank you for testing this and giving me some more hints :)

It seems like I will have to get commit access before I can request
buildbot access, so I will start going about that and hopefully be able
to address this in the patch soon.

Thanks,
Scott

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

[Simon Marchi]

On 2020-03-31 3:18 p.m., Scott Linder wrote:
> @@ -763,6 +760,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));

This should be `memcmp (...) == 0`.  Currently, the function returns true when
the frame is not an outer frame id, which is the opposite of what it is supposed
to do.

With this, the test gdb.base/break.exp on AArch64 runs fine.  I will launch a full
test run to see if there are any other problems.

You can make the new function return "bool" instead of "int", and use true/false instead
of zero/non-zero (both in the code and comments).

Simon

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

[Simon Marchi]

On 2020-06-04 12:11 p.m., Simon Marchi wrote:
> On 2020-03-31 3:18 p.m., Scott Linder wrote:
>> @@ -763,6 +760,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));
> 
> This should be `memcmp (...) == 0`.  Currently, the function returns true when
> the frame is not an outer frame id, which is the opposite of what it is supposed
> to do.
> 
> With this, the test gdb.base/break.exp on AArch64 runs fine.  I will launch a full
> test run to see if there are any other problems.
> 
> You can make the new function return "bool" instead of "int", and use true/false instead
> of zero/non-zero (both in the code and comments).
> 
> Simon

With this change, the full test run on AArch64 came out clean.  I'll try a try
job on the buildbot, but I haven't had much success with it recently.

Simon

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

[Luis Machado]

On 6/4/20 4:23 PM, Simon Marchi wrote:
> On 2020-06-04 12:11 p.m., Simon Marchi wrote:
>> On 2020-03-31 3:18 p.m., Scott Linder wrote:
>>> @@ -763,6 +760,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));
>>
>> This should be `memcmp (...) == 0`.  Currently, the function returns true when
>> the frame is not an outer frame id, which is the opposite of what it is supposed
>> to do.
>>
>> With this, the test gdb.base/break.exp on AArch64 runs fine.  I will launch a full
>> test run to see if there are any other problems.
>>
>> You can make the new function return "bool" instead of "int", and use true/false instead
>> of zero/non-zero (both in the code and comments).
>>
>> Simon
> 
> With this change, the full test run on AArch64 came out clean.  I'll try a try
> job on the buildbot, but I haven't had much success with it recently.
> 
> Simon
> 

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.

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

[Simon Marchi]

[-- 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

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

[Luis Machado]

On 6/8/20 1:01 PM, Simon Marchi wrote:
> 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'm running Ubuntu 18.04.4 LTS, kernel 4.12.13 on an aarch64 
Cortex-A72-based machine. Compiler is gcc version 7.5.0.

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

That's the change I did as well (otherwise gdb.base/break.exp would 
fail). I'm guessing there may be code generation differences that are 
triggering those issues.

I can take a look at the failure mode later, to provide some more 
information.