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

[Simon Marchi <simark@simark.ca>]

On 2020-07-08 7:31 p.m., Pedro Alves wrote:
> If the remote target closes while we're reading registers/memory for
> restoring the selected frame in scoped_restore_current_thread's dtor,
> the corresponding TARGET_CLOSE_ERROR error is swallowed by the
> scoped_restore_current_thread's dtor, because letting exceptions
> escape from a dtor is bad.  It isn't great to lose that errors like
> that, though.  I've been thinking about how to avoid it, and I came up
> with this patch.
> 
> The idea here is to make scoped_restore_current_thread's dtor do as
> little as possible, to avoid any work that might throw in the first
> place.  And to do that, instead of having the dtor call
> restore_selected_frame, which re-finds the previously selected frame,
> just record the frame_id/level of the desired selected frame, and have
> get_selected_frame find the frame the next time it is called.  In
> effect, this implements most of Cagney's suggestion, here:
> 
>   /* On demand, create the selected frame and then return it.  If the
>      selected frame can not be created, this function prints then throws
>      an error.  When MESSAGE is non-NULL, use it for the error message,
>      otherwize use a generic error message.  */
>   /* FIXME: cagney/2002-11-28: At present, when there is no selected
>      frame, this function always returns the current (inner most) frame.
>      It should instead, when a thread has previously had its frame
>      selected (but not resumed) and the frame cache invalidated, find
>      and then return that thread's previously selected frame.  */
>   extern struct frame_info *get_selected_frame (const char *message);
> 
> The only thing missing to fully implement that would be to make
> reinit_frame_cache just clear selected_frame instead of calling
> select_frame(NULL), and the call select_frame(NULL) explicitly in the
> places where we really wanted reinit_frame_cache to go back to the
> current frame too.  That can done separately, though, I'm not
> proposing to do that in this patch.
> 
> restore_selected_frame should really move from thread.c to frame.c,
> but I didn't do that here, just to avoid churn in the patch while it
> collects comments.  I will do that as a preparatory patch if people
> agree with this approach.
> 
> Incidentally, this patch alone would fix the crashes fixed by the
> previous patches in the series, because with this,
> scoped_restore_current_thread's constructor doesn't throw either.

I don't understand all the code changes, the but the idea sounds fine
to me.

> -/* 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 restore_selected_frame).  Also get_frame_id may access
> +	 the target's registers/memory, and thus skipping get_frame_id
> +	 optimizes the common case.  */
> +      selected_frame_level = -1;
> +      selected_frame_id = null_frame_id;
> +    }
> +  else
> +    selected_frame_id = get_frame_id (fi);
> +

I don't really understand this part, why don't we want to set selected_frame_level
and selected_frame_id when the level is 0.  I'm more interested by why it wouldn't
be correct or how it would break things, rather than the optimization aspect.

Simon