[PATCH v2 0/3] Support frames inlined in outer frames

[PATCH v2 0/3] Support frames inlined in outer frames

[Simon Marchi]

This is essentially a v2 of this thread here:

    https://sourceware.org/pipermail/gdb-patches/2020-March/166786.html

In summary, this is what the patches do:

1. fixes a latent bug that triggered when trying to unwind a frame
   inlined in the outer frame
2. changes how outer frames are represented
3. allows frames inlined in outer frames, adds a test that runs on
   regular hardware

I don't see any regression on x86-64.  I tested previous iterations of
the series (with similar code) on aarch64 too and didn't see any
regression.

Scott Linder (1):
  gdb: support frames inlined into the outer frame

Simon Marchi (2):
  gdb: make frame_unwind_got_optimized return a not_lval value
  gdb: introduce explicit outer frame id kind

 gdb/frame-unwind.c                            |  14 +-
 gdb/frame-unwind.h                            |   3 +
 gdb/frame.c                                   |  21 +--
 gdb/frame.h                                   |   9 +-
 gdb/inline-frame.c                            |   4 -
 .../gdb.dwarf2/dw2-reg-undefined.exp          |  12 ++
 .../gdb.dwarf2/frame-inlined-in-outer-frame.S | 137 ++++++++++++++++++
 .../frame-inlined-in-outer-frame.exp          | 114 +++++++++++++++
 8 files changed, 281 insertions(+), 33 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.S
 create mode 100644 gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.exp

-- 
2.28.0

[PATCH v2 1/3] gdb: make frame_unwind_got_optimized return a not_lval value

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

TLDR: frame_unwind_got_optimized uses wrong frame id value, trying to
fix it makes GDB sad, return not_lval value and don't use frame id value
instead.

Longer version:

The `prev_register` method of the `frame_unwind` interface corresponds
to asking the question: "where did this frame - passed as a parameter -
save the value this register had in its caller frame?".  When "this
frame" did not save that register value (DW_CFA_undefined in DWARF), the
implementation can use the `frame_unwind_got_optimized` function to
create a struct value that represents the optimized out / not saved
register.

`frame_unwind_got_optimized` marks the value as fully optimized out,
sets the lval field to lval_register and assigns the required data for
lval_register: the next frame id and the register number.  The problem
is that it uses the frame id from the wrong frame (see below for in
depth explanation).  In practice, this is not problematic because the
frame id is never used: the value is already not lazy (and is marked as
optimized out), so the value is never fetched from the target.

When trying to change it to put the right next frame id in the value, we
bump into problems: computing the frame id for some frame requires
unwinding some register, if that register is not saved / optimized out,
we try to get the frame id that we are currently computing.

This patch addresses the problem by changing
`frame_unwind_got_optimized` to return a not_lval value instead.  Doing
so, we don't need to put a frame id, so we don't hit that problem.  It
may seem like an unnecessary change today, because it looks like we're
fixing something that is not broken (from the user point of view).
However, the bug becomes user visible with the following patches, where
inline frames are involved.  I put this change in its own patch to keep
it logically separate.

Let's now illustrate how we are putting the wrong frame id in the value
returned by `frame_unwind_got_optimized`.  Let's assume this stack:

    frame #0
    frame #1
    frame #2
    frame #3

Let's suppose that we are calling `frame_unwind_register_value` with
frame #2 as the "next_frame" parameter and some register number X as the
regnum parameter.  That is like asking the question "where did frame #2
save frame #3's value for register X".

`frame_unwind_register_value` calls the frame unwinder's `prev_register`
method, which in our case is `dwarf2_frame_prev_register`.  Note that in
`dwarf2_frame_prev_register`, the parameter is now called `this_frame`,
but its value is still frame #2, and we are still looking for where
frame #2 saved frame #3's value of register X.

Let's now suppose that frame #2's CFI explicitly indicates that the
register X is was not saved (DW_CFA_undefined).  We go into
`frame_unwind_got_optimized`.

In `frame_unwind_got_optimized`, the intent is to create a value that
represents register X in frame #3.  An lval_register value requires that
we specify the id of the _next_ frame, that is the frame from which we
would need to unwind in order to get the value.  Therefore, we would
want to put the id of frame #2 in there.

However, `frame_unwind_got_optimized` does:

    VALUE_NEXT_FRAME_ID (val)
      = get_frame_id (get_next_frame_sentinel_okay (frame));

where `frame` is frame #2.  The get_next_frame_sentinel_okay call
returns frame #1, so we end up putting frame #1's id in the value.

Let's now pretend that we try to "fix" it by placing the right frame id,
in other words doing this change:

    --- a/gdb/frame-unwind.c
    +++ b/gdb/frame-unwind.c
    @@ -260,8 +260,7 @@ frame_unwind_got_optimized (struct frame_info *frame, int regnum)
       mark_value_bytes_optimized_out (val, 0, TYPE_LENGTH (type));
       VALUE_LVAL (val) = lval_register;
       VALUE_REGNUM (val) = regnum;
    -  VALUE_NEXT_FRAME_ID (val)
    -    = get_frame_id (get_next_frame_sentinel_okay (frame));
    +  VALUE_NEXT_FRAME_ID (val) = get_frame_id (frame);
       return val;
     }

This makes some tests fails, such as gdb.dwarf2/dw2-undefined-ret-addr.exp,
like so:

    ...
    #9  0x0000557a8ab15a5d in internal_error (file=0x557a8b31ef80 "/home/simark/src/binutils-gdb/gdb/frame.c", line=623, fmt=0x557a8b31efe0 "%s: Assertion `%s' failed.") at /home/simark/src/binutils-gdb/gdbsupport/errors.cc:55
    #10 0x0000557a87f816d6 in get_frame_id (fi=0x62100034bde0) at /home/simark/src/binutils-gdb/gdb/frame.c:623
    #11 0x0000557a87f7cac7 in frame_unwind_got_optimized (frame=0x62100034bde0, regnum=16) at /home/simark/src/binutils-gdb/gdb/frame-unwind.c:264
    #12 0x0000557a87a71a76 in dwarf2_frame_prev_register (this_frame=0x62100034bde0, this_cache=0x62100034bdf8, regnum=16) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1267
    #13 0x0000557a87f86621 in frame_unwind_register_value (next_frame=0x62100034bde0, regnum=16) at /home/simark/src/binutils-gdb/gdb/frame.c:1288
    #14 0x0000557a87f855d5 in frame_register_unwind (next_frame=0x62100034bde0, regnum=16, optimizedp=0x7fff5f459070, unavailablep=0x7fff5f459080, lvalp=0x7fff5f4590a0, addrp=0x7fff5f4590b0, realnump=0x7fff5f459090, bufferp=0x7fff5f459150 "") at /home/simark/src/binutils-gdb/gdb/frame.c:1191
    #15 0x0000557a87f860ef in frame_unwind_register (next_frame=0x62100034bde0, regnum=16, buf=0x7fff5f459150 "") at /home/simark/src/binutils-gdb/gdb/frame.c:1247
    #16 0x0000557a881875f9 in i386_unwind_pc (gdbarch=0x621000190110, next_frame=0x62100034bde0) at /home/simark/src/binutils-gdb/gdb/i386-tdep.c:1971
    #17 0x0000557a87fe58a5 in gdbarch_unwind_pc (gdbarch=0x621000190110, next_frame=0x62100034bde0) at /home/simark/src/binutils-gdb/gdb/gdbarch.c:3062
    #18 0x0000557a87a6267b in dwarf2_tailcall_sniffer_first (this_frame=0x62100034bde0, tailcall_cachep=0x62100034bee0, entry_cfa_sp_offsetp=0x7fff5f4593f0) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame-tailcall.c:387
    #19 0x0000557a87a70cdf in dwarf2_frame_cache (this_frame=0x62100034bde0, this_cache=0x62100034bdf8) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1198
    #20 0x0000557a87a711c2 in dwarf2_frame_this_id (this_frame=0x62100034bde0, this_cache=0x62100034bdf8, this_id=0x62100034be40) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1226
    #21 0x0000557a87f81167 in compute_frame_id (fi=0x62100034bde0) at /home/simark/src/binutils-gdb/gdb/frame.c:587
    #22 0x0000557a87f81803 in get_frame_id (fi=0x62100034bde0) at /home/simark/src/binutils-gdb/gdb/frame.c:635
    #23 0x0000557a87f7efef in scoped_restore_selected_frame::scoped_restore_selected_frame (this=0x7fff5f459920) at /home/simark/src/binutils-gdb/gdb/frame.c:320
    #24 0x0000557a891488ae in print_frame_args (fp_opts=..., func=0x621000183b90, frame=0x62100034bde0, num=-1, stream=0x6030000caa20) at /home/simark/src/binutils-gdb/gdb/stack.c:750
    #25 0x0000557a8914e87a in print_frame (fp_opts=..., frame=0x62100034bde0, print_level=0, print_what=SRC_AND_LOC, print_args=1, sal=...) at /home/simark/src/binutils-gdb/gdb/stack.c:1394
    #26 0x0000557a8914c2ae in print_frame_info (fp_opts=..., frame=0x62100034bde0, print_level=0, print_what=SRC_AND_LOC, print_args=1, set_current_sal=1) at /home/simark/src/binutils-gdb/gdb/stack.c:1119
    ...

We end up calling get_frame_id (in the hunk above, frame #10)  while we are
computing it (frame #21), and that's not good.

Now, the question is how do we fix this.  I suggest making the unwinder
return a not_lval value in this case.

The reason why we return an lval_register here is to make sure that this
is printed as "not saved" and not "optimized out" down the line.  See
these two commits:

1. 901461f8eb40 ("Print registers not saved in the frame as "<not saved>"
   instead of "<optimized out>".").
2. 6bd273ae450b ("Make "set debug frame 1" output print <not saved> instead of
   <optimized out>.")

The current design (introduced by the first commit) is to check the
value's lval to choose which one to print (see val_print_optimized_out).

Making the unwinder return not_lval instead of lval_register doesn't
break "not saved" when doing "print $rax" or "info registers", because
value_fetch_lazy_register only consumes the contents and optimized-out
property from the value the unwinder returned.  The value being
un-lazified stays an lval_register.

I believe that this is a correct technical solution (and not just
papering over the problem), because what we expect of unwinders is to
tell us where a given register's value is saved.  If the value is saved
in memory, -> lval_memory.  If the value is saved in some other register
of the next frame, -> lval_register.  If the value is not saved, it
doesn't really make sense to return an lval_register value.  not_lval
would be more appropriate.  If the code then wants to represent an
optimized out register value (like value_fetch_lazy_register does), then
it's a separate concern which shouldn't involve the unwinder.

This change breaks the output of "set debug frame 1" though (introduced
by the second commit), since that logging statement consumes the return
value of the unwinder directly.  To keep the correct behavior, just make
`frame_unwind_register_value` call `val_print_not_saved` directly,
instead of `val_print_optimized_out`.  This is fine because we know in
this context that we are always talking about a register value, and that
we want to show "not saved" for those.

I augmented the gdb.dwarf2/dw2-reg-undefined.exp test case to test some
cases I stumbled on while working on this, which I think are not tested
anywhere:

- the "set debug frame 1" debug output mentioned above.  It's just debug
  output, but if we want to make sure it doesn't change, it should be
  tested
- printing not-saved register values from the history (should print not
  saved)
- copying a not-saved register value in a convenience variable.  In this
  case, we expect that printing the convenience variable shows
  "optimized out", because we copied the value, not the property of
  where the value came from.

gdb/ChangeLog:

	* frame-unwind.c (frame_unwind_got_optimized): Don't set
	regnum/frame in value.  Call allocate_value_lazy.
	* frame.c (frame_unwind_register_value): Use
	val_print_not_saved.

gdb/testsuite/ChangeLog:

	* gdb.dwarf2/dw2-reg-undefined.exp: Test "set debug frame 1"
	output, printing a "not saved" value from history and printing a
	convenience variable created from a "not saved" value.

Change-Id: If451739a3ef7a5b453b1f50707e21ce16d74807e
---
 gdb/frame-unwind.c                             | 14 ++------------
 gdb/frame-unwind.h                             |  3 +++
 gdb/frame.c                                    |  2 +-
 gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp | 12 ++++++++++++
 4 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
index 064f6ebebdaa..32124c5715a6 100644
--- a/gdb/frame-unwind.c
+++ b/gdb/frame-unwind.c
@@ -251,18 +251,8 @@ frame_unwind_got_optimized (struct frame_info *frame, int regnum)
 {
   struct gdbarch *gdbarch = frame_unwind_arch (frame);
   struct type *type = register_type (gdbarch, regnum);
-  struct value *val;
-
-  /* Return an lval_register value, so that we print it as
-     "<not saved>".  */
-  val = allocate_value_lazy (type);
-  set_value_lazy (val, 0);
-  mark_value_bytes_optimized_out (val, 0, TYPE_LENGTH (type));
-  VALUE_LVAL (val) = lval_register;
-  VALUE_REGNUM (val) = regnum;
-  VALUE_NEXT_FRAME_ID (val)
-    = get_frame_id (get_next_frame_sentinel_okay (frame));
-  return val;
+
+  return allocate_optimized_out_value (type);
 }
 
 /* Return a value which indicates that FRAME copied REGNUM into
diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
index c80ddc84f291..25a601ccf4bb 100644
--- a/gdb/frame-unwind.h
+++ b/gdb/frame-unwind.h
@@ -133,6 +133,9 @@ typedef void (frame_this_id_ftype) (struct frame_info *this_frame,
    may be a lazy reference to memory, a lazy reference to the value of
    a register in THIS frame, or a non-lvalue.
 
+   If the previous frame's register was not saved by THIS_FRAME and is
+   therefore undefined, return a not_lval wholly optimized-out value.
+
    THIS_PROLOGUE_CACHE can be used to share any prolog analysis data
    with the other unwind methods.  Memory for that cache should be
    allocated using FRAME_OBSTACK_ZALLOC().  */
diff --git a/gdb/frame.c b/gdb/frame.c
index 7ab3cdcdad41..ccaf97dc7e91 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1295,7 +1295,7 @@ frame_unwind_register_value (frame_info *next_frame, int regnum)
       if (value_optimized_out (value))
 	{
 	  fprintf_unfiltered (gdb_stdlog, " ");
-	  val_print_optimized_out (value, gdb_stdlog);
+	  val_print_not_saved (gdb_stdlog);
 	}
       else
 	{
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
index a3e6f28c54f0..4ab7e2dbfc44 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
@@ -74,3 +74,15 @@ for {set f 0} {$f < 3} {incr f} {
 		    "r9\\s+${pattern_r8_r9_info}\\s*"] \
 	"Check values of rax, rbx, rcx, r8, r9 in frame ${f}"
 }
+
+# Test that the debug log statement in frame_unwind_register_value produces
+# "not saved" and not# "optimized out".
+gdb_test "set debug frame 1"
+gdb_test {print $rax} {frame_unwind_register_value[^\r\n]+rax[^\r\n]+not saved.*}
+gdb_test "set debug frame 0"
+
+# Test that history values show "not saved" and not "optimized out".
+gdb_test "print" " = <not saved>"
+
+# Test that convenience variables _don't_ show "not saved".
+gdb_test {print $foo = $rax} " = <optimized out>"
-- 
2.28.0

[PATCH v2 2/3] gdb: introduce explicit outer frame id kind

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

In the following patch, we'll need to easily differentiate the frame_id
of the outer frame (or the frame id of a frame inlined into the outer
frame) from a simply invalid frame id.

Currently, the frame id of the outer frame has `stack_status` set to
FID_STACK_INVALID plus special_addr_p set.  A frame inlined into the
outer frame would also have `artificial_depth` set to greater than one.
That makes the job of differntiating the frame id of the outer frame (or a
frame inlined into the outer frame) cumbersome.

To make it easier, give the outer frame id its own frame_id_stack_status
enum value.  outer_frame_id then becomes very similar to
sentinel_frame_id, another "special" frame id value.

In frame_id_p, we don't need a special case for the outer frame id, as
it's no long a special case of FID_STACK_INVALID.  Same goes for
frame_id_eq.

So in the end, FID_STACK_OUTER isn't even used (except in
fprint_frame_id).  But that's expected: all the times we wanted to
identify an outer frame was to differentiate it from an otherwise
invalid frame.  Since their frame_id_stack_status value is different
now, that is done naturally.

gdb/ChangeLog:

	* frame.h (enum frame_id_stack_status) <FID_STACK_OUTER>: New.
	* frame.c (fprint_frame_id): Handle FID_STACK_OUTER.
	(outer_frame_id): Use FID_STACK_OUTER instead of
	FID_STACK_INVALID.
	(frame_id_p): Don't check for outer_frame_id.

Change-Id: I654e7f936349debc4f04f7f684b15e71a0c37619
---
 gdb/frame.c | 19 +++++--------------
 gdb/frame.h |  9 +++++++--
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index ccaf97dc7e91..54f4c613c9e8 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -396,8 +396,11 @@ fprint_frame_id (struct ui_file *file, struct frame_id id)
     fprintf_unfiltered (file, "stack=<unavailable>");
   else if (id.stack_status == FID_STACK_SENTINEL)
     fprintf_unfiltered (file, "stack=<sentinel>");
+  else if (id.stack_status == FID_STACK_OUTER)
+    fprintf_unfiltered (file, "stack=<outer>");
   else
     fprintf_unfiltered (file, "stack=%s", hex_string (id.stack_addr));
+
   fprintf_unfiltered (file, ",");
 
   fprint_field (file, "code", id.code_addr_p, id.code_addr);
@@ -672,7 +675,7 @@ frame_unwind_caller_id (struct frame_info *next_frame)
 
 const struct frame_id null_frame_id = { 0 }; /* All zeros.  */
 const struct frame_id sentinel_frame_id = { 0, 0, 0, FID_STACK_SENTINEL, 0, 1, 0 };
-const struct frame_id outer_frame_id = { 0, 0, 0, FID_STACK_INVALID, 0, 1, 0 };
+const struct frame_id outer_frame_id = { 0, 0, 0, FID_STACK_OUTER, 0, 1, 0 };
 
 struct frame_id
 frame_id_build_special (CORE_ADDR stack_addr, CORE_ADDR code_addr,
@@ -746,10 +749,6 @@ frame_id_p (frame_id l)
   /* The frame is valid iff it has a valid stack address.  */
   bool p = l.stack_status != FID_STACK_INVALID;
 
-  /* outer_frame_id is also valid.  */
-  if (!p && memcmp (&l, &outer_frame_id, sizeof (l)) == 0)
-    p = true;
-
   if (frame_debug)
     {
       fprintf_unfiltered (gdb_stdlog, "{ frame_id_p (l=");
@@ -774,15 +773,7 @@ frame_id_eq (frame_id l, frame_id r)
 {
   bool 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 = true;
-  else if (l.stack_status == FID_STACK_INVALID
+  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.
        Note that a frame ID is invalid iff it is the null frame ID.  */
diff --git a/gdb/frame.h b/gdb/frame.h
index 1c6afad1ae95..3ceb7b32effa 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -85,8 +85,7 @@ struct frame_print_options;
 
 enum frame_id_stack_status
 {
-  /* Stack address is invalid.  E.g., this frame is the outermost
-     (i.e., _start), and the stack hasn't been setup yet.  */
+  /* Stack address is invalid.  */
   FID_STACK_INVALID = 0,
 
   /* Stack address is valid, and is found in the stack_addr field.  */
@@ -95,6 +94,12 @@ enum frame_id_stack_status
   /* Sentinel frame.  */
   FID_STACK_SENTINEL = 2,
 
+  /* Outer frame.  Since a frame's stack address is typically defined as the
+     value the stack pointer had prior to the activation of the frame, an outer
+     frame doesn't have a stack address.  The frame ids of frames inlined in the
+     outer frame are also of this type.  */
+  FID_STACK_OUTER = 3,
+
   /* Stack address is unavailable.  I.e., there's a valid stack, but
      we don't know where it is (because memory or registers we'd
      compute it from were not collected).  */
-- 
2.28.0

[PATCH v2 3/3] gdb: support frames inlined into the outer frame

[Simon Marchi]

From: Scott Linder <scott@scottlinder.com>

Remove the restriction (gdb_assert) that prevents creating frames
inlined in the outer frame.  Like for frames inlined in a standard frame
(FID_STACK_VALID), a frame inlined into the outer frame will have:

 - artificial_depth greater than 0
 - code_addr equal to the first executed instruction in the block
   corresponding to the inlined function

It will however have its stack_status set to FID_STACK_OUTER, like the
outer frame.

This is not typically seen on your everyday system (e.g. a Linux /
x86-64 process), because the outer frame would be for instance the
_start function, probably written in assembly and very unlikely to have
anything inlined in it.  However this could happen in more "bare-metal"
scenarios.  In particular, this was seen in ROCm GDB [1], where the
compiler does inline functions in the top-level kernel functions (kernel
in the sense of compute kernel, not userspace vs kernel).

I however wrote a test that replicates the issue on x86-64 and a few
other arches I had access to.  Since we need to control precisely the
emitted DWARF CFI, I didn't find another way than to write it in
assembly.  The DWARF is generated using the testsuite's DWARF assembler,
except the unwind information, which is written using CFI directives
(and therefore generated by the actual assembler).  I think the test is
adequately commented, but if anything is unclear, just ask and I'll add
more info.

[1] https://github.com/ROCm-Developer-Tools/ROCgdb/

gdb/ChangeLog:

YYYY-MM-DD  Scott Linder  <scott@scottlinder.com>
YYYY-MM-DD  Simon Marchi  <simon.marchi@efficios.com>

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

gdb/testsuite/ChangeLog:

	* gdb.dwarf2/frame-inlined-in-outer-frame.exp: New file.
	* gdb.dwarf2/frame-inlined-in-outer-frame.S: New file.

Change-Id: I8aa129c667dccc31590ffdf426586418493a6ebe
---
 gdb/inline-frame.c                            |   4 -
 .../gdb.dwarf2/frame-inlined-in-outer-frame.S | 137 ++++++++++++++++++
 .../frame-inlined-in-outer-frame.exp          | 114 +++++++++++++++
 3 files changed, 251 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.S
 create mode 100644 gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.exp

diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
index 35ceb27d9c14..300b1224db03 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
diff --git a/gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.S b/gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.S
new file mode 100644
index 000000000000..63c4bc6382d2
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.S
@@ -0,0 +1,137 @@
+/* Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <asm/unistd.h>
+
+/* Define these for each architecture:
+
+   1) RETURN_ADDRESS_REGNO: The register number representing the return
+      address in the DWARF CFI.  It can be easily be looked up using
+      `readelf --debug-dump=frames-interp` on an existing binary of that
+      architecture, where it says `ra=X`.
+
+   2) exit_0: a sequence of instruction to execute the exit syscall with
+      argument 0.  */
+
+#if defined(__x86_64__)
+
+# define RETURN_ADDRESS_REGNO 16
+
+.macro exit_0
+	mov $__NR_exit, %rax
+	mov $0, %rdi
+	syscall
+.endm
+
+#elif defined(__i386__)
+
+# define RETURN_ADDRESS_REGNO 8
+
+.macro exit_0
+	mov $__NR_exit, %eax
+	mov $0, %ebx
+	int $0x80
+.endm
+
+#elif defined(__aarch64__)
+
+# define RETURN_ADDRESS_REGNO 30
+
+.macro exit_0
+	mov x0, #0
+	mov x8, #__NR_exit
+	svc #0
+.endm
+
+#elif defined(__arm__)
+
+# define RETURN_ADDRESS_REGNO 14
+
+.macro exit_0
+	ldr r7, =__NR_exit
+	ldr r0, =0
+	swi 0x0
+.endm
+
+#else
+# error "Unsupported architecture"
+#endif
+
+/* The following assembly program mimics this pseudo C program, where
+   everything has been inlined:
+
+    1 void bar(void) {
+    2   nop;
+    3 }
+    4
+    5 void foo(void) {
+    6   nop;
+    7   bar();
+    8   nop;
+    9 }
+   10
+   11 void _start(void) {
+   12   nop;
+   13   foo();
+   14   nop;
+   15   exit(0);
+   16 }
+*/
+
+.global _start
+_start:
+.cfi_startproc
+
+/* State that the return address for this frame is undefined. */
+.cfi_undefined RETURN_ADDRESS_REGNO
+
+.global __cu_low_pc
+__cu_low_pc:
+
+.global __start_low_pc
+__start_low_pc:
+	/* Line 12 */
+	nop
+
+.global __foo_low_pc
+__foo_low_pc:
+	/* Line 6 */
+	nop
+
+.global __bar_low_pc
+__bar_low_pc:
+	/* Line 2 */
+	nop
+
+.global __bar_high_pc
+__bar_high_pc:
+	/* Line 8 */
+	nop
+
+.global __foo_high_pc
+__foo_high_pc:
+	/* Line 14 */
+	nop
+
+	/* Line 15 */
+	exit_0
+
+.cfi_endproc
+
+.global __start_high_pc
+__start_high_pc:
+
+.global __cu_high_pc
+__cu_high_pc:
diff --git a/gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.exp b/gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.exp
new file mode 100644
index 000000000000..721b521e8306
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.exp
@@ -0,0 +1,114 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test unwinding when we have a frame inlined in the outer frame (in the sense
+# of frame.c:outer_frame_id).
+#
+# The conditions required to reproduce the original issue are:
+#
+#  1. Have an outer frame whose DWARF CFI explicitly says that the frame return
+#     address is undefined.
+#  2. A frame inlined in this other frame.
+#
+# Because of (1), the test has to be written in assembly with explicit CFI
+# directives.
+
+load_lib dwarf.exp
+
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile .S
+
+set dwarf_asm [standard_output_file dwarf-asm.S]
+Dwarf::assemble $dwarf_asm {
+    declare_labels foo_subprogram bar_subprogram
+    declare_labels stmt_list
+
+    # See the comment in the .S file for the equivalent C program this is meant
+    # to represent.
+
+    cu { addr_size 4 } {
+	DW_TAG_compile_unit {
+	    {DW_AT_name file1.txt}
+	    {DW_AT_stmt_list $stmt_list DW_FORM_sec_offset}
+	    {DW_AT_language @DW_LANG_C99}
+	    {DW_AT_low_pc __cu_low_pc DW_FORM_addr}
+	    {DW_AT_high_pc __cu_high_pc DW_FORM_addr}
+	} {
+	    DW_TAG_subprogram {
+		{DW_AT_name "_start"}
+		{DW_AT_low_pc __start_low_pc DW_FORM_addr}
+		{DW_AT_high_pc __start_high_pc DW_FORM_addr}
+	    } {
+		DW_TAG_inlined_subroutine {
+		    {DW_AT_abstract_origin :$foo_subprogram}
+		    {DW_AT_low_pc __foo_low_pc DW_FORM_addr}
+		    {DW_AT_high_pc __foo_high_pc DW_FORM_addr}
+		    {DW_AT_call_file 1 DW_FORM_data1}
+		    {DW_AT_call_line 13 DW_FORM_data1}
+		} {
+		    DW_TAG_inlined_subroutine {
+			{DW_AT_abstract_origin :$bar_subprogram}
+			{DW_AT_low_pc __bar_low_pc DW_FORM_addr}
+			{DW_AT_high_pc __bar_high_pc DW_FORM_addr}
+			{DW_AT_call_file 1 DW_FORM_data1}
+			{DW_AT_call_line 7 DW_FORM_data1}
+		    }
+		}
+	    }
+
+	    foo_subprogram: DW_TAG_subprogram {
+		{DW_AT_name "foo"}
+		{DW_AT_prototyped 1 DW_FORM_flag_present}
+		{DW_AT_inline 0x1 DW_FORM_data1}
+	    }
+
+	    bar_subprogram: DW_TAG_subprogram {
+		{DW_AT_name "bar"}
+		{DW_AT_prototyped 1 DW_FORM_flag_present}
+		{DW_AT_inline 0x1 DW_FORM_data1}
+	    }
+	}
+    }
+
+    lines { } stmt_list {
+	global srcdir subdir srcfile
+
+	include_dir "/some/directory"
+	file_name "/some/directory/file.c" 0
+    }
+}
+
+if { [build_executable ${testfile}.exp ${testfile} "$srcfile $dwarf_asm" \
+      {additional_flags=-nostdlib additional_flags=-static}] != 0 } {
+    untested "failed to compile"
+    return
+}
+
+clean_restart $binfile
+
+if { [gdb_starti_cmd] != 0 } {
+    fail "failed to run to first instruction"
+    return
+}
+
+gdb_test "frame" "in _start .*"
+
+gdb_test "stepi" "in foo .*" "step into foo"
+gdb_test "stepi" "in bar .*" "step into bar"
+gdb_test "stepi" "in foo .*" "step back into foo"
+gdb_test "stepi" "in _start .*" "step back into _start"
-- 
2.28.0

Re: [PATCH v2 1/3] gdb: make frame_unwind_got_optimized return a not_lval value

[Pedro Alves]

On 8/27/20 9:57 PM, Simon Marchi via Gdb-patches wrote:

>  /* Return a value which indicates that FRAME copied REGNUM into
> diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
> index c80ddc84f291..25a601ccf4bb 100644
> --- a/gdb/frame-unwind.h
> +++ b/gdb/frame-unwind.h
> @@ -133,6 +133,9 @@ typedef void (frame_this_id_ftype) (struct frame_info *this_frame,
>     may be a lazy reference to memory, a lazy reference to the value of
>     a register in THIS frame, or a non-lvalue.
>  
> +   If the previous frame's register was not saved by THIS_FRAME and is
> +   therefore undefined, return a not_lval wholly optimized-out value.

Super nit:

"not_lval wholly optimized-out value" doesn't sound as natural
to me as "wholly optimized-out not_lval value" would.

It's just like "register optimized-out value" doesn't sound as
natural as "optimized-out register value" to me.

English adjective order is fun! 
https://www.theguardian.com/commentisfree/2016/sep/13/sentence-order-adjectives-rule-elements-of-eloquence-dictionary
https://www.grammar-monster.com/lessons/order_of_adjectives.htm

> +
>     THIS_PROLOGUE_CACHE can be used to share any prolog analysis data
>     with the other unwind methods.  Memory for that cache should be
>     allocated using FRAME_OBSTACK_ZALLOC().  */
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 7ab3cdcdad41..ccaf97dc7e91 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1295,7 +1295,7 @@ frame_unwind_register_value (frame_info *next_frame, int regnum)
>        if (value_optimized_out (value))
>  	{
>  	  fprintf_unfiltered (gdb_stdlog, " ");
> -	  val_print_optimized_out (value, gdb_stdlog);
> +	  val_print_not_saved (gdb_stdlog);
>  	}
>        else
>  	{
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
> index a3e6f28c54f0..4ab7e2dbfc44 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
> @@ -74,3 +74,15 @@ for {set f 0} {$f < 3} {incr f} {
>  		    "r9\\s+${pattern_r8_r9_info}\\s*"] \
>  	"Check values of rax, rbx, rcx, r8, r9 in frame ${f}"
>  }
> +
> +# Test that the debug log statement in frame_unwind_register_value produces
> +# "not saved" and not# "optimized out".

Spurious # after "not".

Re: [PATCH v2 2/3] gdb: introduce explicit outer frame id kind

[Pedro Alves]

On 8/27/20 9:57 PM, Simon Marchi via Gdb-patches wrote:

> -  else if (l.stack_status == FID_STACK_INVALID
> +  if (l.stack_status == FID_STACK_INVALID
>  	   || r.stack_status == FID_STACK_INVALID)

The indentation of that second line in the predicate should be
fixed, I think.

Re: [PATCH v2 0/3] Support frames inlined in outer frames

[Pedro Alves]

On 8/27/20 9:57 PM, Simon Marchi via Gdb-patches wrote:
> This is essentially a v2 of this thread here:
> 
>     https://sourceware.org/pipermail/gdb-patches/2020-March/166786.html
> 
> In summary, this is what the patches do:
> 
> 1. fixes a latent bug that triggered when trying to unwind a frame
>    inlined in the outer frame
> 2. changes how outer frames are represented
> 3. allows frames inlined in outer frames, adds a test that runs on
>    regular hardware
> 
> I don't see any regression on x86-64.  I tested previous iterations of
> the series (with similar code) on aarch64 too and didn't see any
> regression.

This all LGTM.  Feel free to push with the nits I pointed out on
patches #1 and #2 addressed.

For the record, I was involved in discussions about this with Simon and
the AMD folks as this went through multiple potential solutions before
settling here.  I think this ended up being quite nice and clean.

Thanks,
Pedro Alves

Re: [PATCH v2 0/3] Support frames inlined in outer frames

[Andrew Burgess]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2020-08-27 16:57:21 -0400]:

> This is essentially a v2 of this thread here:
> 
>     https://sourceware.org/pipermail/gdb-patches/2020-March/166786.html
> 
> In summary, this is what the patches do:
> 
> 1. fixes a latent bug that triggered when trying to unwind a frame
>    inlined in the outer frame
> 2. changes how outer frames are represented
> 3. allows frames inlined in outer frames, adds a test that runs on
>    regular hardware
> 
> I don't see any regression on x86-64.  I tested previous iterations of
> the series (with similar code) on aarch64 too and didn't see any
> regression.

I took a look through all these patches, and they all look good to me.

Thanks,
Andrew


> 
> Scott Linder (1):
>   gdb: support frames inlined into the outer frame
> 
> Simon Marchi (2):
>   gdb: make frame_unwind_got_optimized return a not_lval value
>   gdb: introduce explicit outer frame id kind
> 
>  gdb/frame-unwind.c                            |  14 +-
>  gdb/frame-unwind.h                            |   3 +
>  gdb/frame.c                                   |  21 +--
>  gdb/frame.h                                   |   9 +-
>  gdb/inline-frame.c                            |   4 -
>  .../gdb.dwarf2/dw2-reg-undefined.exp          |  12 ++
>  .../gdb.dwarf2/frame-inlined-in-outer-frame.S | 137 ++++++++++++++++++
>  .../frame-inlined-in-outer-frame.exp          | 114 +++++++++++++++
>  8 files changed, 281 insertions(+), 33 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.S
>  create mode 100644 gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.exp
> 
> -- 
> 2.28.0
> 

Re: [PATCH v2 0/3] Support frames inlined in outer frames

[Simon Marchi]

On 2020-08-28 4:50 a.m., Andrew Burgess wrote:
> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2020-08-27 16:57:21 -0400]:
> 
>> This is essentially a v2 of this thread here:
>>
>>     https://sourceware.org/pipermail/gdb-patches/2020-March/166786.html
>>
>> In summary, this is what the patches do:
>>
>> 1. fixes a latent bug that triggered when trying to unwind a frame
>>    inlined in the outer frame
>> 2. changes how outer frames are represented
>> 3. allows frames inlined in outer frames, adds a test that runs on
>>    regular hardware
>>
>> I don't see any regression on x86-64.  I tested previous iterations of
>> the series (with similar code) on aarch64 too and didn't see any
>> regression.
> 
> I took a look through all these patches, and they all look good to me.
> 
> Thanks,
> Andrew

Thanks, I merged them.  I forgot to fix the nits pointed out by Pedro before merging, so I pushed another patch on top to do so:


From f3bd50f1984e83e6abf5e971c56ce8fac3e936db Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Mon, 31 Aug 2020 13:31:01 -0400
Subject: [PATCH] gdb: fix nits in previous patches

I forgot to fix some nits pointed out in review before merging the
"frame inlined in outer frame series", this patch fixes them.

gdb/ChangeLog:

	* frame-unwind.h (frame_prev_register_ftype): Fix adjective
	ordering in comment.
	* frame.c (frame_id_eq): Fix indentation.

gdb/testsuite/ChangeLog:

	* gdb.dwarf2/dw2-reg-undefined.exp: Remove spurious #.

Change-Id: Iaddde9677fc3f68382558d1a16f5a0b4beb78bac
---
 gdb/ChangeLog                                  | 6 ++++++
 gdb/frame-unwind.h                             | 2 +-
 gdb/frame.c                                    | 2 +-
 gdb/testsuite/ChangeLog                        | 4 ++++
 gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp | 2 +-
 5 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 9637e4a7b12d..cc998d5ee585 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2020-08-31  Simon Marchi  <simon.marchi@efficios.com>
+
+	* frame-unwind.h (frame_prev_register_ftype): Fix adjective
+	ordering in comment.
+	* frame.c (frame_id_eq): Fix indentation.
+
 2020-08-31  Scott Linder  <scott@scottlinder.com>
 	    Simon Marchi  <simon.marchi@efficios.com>

diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
index 25a601ccf4bb..6cc1a888861e 100644
--- a/gdb/frame-unwind.h
+++ b/gdb/frame-unwind.h
@@ -134,7 +134,7 @@ typedef void (frame_this_id_ftype) (struct frame_info *this_frame,
    a register in THIS frame, or a non-lvalue.

    If the previous frame's register was not saved by THIS_FRAME and is
-   therefore undefined, return a not_lval wholly optimized-out value.
+   therefore undefined, return a wholly optimized-out not_lval value.

    THIS_PROLOGUE_CACHE can be used to share any prolog analysis data
    with the other unwind methods.  Memory for that cache should be
diff --git a/gdb/frame.c b/gdb/frame.c
index 54f4c613c9e8..0b708e668272 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -774,7 +774,7 @@ frame_id_eq (frame_id l, frame_id r)
   bool eq;

   if (l.stack_status == FID_STACK_INVALID
-	   || r.stack_status == FID_STACK_INVALID)
+      || r.stack_status == FID_STACK_INVALID)
     /* Like a NaN, if either ID is invalid, the result is false.
        Note that a frame ID is invalid iff it is the null frame ID.  */
     eq = false;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 66f1fe8533f2..148f31d26dd6 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2020-08-31  Simon Marchi  <simon.marchi@efficios.com>
+
+	* gdb.dwarf2/dw2-reg-undefined.exp: Remove spurious #.
+
 2020-08-31  Simon Marchi  <simon.marchi@efficios.com>

 	* gdb.dwarf2/frame-inlined-in-outer-frame.exp: New file.
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
index 4ab7e2dbfc44..75ea1f7b88ac 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
@@ -76,7 +76,7 @@ for {set f 0} {$f < 3} {incr f} {
 }

 # Test that the debug log statement in frame_unwind_register_value produces
-# "not saved" and not# "optimized out".
+# "not saved" and not "optimized out".
 gdb_test "set debug frame 1"
 gdb_test {print $rax} {frame_unwind_register_value[^\r\n]+rax[^\r\n]+not saved.*}
 gdb_test "set debug frame 0"
-- 
2.26.2

[committed][gdb/testsuite] Fix gdb.dwarf2/frame-inlined-in-outer-frame.exp

[Tom de Vries]

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

[ was: Re: [PATCH v2 3/3] gdb: support frames inlined into the outer frame ]

On 8/27/20 10:57 PM, Simon Marchi via Gdb-patches wrote:
> +clean_restart $binfile
> +
> +if { [gdb_starti_cmd] != 0 } {
> +    fail "failed to run to first instruction"
> +    return
> +}
> +
> +gdb_test "frame" "in _start .*"
> +
> +gdb_test "stepi" "in foo .*" "step into foo"

Patch below committed.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Fix-gdb.dwarf2-frame-inlined-in-outer-frame.exp.patch --]
[-- Type: text/x-patch, Size: 1493 bytes --]

[gdb/testsuite] Fix gdb.dwarf2/frame-inlined-in-outer-frame.exp

I'm running into the following FAIL:
...
(gdb) starti ^M
Starting program: frame-inlined-in-outer-frame frame^M
^M
^M
Program stopped.^M
0x0000000000401000 in _start ()^M
(gdb) PASS: gdb.dwarf2/frame-inlined-in-outer-frame.exp: frame
frame^M
(gdb) FAIL: gdb.dwarf2/frame-inlined-in-outer-frame.exp: step into foo
stepi^M
0x0000000000401001 in foo ()^M
...

The problem is that the .exp file issues a gdb_starti_cmd without consuming
the resulting prompt.  Consequently, the gdb_test issuing the frame command
consumes that prompt, and things are out-of-sync from that point onwards.

Fix this by consuming the gdb prompt after gdb_starti_cmd.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

2020-09-08  Tom de Vries  <tdevries@suse.de>

	* gdb.dwarf2/frame-inlined-in-outer-frame.exp: Consume gdb prompt
	after gdb_starti_cmd.

---
 gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.exp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.exp b/gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.exp
index 721b521e83..0ded2c0e76 100644
--- a/gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.exp
+++ b/gdb/testsuite/gdb.dwarf2/frame-inlined-in-outer-frame.exp
@@ -105,6 +105,7 @@ if { [gdb_starti_cmd] != 0 } {
     fail "failed to run to first instruction"
     return
 }
+gdb_test "" "Program stopped.*" "starti prompt"
 
 gdb_test "frame" "in _start .*"