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

[Pedro Alves]

On 7/9/20 12:56 PM, Pedro Alves wrote:
> On 7/9/20 4:49 AM, Simon Marchi wrote:
>>>  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.
>>
> 
> At first, I was recording frame 0 normally, without that special case.
> But running the testsuite revealed regressions in a couple testcases:
> 
>  gdb.python/py-unwind-maint.exp
>  gdb.server/bkpt-other-inferior.exp
> 
> Both are related to the get_frame_id call.  Before the patch, get_frame_id
> isn't called on the current frame until you try to backtrace from it.
> Adding the get_frame_id call makes the gdb.python/py-unwind-maint.exp testcase
> print the Python unwinder callbacks in a different order, unexpected
> by the testcase.  I didn't look too deeply into this one, but I suspect
> it would just be a matter of adjusting the testcase's expectations.
> 
> The gdb.server/bkpt-other-inferior.exp one though is what got me
> thinking.  The testcase makes sure that setting a breakpoint in a
> function that doesn't exist in the remote inferior does not cause
> remote protocol traffic.  After the patch, without the special casing,
> the testcase would fail because the get_frame_id call, coming from 
> 
>  check_frame_language_change  # called after every command
>   -> get_selected_frame
>     -> restore_selected_frame
>       -> select_frame(get_current_frame())
>          -> get_frame_id
> 
> would cause registers and memory to be read from the remote target (when
> restoring the selected frame).  Those accesses aren't wrong, but they
> aren't the kind that the bug the testcase is looking for.  Those were
> about spurious/incorrect remote protocol accesses when parsing the
> function's prologue.
> 
> Neither of these cases were strictly incorrect, though they got me
> thinking, and I came to the conclusion that warning when we fail to
> re-find the current frame is pointless, and that avoids having
> unbreak the testcases mentioned, or even redo them differently in
> the gdb.server/bkpt-other-inferior.exp case.
> 
> I've updated the comment to make it clearer with an example.
> 
> 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 declared lookup_selected_frame in frame.h already, thinking that
> it's easier if I move lookup_selected_frame from thread.c to frame.c
> after this is in, instead of before.
> 
> I rewrote most of the comments.  For example, I think the
> selected_frame_id/selected_frame_level/selected_frame comments are now
> much clearer.
> 
> And I made scoped_restore_selected_frame save/restore the language
> too.  I was only doing that in scoped_restore_current_thread before.
> 
> Let me know what you think of this version.

I've pushed this, along with all the PR26199 patches to:

 users/palves/pr26199-busy-loop-target-events

(The version pushed has a couple comment typos fixed compared to the
one posted.)