[PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC)

[Simon Marchi]

> I've also polished the patch some more.  I now renamed
> the current restore_selected_frame to lookup_selected_frame,
> to give space to the new save_selected_frame/restore_selected_frame
> pair.  select_frame_lazy is now restore_selected_frame.
> save_selected_frame/restore_selected_frame are now noexcept, and
> their intro comments explain why.

I noticed there is also a `restore_selected_frame` in infrun.c... I don't
know if it's replicating features also implemented in frame.c?

> @@ -1641,10 +1639,51 @@ get_current_frame (void)
>  }
>  
>  /* The "selected" stack frame is used by default for local and arg
> -   access.  May be zero, for no selected frame.  */
> -
> +   access.  */
> +
> +/* The "single source of truth" for the selected frame is the
> +   SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL pair.  Frame IDs can be
> +   saved/restored across reinitializing the frame cache, while
> +   frame_info pointers can't (frame_info objects are invalidated).  If
> +   we know the corresponding frame_info object, it is cached in
> +   SELECTED_FRAME.  If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are
> +   null_ptid / -1, and the target has stack and is stopped, the
> +   selected frame is the current (innermost) frame, otherwise there's
> +   no selected frame.  */

Pedantically, the `otherwise` could let the reader think that if
SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are not null_ptid / -1, then there
is no selected frame.  I'd suggest moving this out to its own sentence to be
clear.

I'd also make it more explicit here that when the innermost frame is selected,
SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL.  Currently, it says that if
SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_ptid / -1, it means the
innermost frame is selected, but it doesn't imply that if the innermost frame
is selected, then SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_ptid / -1
(difference between if and "if and only if").

I think it would help to separate that in paragraphes to ease reading.

Also, I just noticed that you comment uses `null_ptid` instead of `null_frame_id`,
which just shows how much `null_ptid` is engraved in your brain :).  I also
re-worked the comment for 15 minutes before noticing :).

So:

/* The "single source of truth" for the selected frame is the
   SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL pair.

   Frame IDs can be saved/restored across reinitializing the frame cache, while
   frame_info pointers can't (frame_info objects are invalidated).  If we know
   the corresponding frame_info object, it is cached in SELECTED_FRAME.

   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.  This means that SELECTED_FRAME_LEVEL is never 0 and
   SELECTED_FRAME_ID is never the ID of the innermost frame.

   If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1, and the
   target has no stack or is executing, the selected, then there's no selected
   frame.  */

> +static frame_id selected_frame_id = null_frame_id;
> +static int selected_frame_level = -1;
> +
> +/* The cached frame_info object pointing to the selected frame.
> +   Looked up on demand by get_selected_frame.  */
>  static struct frame_info *selected_frame;

Ah ok, in my previous reply I almost said: "so this essentially becomes
a cache for selected_frame_id/selected_frame_level", but changed it because
I *thought* I had understood that it wasn't the case when the frame_level
was > 0.  But if we can word it this way, that makes it simpler to understand.

>  
> +/* See frame.h.  */
> +
> +void
> +save_selected_frame (frame_id *frame_id, int *frame_level)
> +  noexcept
> +{
> +  *frame_id = selected_frame_id;
> +  *frame_level = selected_frame_level;
> +}
> +
> +/* See frame.h.  */
> +
> +void
> +restore_selected_frame (frame_id a_frame_id, int frame_level)
> +  noexcept
> +{
> +  /* get_selected_frame_info never returns level == 0, so we shouldn't
> +     see it here either.  */
> +  gdb_assert (frame_level != 0);

We could also assert that a_frame_id can be null_frame_id IFF frame_level is -1.

> +
> +  selected_frame_id = a_frame_id;
> +  selected_frame_level = frame_level;
> +
> +  /* Will be looked up latter by get_seleted_frame.  */
> +  selected_frame = nullptr;
> +}
> +
>  int
>  has_stack_frames (void)
>  {
> @@ -1682,24 +1721,14 @@ get_selected_frame (const char *message)
>      {
>        if (message != NULL && !has_stack_frames ())
>  	error (("%s"), message);
> -      /* Hey!  Don't trust this.  It should really be re-finding the
> -	 last selected frame of the currently selected thread.  This,
> -	 though, is better than nothing.  */
> -      select_frame (get_current_frame ());
> +
> +      lookup_selected_frame (selected_frame_id, selected_frame_level);

Could you fix (in this patch or another one) the comment of `get_selected_frame`
to be `/* See frame.h.  */` and make sure that the version in frame.h is the
most up to date one?

> @@ -1712,12 +1741,42 @@ deprecated_safe_get_selected_frame (void)
>    return get_selected_frame (NULL);
>  }
>  
> -/* Select frame FI (or NULL - to invalidate the current frame).  */
> +/* Select frame FI (or NULL - to invalidate the selected frame).  */
>  
>  void
>  select_frame (struct frame_info *fi)
>  {
>    selected_frame = fi;
> +  selected_frame_level = frame_relative_level (fi);
> +  if (selected_frame_level == 0)
> +    {
> +      /* Treat the current frame especially -- we want to always
> +	 save/restore it without warning, even if the frame ID changes
> +	 (see lookup_selected_frame).  E.g.:
> +
> +	  // The current frame is selected, the target had just stopped.
> +	  {
> +	    scoped_restore_selected_frame restore_frame;
> +	    some_operation_that_changes_the_stack ();
> +	  }
> +	  // scoped_restore_selected_frame's dtor runs, but the
> +	  // original frame_id can't be found.  No matter whether it
> +	  // is found or not, we still end up with the now-current
> +	  // frame selected.  Warning in lookup_selected_frame in this
> +	  // case seems pointless.
> +
> +	 Also get_frame_id may access the target's registers/memory,
> +	 and thus skipping get_frame_id optimizes the common case.
> +
> +	 Saving the selected frame this way makes get_selected_frame
> +	 and restore_current_frame return/re-select whatever frame is

restore_selected_frame

> @@ -329,13 +335,33 @@ extern void reinit_frame_cache (void);
>     and then return that thread's previously selected frame.  */
>  extern struct frame_info *get_selected_frame (const char *message);
>  
> -/* If there is a selected frame, return it.  Otherwise, return NULL.  */
> -extern struct frame_info *get_selected_frame_if_set (void);
> -
> -/* Select a specific frame.  NULL, apparently implies re-select the
> -   inner most frame.  */
> +/* Select a specific frame.  NULL implies re-select the inner most
> +   frame.  */
>  extern void select_frame (struct frame_info *);
>  
> +/* Save the frame ID and frame level of the selected frame in FRAME_ID
> +   and FRAME_LEVEL, to be restored later with restore_selected_frame.
> +   This is preferred over getting the same info out of
> +   get_selected_frame directly because this function does not create
> +   the selected-frame's frame_info object if it hasn't been created
> +   yet, and thus doesn't throw.  */
> +extern void save_selected_frame (frame_id *frame_id, int *frame_level)
> +  noexcept;
> +
> +/* Restore selected frame as saved with save_selected_frame.  Does not
> +   try to find the corresponding frame_info object.  Instead the next
> +   call to get_selected_frame will look it up and cache the result.
> +   This function does not throw, it is designed to be safe to called
> +   from the destructors of RAII types.  */
> +extern void restore_selected_frame (frame_id frame_id, int frame_level)
> +  noexcept;
> +
> +/* Lookup the frame_info object for the selected frame FRAME_ID /
> +   FRAME_LEVEL and cache the result.  If FRAME_LEVEL > 0 and the
> +   originally selected frame isn't found, warn and select the
> +   innermost (current) frame.  */
> +extern void lookup_selected_frame (frame_id frame_id, int frame_level);

As I mentioned above, I think the comments would be easier to read with
a bit more newlines.  In general, I like to have one short sentence by
itself first, that says what the function does at a very high level.  A
bit like in man pages you have a short description next to the name:

  rm - remove files or directories
  sudo, sudoedit — execute a command as another user
  ssh — OpenSSH remote login client

Then, you can go in details about the behavior of the function in following
paragraphs, making sure to split different ideas in different paragraphs.

At first, I thought it would just be nitpicking to ask people to space
out their comments and code a bit more, but now I think it really does
make a difference in readability (at least, it helps me).

> diff --git a/gdb/stack.c b/gdb/stack.c
> index 265e764dc2..93de451a12 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -1836,9 +1836,9 @@ trailing_outermost_frame (int count)
>  static void
>  select_frame_command_core (struct frame_info *fi, bool ignored)
>  {
> -  struct frame_info *prev_frame = get_selected_frame_if_set ();
> +  frame_info *prev_frame = get_selected_frame (NULL);
>    select_frame (fi);
> -  if (get_selected_frame_if_set () != prev_frame)
> +  if (get_selected_frame (NULL) != prev_frame)

I'm telling people that we try to use `nullptr` for new code.  I don't
know what's your opinion on this.  I would just like to have a simple
and clear so we don't have to wonder which of `NULL` and `nullptr` to
use.

>      gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME);
>  }
>  
> @@ -1857,10 +1857,9 @@ select_frame_for_mi (struct frame_info *fi)
>  static void
>  frame_command_core (struct frame_info *fi, bool ignored)
>  {
> -  struct frame_info *prev_frame = get_selected_frame_if_set ();
> -
> +  frame_info *prev_frame = get_selected_frame (nullptr);
>    select_frame (fi);
> -  if (get_selected_frame_if_set () != prev_frame)
> +  if (get_selected_frame (nullptr) != prev_frame)

... especially that you've used `nullptr` here :).

Although maybe that in this case, get_selected_frame could have a default
parameter value of `nullptr`.

> diff --git a/gdb/thread.c b/gdb/thread.c
> index a3c2be7dd0..e0b49abf0c 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -1325,20 +1325,25 @@ switch_to_thread (process_stratum_target *proc_target, ptid_t ptid)
>    switch_to_thread (thr);
>  }
>  
> -static void
> -restore_selected_frame (struct frame_id a_frame_id, int frame_level)
> +/* See frame.h.  */
> +
> +void
> +lookup_selected_frame (struct frame_id a_frame_id, int frame_level)
>  {
>    struct frame_info *frame = NULL;
>    int count;
>  
> -  /* This means there was no selected frame.  */
> +  /* This either means there was no selected frame, or the selected
> +     frame was the innermost (the current frame).  */

It's a bit heavy to always precise that `innermost` means the `current`
frame.  Let's choose one name and stick to it, it will be clear enough
on its own.  I'd use `current`, since that's how the functions are named
(e.g. get_current_frame).  I understand there might be a bit confusion
between `selected` and `current` for newcomers, but once you know the
distinction, it's pretty clear.

I'd also add to that comment, what is the action we take as a result.

So, maybe:

  /* This either means there was no selected frame, or the selected frame was
     the current one.  In either case, try to select the current frame.  */


> @@ -1409,22 +1414,14 @@ scoped_restore_current_thread::restore ()
>        && target_has_stack
>        && target_has_memory)
>      restore_selected_frame (m_selected_frame_id, m_selected_frame_level);
> +
> +  set_language (m_lang);
>  }
>  
>  scoped_restore_current_thread::~scoped_restore_current_thread ()
>  {
>    if (!m_dont_restore)
> -    {
> -      try
> -	{
> -	  restore ();
> -	}
> -      catch (const gdb_exception &ex)
> -	{
> -	  /* We're in a dtor, there's really nothing else we can do
> -	     but swallow the exception.  */
> -	}
> -    }
> +    restore ();

I don't know if it would be worth it, but I'd like if we could assert (abort
GDB) if an exception does try to exit the destructor.  The `restore` method
is non-trivial and calls into other non-trivial functions, so it would be
possible for a change far far away to cause that to happen.

What do you think of keeping the try/catch, but using `gdb_assert_not_reached`
in it?

Simon