[2/6] gdb: make it possible to restore selected user-created frames

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

I would like to improve frame_info_ptr to automatically grab the
information needed to reinflate a frame, and automatically reinflate it
as needed.  One thing that is in the way is the fact that some frames
can be created out of thin air by the create_new_frame function.  These
frames are not the fruit of unwinding from the target's current frame.
These frames are created by the "select-frame view" command.

The reinflation code works by saving the given frame's id (with an
exception for frame #0).  When reinflating, it unwinds from the target's
current frame, looking for a frame with that id.  Since user-created
frames created by create_new_frame can't be "re-found" through
unwinding, frame_info_ptr wouldn't be able to reinflate such a frame
today.  And since frame_info_ptr::reinflate asserts that m_ptr is not
nullptr on exit, I guess GDB would crash.

The same problem exists when saving and restoring the selected frame if
the selected frame is a user-created one (except the crash part).
save_selected_frame, restore_selected_frame and lookup_selected_frame
(all used in the process of saving and resoring the selected frame)
don't know how to handle user-created frames.  This can be observed
here, using the test included in this patch:

    $ ./gdb --data-directory=data-directory -nx -q testsuite/outputs/gdb.base/frame-view/frame-view
    Reading symbols from testsuite/outputs/gdb.base/frame-view/frame-view...
    (gdb) break thread_func
    Breakpoint 1 at 0x11a2: file /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c, line 42.
    (gdb) run
    Starting program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/frame-view/frame-view

    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/usr/lib/../lib/libthread_db.so.1".
    [New Thread 0x7ffff7cc46c0 (LWP 4171134)]
    [Switching to Thread 0x7ffff7cc46c0 (LWP 4171134)]

    Thread 2 "frame-view" hit Breakpoint 1, thread_func (p=0x0) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:42
    42        foo (11);
    (gdb) info frame
    Stack level 0, frame at 0x7ffff7cc3ee0:
     rip = 0x5555555551a2 in thread_func (/home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:42); saved rip = 0x7ffff7d4e8fd
     called by frame at 0x7ffff7cc3f80
     source language c.
     Arglist at 0x7ffff7cc3ed0, args: p=0x0
     Locals at 0x7ffff7cc3ed0, Previous frame's sp is 0x7ffff7cc3ee0
     Saved registers:
      rbp at 0x7ffff7cc3ed0, rip at 0x7ffff7cc3ed8
    (gdb) thread 1
    [Switching to thread 1 (Thread 0x7ffff7cc5740 (LWP 4171122))]
    #0  0x00007ffff7d4b4b6 in ?? () from /usr/lib/libc.so.6

Here, we create a custom frame for thread 1 (using the stack from thread
2, for convenience):

    (gdb) select-frame view 0x7ffff7cc3f80 0x5555555551a2

The first calls to "frame" looks good:

    (gdb) frame
    #0  thread_func (p=0x7ffff7d4e630) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:42
    42        foo (11);

But not the second one:

    (gdb) frame
    #0  0x00007ffff7d4b4b6 in ?? () from /usr/lib/libc.so.6

This second "frame" command shows the original frame instead of the
user-created frame.

It's not totally clear how the "select-frame view" feature is expected
to behave, especially since it's not tested.  I heard accounts that it
used to be possible to select a frame like this and do "up" and "down"
to navigate the backtrace starting from that frame.  But that doesn't
work today:

    (gdb) select-frame view 0x7ffff7cc3f80 0x5555555551a2
    (gdb) up
    Initial frame selected; you cannot go up.
    (gdb) down
    Bottom (innermost) frame selected; you cannot go down.

and "backtrace" always shows the actual thread's backtrace, it ignores
the user-created frame:

    (gdb) bt
    #0  0x00007ffff7d4b4b6 in ?? () from /usr/lib/libc.so.6
    #1  0x00007ffff7d50403 in ?? () from /usr/lib/libc.so.6
    #2  0x000055555555521a in main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:56

However, I think we can agree that the "frame" command changing the
selected frame, as shown above, is a bug.  I would expect that command
to show the currently selected frame and not change it.

This happens because the scoped_restore_selected_frame object in
print_frame_args.  frame is saved here, and restored in the destructor
of scoped_restore_selected_frame:

    #0  save_selected_frame (frame_id=0x7ffdc0020ad0, frame_level=0x7ffdc0020af0) at /home/simark/src/binutils-gdb/gdb/frame.c:1682
    #1  0x00005631390242f0 in scoped_restore_selected_frame::scoped_restore_selected_frame (this=0x7ffdc0020ad0) at /home/simark/src/binutils-gdb/gdb/frame.c:324
    #2  0x000056313993581e in print_frame_args (fp_opts=..., func=0x62100023bde0, frame=..., num=-1, stream=0x60b000000300) at /home/simark/src/binutils-gdb/gdb/stack.c:755
    #3  0x000056313993ad49 in print_frame (fp_opts=..., frame=..., print_level=1, print_what=SRC_AND_LOC, print_args=1, sal=...) at /home/simark/src/binutils-gdb/gdb/stack.c:1401
    #4  0x000056313993835d in print_frame_info (fp_opts=..., frame=..., print_level=1, print_what=SRC_AND_LOC, print_args=1, set_current_sal=1) at /home/simark/src/binutils-gdb/gdb/stack.c:1126
    #5  0x0000563139932e0b in print_stack_frame (frame=..., print_level=1, print_what=SRC_AND_LOC, set_current_sal=1) at /home/simark/src/binutils-gdb/gdb/stack.c:368
    #6  0x0000563139932bbe in print_stack_frame_to_uiout (uiout=0x611000016840, frame=..., print_level=1, print_what=SRC_AND_LOC, set_current_sal=1) at /home/simark/src/binutils-gdb/gdb/stack.c:346
    #7  0x0000563139b0641e in print_selected_thread_frame (uiout=0x611000016840, selection=...) at /home/simark/src/binutils-gdb/gdb/thread.c:1993
    #8  0x0000563139940b7f in frame_command_core (fi=..., ignored=true) at /home/simark/src/binutils-gdb/gdb/stack.c:1871
    #9  0x000056313994db9e in frame_command_helper<frame_command_core>::base_command (arg=0x0, from_tty=1) at /home/simark/src/binutils-gdb/gdb/stack.c:1976

Since the user-created frame has level 0 (identified by the saved level
-1), lookup_selected_frame just reselects the target's current frame,
and the user-created frame is lost.

My goal here is not to fix all the "select-frame view"-related use
cases, but to fix this particular problem and then (in a subsequent
patch) use the same approach to make frame_info_ptr capable of
reinflating user-created frames, and then make frame_info_ptr automatic.

So, this patch adds the frame_info::is_user_created field and sets it in
create_new_frame (the field is zeroed-out by default by the other place
that instantiates frames, get_prev_frame_raw).  When selecting such a
frame, we remember through the selected_frame_level/select_frame_id pair
that the select frame is a user-created one (more on this below).  When
saving (save_selected_frame) and restoring (restore_selected_frame) the
selected frame, that pair is simply copied out and copied in.  Nothing
really needs to change here, except removing an assert in
restore_selected_frame.  After a restore, it's lookup_selected_frame's
job (if called) to re-instantiate a frame_info from the information in
that pair.  Add a bit of code there to handle the case of user-created
frames, which means calling create_new_frame again with the right
addresses.  The stack address and pc happen to be saved in the frame_id
itself, so it's quite straightforward.

Now, the question is, how to identify a user-created frame through the
selected_frame_level/selected_frame_id pair.  I initially added a
separate "selected_frame_is_user_created" flag to that pair.  Then it
seemed simpler to make it so user-created frames would be identified by
the selected_frame_level == 0.  User-created frames already happen to
have level 0.  And remember that when the saved frame is the current
target frame, selected_frame_level is -1.  We need to modify
select_frame in any case to make it so it will save the frame id and use
the saved level 0 for user-created frames, despite them having level
0.  So just with that, it's possible to distinguish user-created frames,
no need to add a separate flag.  It results in:

 - if selected_frame_level is -1: the selected frame is the current
   frame
 - if selected_frame_level is 0: the selected frame is a user-created
   one
 - if selected_frame_level is > 0: the selected frame is unwound from
   the current frame

I'm not opposed to adding a separate flag to make it extra clear, but I
think this is clear enough, especially since it's contained in frame.c
(and soon frame_info_ptr), the rest of GDB doesn't need to know about
this convention.  And adding a separate flag adds risks of having
invalid states between the saved frame fields.

Add a minimal test case for the case described above, that is the
"select-frame view" command followed by the "frame" command twice.  In
order to have a known stack frame to switch to, the test spawns a second
thread, and tells the first thread to use the other thread's top frame.

Change-Id: Ifc77848dc465fbd21324b9d44670833e09fe98c7
---
 gdb/frame.c                           | 36 +++++++++++--
 gdb/frame.h                           |  4 ++
 gdb/testsuite/gdb.base/frame-view.c   | 74 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/frame-view.exp | 68 ++++++++++++++++++++++++
 4 files changed, 177 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/frame-view.c
 create mode 100644 gdb/testsuite/gdb.base/frame-view.exp
  

On 02/12/2022 19:00, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
>
> I would like to improve frame_info_ptr to automatically grab the
> information needed to reinflate a frame, and automatically reinflate it
> as needed.  One thing that is in the way is the fact that some frames
> can be created out of thin air by the create_new_frame function.  These
> frames are not the fruit of unwinding from the target's current frame.
> These frames are created by the "select-frame view" command.
>
> The reinflation code works by saving the given frame's id (with an
> exception for frame #0).  When reinflating, it unwinds from the target's
> current frame, looking for a frame with that id.  Since user-created
> frames created by create_new_frame can't be "re-found" through
> unwinding, frame_info_ptr wouldn't be able to reinflate such a frame
> today.  And since frame_info_ptr::reinflate asserts that m_ptr is not
> nullptr on exit, I guess GDB would crash.
>
> The same problem exists when saving and restoring the selected frame if
> the selected frame is a user-created one (except the crash part).
> save_selected_frame, restore_selected_frame and lookup_selected_frame
> (all used in the process of saving and resoring the selected frame)
> don't know how to handle user-created frames.  This can be observed
> here, using the test included in this patch:
>
>      $ ./gdb --data-directory=data-directory -nx -q testsuite/outputs/gdb.base/frame-view/frame-view
>      Reading symbols from testsuite/outputs/gdb.base/frame-view/frame-view...
>      (gdb) break thread_func
>      Breakpoint 1 at 0x11a2: file /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c, line 42.
>      (gdb) run
>      Starting program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/frame-view/frame-view
>
>      [Thread debugging using libthread_db enabled]
>      Using host libthread_db library "/usr/lib/../lib/libthread_db.so.1".
>      [New Thread 0x7ffff7cc46c0 (LWP 4171134)]
>      [Switching to Thread 0x7ffff7cc46c0 (LWP 4171134)]
>
>      Thread 2 "frame-view" hit Breakpoint 1, thread_func (p=0x0) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:42
>      42        foo (11);
>      (gdb) info frame
>      Stack level 0, frame at 0x7ffff7cc3ee0:
>       rip = 0x5555555551a2 in thread_func (/home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:42); saved rip = 0x7ffff7d4e8fd
>       called by frame at 0x7ffff7cc3f80
>       source language c.
>       Arglist at 0x7ffff7cc3ed0, args: p=0x0
>       Locals at 0x7ffff7cc3ed0, Previous frame's sp is 0x7ffff7cc3ee0
>       Saved registers:
>        rbp at 0x7ffff7cc3ed0, rip at 0x7ffff7cc3ed8
>      (gdb) thread 1
>      [Switching to thread 1 (Thread 0x7ffff7cc5740 (LWP 4171122))]
>      #0  0x00007ffff7d4b4b6 in ?? () from /usr/lib/libc.so.6
>
> Here, we create a custom frame for thread 1 (using the stack from thread
> 2, for convenience):
>
>      (gdb) select-frame view 0x7ffff7cc3f80 0x5555555551a2
>
> The first calls to "frame" looks good:
>
>      (gdb) frame
>      #0  thread_func (p=0x7ffff7d4e630) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:42
>      42        foo (11);
>
> But not the second one:
>
>      (gdb) frame
>      #0  0x00007ffff7d4b4b6 in ?? () from /usr/lib/libc.so.6
>
> This second "frame" command shows the original frame instead of the
> user-created frame.
>
> It's not totally clear how the "select-frame view" feature is expected
> to behave, especially since it's not tested.  I heard accounts that it
> used to be possible to select a frame like this and do "up" and "down"
> to navigate the backtrace starting from that frame.  But that doesn't
> work today:
>
>      (gdb) select-frame view 0x7ffff7cc3f80 0x5555555551a2
>      (gdb) up
>      Initial frame selected; you cannot go up.
>      (gdb) down
>      Bottom (innermost) frame selected; you cannot go down.
>
> and "backtrace" always shows the actual thread's backtrace, it ignores
> the user-created frame:
>
>      (gdb) bt
>      #0  0x00007ffff7d4b4b6 in ?? () from /usr/lib/libc.so.6
>      #1  0x00007ffff7d50403 in ?? () from /usr/lib/libc.so.6
>      #2  0x000055555555521a in main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:56
>
> However, I think we can agree that the "frame" command changing the
> selected frame, as shown above, is a bug.  I would expect that command
> to show the currently selected frame and not change it.
>
> This happens because the scoped_restore_selected_frame object in
> print_frame_args.  frame is saved here, and restored in the destructor
> of scoped_restore_selected_frame:
>
>      #0  save_selected_frame (frame_id=0x7ffdc0020ad0, frame_level=0x7ffdc0020af0) at /home/simark/src/binutils-gdb/gdb/frame.c:1682
>      #1  0x00005631390242f0 in scoped_restore_selected_frame::scoped_restore_selected_frame (this=0x7ffdc0020ad0) at /home/simark/src/binutils-gdb/gdb/frame.c:324
>      #2  0x000056313993581e in print_frame_args (fp_opts=..., func=0x62100023bde0, frame=..., num=-1, stream=0x60b000000300) at /home/simark/src/binutils-gdb/gdb/stack.c:755
>      #3  0x000056313993ad49 in print_frame (fp_opts=..., frame=..., print_level=1, print_what=SRC_AND_LOC, print_args=1, sal=...) at /home/simark/src/binutils-gdb/gdb/stack.c:1401
>      #4  0x000056313993835d in print_frame_info (fp_opts=..., frame=..., print_level=1, print_what=SRC_AND_LOC, print_args=1, set_current_sal=1) at /home/simark/src/binutils-gdb/gdb/stack.c:1126
>      #5  0x0000563139932e0b in print_stack_frame (frame=..., print_level=1, print_what=SRC_AND_LOC, set_current_sal=1) at /home/simark/src/binutils-gdb/gdb/stack.c:368
>      #6  0x0000563139932bbe in print_stack_frame_to_uiout (uiout=0x611000016840, frame=..., print_level=1, print_what=SRC_AND_LOC, set_current_sal=1) at /home/simark/src/binutils-gdb/gdb/stack.c:346
>      #7  0x0000563139b0641e in print_selected_thread_frame (uiout=0x611000016840, selection=...) at /home/simark/src/binutils-gdb/gdb/thread.c:1993
>      #8  0x0000563139940b7f in frame_command_core (fi=..., ignored=true) at /home/simark/src/binutils-gdb/gdb/stack.c:1871
>      #9  0x000056313994db9e in frame_command_helper<frame_command_core>::base_command (arg=0x0, from_tty=1) at /home/simark/src/binutils-gdb/gdb/stack.c:1976
>
> Since the user-created frame has level 0 (identified by the saved level
> -1), lookup_selected_frame just reselects the target's current frame,
> and the user-created frame is lost.
>
> My goal here is not to fix all the "select-frame view"-related use
> cases, but to fix this particular problem and then (in a subsequent
> patch) use the same approach to make frame_info_ptr capable of
> reinflating user-created frames, and then make frame_info_ptr automatic.
>
> So, this patch adds the frame_info::is_user_created field and sets it in
> create_new_frame (the field is zeroed-out by default by the other place
> that instantiates frames, get_prev_frame_raw).  When selecting such a
> frame, we remember through the selected_frame_level/select_frame_id pair
> that the select frame is a user-created one (more on this below).  When
> saving (save_selected_frame) and restoring (restore_selected_frame) the
> selected frame, that pair is simply copied out and copied in.  Nothing
> really needs to change here, except removing an assert in
> restore_selected_frame.  After a restore, it's lookup_selected_frame's
> job (if called) to re-instantiate a frame_info from the information in
> that pair.  Add a bit of code there to handle the case of user-created
> frames, which means calling create_new_frame again with the right
> addresses.  The stack address and pc happen to be saved in the frame_id
> itself, so it's quite straightforward.
I agree with everything you said up to here.
>
> Now, the question is, how to identify a user-created frame through the
> selected_frame_level/selected_frame_id pair.  I initially added a
> separate "selected_frame_is_user_created" flag to that pair.  Then it
> seemed simpler to make it so user-created frames would be identified by
> the selected_frame_level == 0.  User-created frames already happen to
> have level 0.  And remember that when the saved frame is the current
> target frame, selected_frame_level is -1.  We need to modify
> select_frame in any case to make it so it will save the frame id and use
> the saved level 0 for user-created frames, despite them having level
> 0.  So just with that, it's possible to distinguish user-created frames,
> no need to add a separate flag.  It results in:

But this doesn't necessarily sound correct if we're supposed to be able 
to go up and down on user created frames. My best guess for such a 
feature would be improving the debugging experience of optimized 
programs or reverse engineering something. If you were able to identify 
that something is an inlined function, for instance, you could make an 
artificial frame to keep track of this information, so that future 
executions (or just moving up and down the stack) was easier.  If that 
guess is correct, it would be possible to have a user created frame at a 
level that isn't level 0, and the detection method fails.

I could be entirely wrong in this assumption, though, and if that is the 
case, your solution makes perfect sense.

>
>   - if selected_frame_level is -1: the selected frame is the current
>     frame
>   - if selected_frame_level is 0: the selected frame is a user-created
>     one
>   - if selected_frame_level is > 0: the selected frame is unwound from
>     the current frame
>
> I'm not opposed to adding a separate flag to make it extra clear, but I
> think this is clear enough, especially since it's contained in frame.c
> (and soon frame_info_ptr), the rest of GDB doesn't need to know about
> this convention.  And adding a separate flag adds risks of having
> invalid states between the saved frame fields.
>
> Add a minimal test case for the case described above, that is the
> "select-frame view" command followed by the "frame" command twice.  In
> order to have a known stack frame to switch to, the test spawns a second
> thread, and tells the first thread to use the other thread's top frame.
>
> Change-Id: Ifc77848dc465fbd21324b9d44670833e09fe98c7
> ---
>   gdb/frame.c                           | 36 +++++++++++--
>   gdb/frame.h                           |  4 ++
>   gdb/testsuite/gdb.base/frame-view.c   | 74 +++++++++++++++++++++++++++
>   gdb/testsuite/gdb.base/frame-view.exp | 68 ++++++++++++++++++++++++
>   4 files changed, 177 insertions(+), 5 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.base/frame-view.c
>   create mode 100644 gdb/testsuite/gdb.base/frame-view.exp
>
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 6a6d968b9a97..e6e58a76668f 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -188,6 +188,10 @@ struct frame_info
>     /* A frame specific string describing the STOP_REASON in more detail.
>        Only valid when PREV_P is set, but even then may still be NULL.  */
>     const char *stop_string;
> +
> +  /* True if this frame was created from addresses given by the user (see
> +     create_new_frame) rather than through unwinding.  */
> +  bool is_user_created;
>   };
>   
>   /* See frame.h.  */
> @@ -1669,6 +1673,11 @@ get_current_frame (void)
>      never 0 and SELECTED_FRAME_ID is never the ID of the innermost
>      frame.
>   
> +   An exception to that is if the selected frame is a used-created one
> +   (created and selected through the "select-frame view" command).  That
> +   is encoded by having SELECTED_FRAME_LEVEL == 0 and SELECTED_FRAME_ID
> +   the frame id derived from the user-provided addresses.
> +

I would slightly reword this comment so there isn't incorrect 
information if someone only reads part of it. Maybe something like:

  If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1,
  and the target has stack and is stopped, the selected frame is the
  current (innermost) frame.  SELECTED_FRAME_ID is never the ID of the 
innermost
  frame. SELECTED_FRAME_LEVEL may be 0 only if the selected frame is a
  a user-created one (created and selected through the "select-frame view"
  command), in which case SELECTED_FRAME_ID is the frame id derived from
the user-provided addresses.

This way there is no way someone can be confused by thinking that 0 is 
never acceptable.

>      If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1,
>      and the target has no stack or is executing, then there's no
>      selected frame.  */
> @@ -1695,10 +1704,6 @@ void
>   restore_selected_frame (frame_id frame_id, int frame_level)
>     noexcept
>   {
> -  /* save_selected_frame never returns level == 0, so we shouldn't see
> -     it here either.  */
> -  gdb_assert (frame_level != 0);
> -
>     /* FRAME_ID can be null_frame_id only IFF frame_level is -1.  */
>     gdb_assert ((frame_level == -1 && !frame_id_p (frame_id))
>   	      || (frame_level != -1 && frame_id_p (frame_id)));
> @@ -1731,6 +1736,15 @@ lookup_selected_frame (struct frame_id a_frame_id, int frame_level)
>         return;
>       }
>   
> +  /* This means the selected frame was a user-created one.  Create a new one
> +     using the user-provided addresses, which happen to be in the frame id.  */
> +  if (frame_level == 0)
> +    {
> +      select_frame (create_new_frame (a_frame_id.stack_addr,
> +				      a_frame_id.code_addr));
> +      return;
> +    }
> +
>     /* select_frame never saves 0 in SELECTED_FRAME_LEVEL, so we
>        shouldn't see it here.  */
>     gdb_assert (frame_level > 0);
> @@ -1855,7 +1869,10 @@ select_frame (frame_info_ptr fi)
>   
>     selected_frame = fi;
>     selected_frame_level = frame_relative_level (fi);
> -  if (selected_frame_level == 0)
> +
> +  /* If the frame is a user-created one, save its level and frame id just like
> +     any other non-level-0 frame.  */
> +  if (selected_frame_level == 0 && !fi->is_user_created)
>       {
>         /* Treat the current frame especially -- we want to always
>   	 save/restore it without warning, even if the frame ID changes
> @@ -1934,6 +1951,7 @@ create_new_frame (CORE_ADDR addr, CORE_ADDR pc)
>   
>     fi = FRAME_OBSTACK_ZALLOC (struct frame_info);
>   
> +  fi->is_user_created = true;
>     fi->next = create_sentinel_frame (current_program_space,
>   				    get_current_regcache ());
>   
> @@ -2841,6 +2859,14 @@ get_frame_type (frame_info_ptr frame)
>     return frame->unwind->type;
>   }
>   
> +/* See frame.h.  */
> +
> +bool
> +frame_is_user_created (const frame_info *frame)
> +{
> +  return frame->is_user_created;
> +}
> +
>   struct program_space *
>   get_frame_program_space (frame_info_ptr frame)
>   {
> diff --git a/gdb/frame.h b/gdb/frame.h
> index cf8bbc6a52bd..12cb27573f4e 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -419,6 +419,10 @@ extern int frame_relative_level (frame_info_ptr fi);
>   
>   extern enum frame_type get_frame_type (frame_info_ptr);
>   
> +/* Return whether FRAME is user created.  */
> +
> +extern bool frame_is_user_created (const frame_info *frame);
> +
>   /* Return the frame's program space.  */
>   extern struct program_space *get_frame_program_space (frame_info_ptr);
>   
> diff --git a/gdb/testsuite/gdb.base/frame-view.c b/gdb/testsuite/gdb.base/frame-view.c
> new file mode 100644
> index 000000000000..7b1cbd6abd42
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/frame-view.c
> @@ -0,0 +1,74 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 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 <pthread.h>
> +#include <assert.h>
> +
> +struct type_1
> +{
> +  int m;
> +};
> +
> +struct type_2
> +{
> +  int n;
> +};
> +
> +static int
> +baz (struct type_1 z1, struct type_2 z2)
> +{
> +  return z1.m + z2.n;
> +}
> +
> +static int
> +bar (struct type_1 y1, struct type_2 y2)
> +{
> +  return baz (y1, y2);
> +}
> +
> +static int
> +foo (struct type_1 x1, struct type_2 x2)
> +{
> +  return bar (x1, x2);
> +}
> +
> +static void *
> +thread_func (void *p)
> +{
> +  struct type_1 t1;
> +  struct type_2 t2;
> +  t1.m = 11;
> +  t2.n = 11;
> +  foo (t1, t2);
> +
> +  return NULL;
> +}
> +
> +int
> +main (void)
> +{
> +  pthread_t thread;
> +  int res;
> +
> +  res = pthread_create (&thread, NULL, thread_func, NULL);
> +  assert (res == 0);
> +
> +  res = pthread_join (thread, NULL);
> +  assert (res == 0);
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/frame-view.exp b/gdb/testsuite/gdb.base/frame-view.exp
> new file mode 100644
> index 000000000000..45f0dcf08904
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/frame-view.exp
> @@ -0,0 +1,68 @@
> +# Copyright 2022 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 the "frame view" family of commands.
> +
> +standard_testfile
> +
> +if { [build_executable "failed to prepare" \
> +	${testfile} ${srcfile}] } {
> +    return
> +}
> +
> +proc test_select_frame_view {} {
> +    clean_restart $::binfile
> +
> +    if { ![runto_main] } {
> +	return
> +    }
> +
> +    # Stop thread 2 at a baz.
> +    gdb_test "break baz"
> +    gdb_test "continue" "Thread 2.*hit Breakpoint $::decimal, baz .*"
> +
> +    # Grab the stack pointer and pc of thread 2's frame.
> +    set frame_sp ""
> +    set frame_pc ""
> +
> +    gdb_test_multiple "info frame" "" {
> +	-re -wrap ".*frame at ($::hex):.*" {
> +	    set frame_sp $expect_out(1,string)
> +	    pass $gdb_test_name
> +	}
> +    }
> +
> +    gdb_test_multiple "print/x \$pc" "" {
> +	-re -wrap " = ($::hex)" {
> +	    set frame_pc $expect_out(1,string)
> +	    pass $gdb_test_name
> +	}
> +    }
> +
> +    if { $frame_sp == "" || $frame_pc == "" } {
> +	# Something must have failed and logged a failure above.
> +	return
> +    }
> +
> +    # Select thread 2's frame in thread 1.
> +    gdb_test "thread 1" "Switching to thread 1 .*"
> +    gdb_test_no_output "select-frame view $frame_sp $frame_pc"
> +
> +    # Verify that the "frame" command does not change the selected frame.
> +    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame"
> +    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame again"
I think it would be nice to have an explanation as to why frame is being 
called twice. Just a mention that there was a bug where we'd lose track 
of user-created frames when using the command "frame" would be enough, I 
think.
> +}
> +
> +test_select_frame_view
  

>> Now, the question is, how to identify a user-created frame through the
>> selected_frame_level/selected_frame_id pair.  I initially added a
>> separate "selected_frame_is_user_created" flag to that pair.  Then it
>> seemed simpler to make it so user-created frames would be identified by
>> the selected_frame_level == 0.  User-created frames already happen to
>> have level 0.  And remember that when the saved frame is the current
>> target frame, selected_frame_level is -1.  We need to modify
>> select_frame in any case to make it so it will save the frame id and use
>> the saved level 0 for user-created frames, despite them having level
>> 0.  So just with that, it's possible to distinguish user-created frames,
>> no need to add a separate flag.  It results in:
> 

> But this doesn't necessarily sound correct if we're supposed to be
> able to go up and down on user created frames. My best guess for such
> a feature would be improving the debugging experience of optimized
> programs or reverse engineering something. If you were able to
> identify that something is an inlined function, for instance, you
> could make an artificial frame to keep track of this information, so
> that future executions (or just moving up and down the stack) was
> easier.  If that guess is correct, it would be possible to have a user
> created frame at a level that isn't level 0, and the detection method
> fails.
You are right, if it was possible to unwind from that user frame, I
think we'd need to keep a separate flag to say "this frame was unwound
from a user-created frame, not the target current frame".  And we would
probably need to keep that user-created frame id too (in addition to the
selected frame id), to recreate it.  But at the moment, since it's not
possible to unwind go up / down from a user-created frame, I don't think
we should account for that.  If it ever becomes possible to unwind
user-created frames, we can change the strategy then.

>> @@ -1669,6 +1673,11 @@ get_current_frame (void)
>>      never 0 and SELECTED_FRAME_ID is never the ID of the innermost
>>      frame.
>>   +   An exception to that is if the selected frame is a used-created one
>> +   (created and selected through the "select-frame view" command).  That
>> +   is encoded by having SELECTED_FRAME_LEVEL == 0 and SELECTED_FRAME_ID
>> +   the frame id derived from the user-provided addresses.
>> +
> 
> I would slightly reword this comment so there isn't incorrect information if someone only reads part of it. Maybe something like:
> 
>  If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1,
>  and the target has stack and is stopped, the selected frame is the
>  current (innermost) frame.  SELECTED_FRAME_ID is never the ID of the innermost
>  frame. SELECTED_FRAME_LEVEL may be 0 only if the selected frame is a
>  a user-created one (created and selected through the "select-frame view"
>  command), in which case SELECTED_FRAME_ID is the frame id derived from
> the user-provided addresses.
> 
> This way there is no way someone can be confused by thinking that 0 is never acceptable.

Thanks, I used that.

>> +    # Verify that the "frame" command does not change the selected frame.
>> +    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame"
>> +    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame again"

> I think it would be nice to have an explanation as to why frame is
> being called twice. Just a mention that there was a bug where we'd
> lose track of user-created frames when using the command "frame" would
> be enough, I think.

Hmm I think the current comment pretty much says that.  By saying
"Verify that the frame command does not change the selected frame", it
implies there was a bug where the frame command did change the selected
frame.  But I can make it clearer if you'd like, comments are not
expensive.

Simon
  

On 08/12/2022 21:53, Simon Marchi wrote:
>>> Now, the question is, how to identify a user-created frame through the
>>> selected_frame_level/selected_frame_id pair.  I initially added a
>>> separate "selected_frame_is_user_created" flag to that pair.  Then it
>>> seemed simpler to make it so user-created frames would be identified by
>>> the selected_frame_level == 0.  User-created frames already happen to
>>> have level 0.  And remember that when the saved frame is the current
>>> target frame, selected_frame_level is -1.  We need to modify
>>> select_frame in any case to make it so it will save the frame id and use
>>> the saved level 0 for user-created frames, despite them having level
>>> 0.  So just with that, it's possible to distinguish user-created frames,
>>> no need to add a separate flag.  It results in:
>> But this doesn't necessarily sound correct if we're supposed to be
>> able to go up and down on user created frames. My best guess for such
>> a feature would be improving the debugging experience of optimized
>> programs or reverse engineering something. If you were able to
>> identify that something is an inlined function, for instance, you
>> could make an artificial frame to keep track of this information, so
>> that future executions (or just moving up and down the stack) was
>> easier.  If that guess is correct, it would be possible to have a user
>> created frame at a level that isn't level 0, and the detection method
>> fails.
> You are right, if it was possible to unwind from that user frame, I
> think we'd need to keep a separate flag to say "this frame was unwound
> from a user-created frame, not the target current frame".  And we would
> probably need to keep that user-created frame id too (in addition to the
> selected frame id), to recreate it.  But at the moment, since it's not
> possible to unwind go up / down from a user-created frame, I don't think
> we should account for that.  If it ever becomes possible to unwind
> user-created frames, we can change the strategy then.

Makes sense. I agree with the patch, then

Reviewed-By: Bruno Larsen <blarsen@redhat.com>

>
>>> @@ -1669,6 +1673,11 @@ get_current_frame (void)
>>>       never 0 and SELECTED_FRAME_ID is never the ID of the innermost
>>>       frame.
>>>    +   An exception to that is if the selected frame is a used-created one
>>> +   (created and selected through the "select-frame view" command).  That
>>> +   is encoded by having SELECTED_FRAME_LEVEL == 0 and SELECTED_FRAME_ID
>>> +   the frame id derived from the user-provided addresses.
>>> +
>> I would slightly reword this comment so there isn't incorrect information if someone only reads part of it. Maybe something like:
>>
>>   If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1,
>>   and the target has stack and is stopped, the selected frame is the
>>   current (innermost) frame.  SELECTED_FRAME_ID is never the ID of the innermost
>>   frame. SELECTED_FRAME_LEVEL may be 0 only if the selected frame is a
>>   a user-created one (created and selected through the "select-frame view"
>>   command), in which case SELECTED_FRAME_ID is the frame id derived from
>> the user-provided addresses.
>>
>> This way there is no way someone can be confused by thinking that 0 is never acceptable.
> Thanks, I used that.
>
>>> +    # Verify that the "frame" command does not change the selected frame.
>>> +    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame"
>>> +    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame again"
>> I think it would be nice to have an explanation as to why frame is
>> being called twice. Just a mention that there was a bug where we'd
>> lose track of user-created frames when using the command "frame" would
>> be enough, I think.
> Hmm I think the current comment pretty much says that.  By saying
> "Verify that the frame command does not change the selected frame", it
> implies there was a bug where the frame command did change the selected
> frame.  But I can make it clearer if you'd like, comments are not
> expensive.
Yeah, I guess you're right, I must have been a bit too tired when I read 
this to not notice the implication you mentioned. It's probably fine.