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

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Scott Linder
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
Reply | Threaded
Open this post in threaded view
|

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

Scott Linder
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, [hidden email] 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

0001-gdb-Support-frames-inlined-into-the-outer-frame.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Simon Marchi-4
On 2020-03-18 5:17 p.m., [hidden email] 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
Reply | Threaded
Open this post in threaded view
|

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

Scott Linder
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., [hidden email] 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
Reply | Threaded
Open this post in threaded view
|

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

Simon Marchi-4
On 2020-03-18 5:42 p.m., [hidden email] 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).
Reply | Threaded
Open this post in threaded view
|

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.

Reply | Threaded
Open this post in threaded view
|

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

Scott Linder
In reply to this post by Simon Marchi-4
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  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
Scott,

Thanks for looking into this.

* Scott Linder <[hidden email]> [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  <[hidden email]>
>
> * 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
Reply | Threaded
Open this post in threaded view
|

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

Scott Linder
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 <[hidden email]> [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  <[hidden email]>
>>
>> * 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
Reply | Threaded
Open this post in threaded view
|

[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  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
In reply to this post by Scott Linder
On 3/30/20 11:22 PM, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
In reply to this post by Scott Linder
* Scott Linder <[hidden email]> [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  <[hidden email]>
>
> * 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
>
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
In reply to this post by Scott Linder
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  <[hidden email]>
>
> * 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
>
Reply | Threaded
Open this post in threaded view
|

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

Scott Linder
In reply to this post by Sourceware - gdb-patches mailing list
On Thu, Apr 02, 2020 at 08:30:19PM +0100, Pedro Alves wrote:

> On 3/30/20 11:22 PM, [hidden email] 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
Reply | Threaded
Open this post in threaded view
|

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

Scott Linder
In reply to this post by Andrew Burgess
On Fri, Apr 03, 2020 at 06:00:31PM +0100, Andrew Burgess wrote:

> * Scott Linder <[hidden email]> [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  <[hidden email]>
> >
> > * 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
> >
Reply | Threaded
Open this post in threaded view
|

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

Scott Linder
In reply to this post by Sourceware - gdb-patches mailing list
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