8.3 regression: terminate called after throwing an instance of 'gdb_exception_error'

  This problem was found when running the valgrind gdbserver tests with
GDB 8.3 (the valgrind test gdbserver_tests/mcinfcallWSRU hangs
due to GDB crashing).

An easy way to reproduce:

valgrind sleep 10000

In another window:
gdb
target remote | vgdb
p printf("make sleep print something\n")
terminate called after throwing an instance of 'gdb_exception_error'
Aborted


The problem is due to the fact that sometimes, Valgrind gdbserver
refuses to change registers : when GDB has 'waken up' the valgrind
gdbserver while the valgrind process is blocked in a syscall, it is not possible
to change the (simulated) valgrind registers, as in this state, valgrind
has "switched" to the real registers.

So, in such a state, the valgrind gdbserver returns an error to the P packet.
With GDB 8.2, that gives:
(gdb) p printf("make sleep print something\n")
Could not write register "rdi"; remote failure reply 'E.
ERROR changing register rdi regno 5
gdb commands changing registers (pc, sp, ...) (e.g. 'jump',
set pc, calling from gdb a function in the debugged process, ...)
can only be accepted if the thread is VgTs_Runnable or VgTs_Yielding state
Thread status is VgTs_WaitSys
'
(gdb) 

With 8.3, GDB aborts with this 'terminate called ...' message.

As far as I can see, the problem is:
Before pushing the inferior call, GDB 8.3 saves the state of the inferior,
using the below type:
 typedef std::unique_ptr<infcall_suspend_state, infcall_suspend_state_deleter>
    infcall_suspend_state_up;

Then the push of inferior call tries to modify a register.  It fails with
the above message, causing a throw.
This throw causes the infcall_suspend_state_up destructor to be called,
calling the deleter.
The deleter tries to restore the state by modifying back the register
to the saved value.  It similarly fails and causes an exception in the
destructor.
That then causes the above abort.

The below patch avoids the crash.  Not very clear to me if that
is the correct way to do.
Note that the valgrind test then still fails, because the below
produces more messages than the GDB 8.2.
So, maybe we could/should just silently ignore errors in the exception
handler ?
(IIUC, in GDB 8.2, this was done with a cleanup that was ignoring
such errors).

Philippe
  

On 4/10/19 5:58 PM, Philippe Waroquiers wrote:
> This problem was found when running the valgrind gdbserver tests with
> GDB 8.3 (the valgrind test gdbserver_tests/mcinfcallWSRU hangs
> due to GDB crashing).
> 
> An easy way to reproduce:
> 
> valgrind sleep 10000
> 
> In another window:
> gdb
> target remote | vgdb
> p printf("make sleep print something\n")
> terminate called after throwing an instance of 'gdb_exception_error'
> Aborted

I think that that error message must have been printed by gdb master,
since that type doesn't exist in the 8.3 branch?  That was only added
very recently, with the elimination of the TRY/CATCH macros.

> 
> 
> The problem is due to the fact that sometimes, Valgrind gdbserver
> refuses to change registers : when GDB has 'waken up' the valgrind
> gdbserver while the valgrind process is blocked in a syscall, it is not possible
> to change the (simulated) valgrind registers, as in this state, valgrind
> has "switched" to the real registers.
> 
> So, in such a state, the valgrind gdbserver returns an error to the P packet.
> With GDB 8.2, that gives:
> (gdb) p printf("make sleep print something\n")
> Could not write register "rdi"; remote failure reply 'E.
> ERROR changing register rdi regno 5
> gdb commands changing registers (pc, sp, ...) (e.g. 'jump',
> set pc, calling from gdb a function in the debugged process, ...)
> can only be accepted if the thread is VgTs_Runnable or VgTs_Yielding state
> Thread status is VgTs_WaitSys
> '
> (gdb) 
> 
> With 8.3, GDB aborts with this 'terminate called ...' message.
> 
> As far as I can see, the problem is:
> Before pushing the inferior call, GDB 8.3 saves the state of the inferior,
> using the below type:
>  typedef std::unique_ptr<infcall_suspend_state, infcall_suspend_state_deleter>
>     infcall_suspend_state_up;
> 
> Then the push of inferior call tries to modify a register.  It fails with
> the above message, causing a throw.
> This throw causes the infcall_suspend_state_up destructor to be called,
> calling the deleter.
> The deleter tries to restore the state by modifying back the register
> to the saved value.  It similarly fails and causes an exception in the
> destructor.
> That then causes the above abort.
> 
> The below patch avoids the crash.  Not very clear to me if that
> is the correct way to do.
> Note that the valgrind test then still fails, because the below
> produces more messages than the GDB 8.2.
> So, maybe we could/should just silently ignore errors in the exception
> handler ?

Yeah, I guess silent is fine.  Unless we fail to restore the registers
in the normal, non-exception path?  Maybe print the warning if
std::uncaught_exception() is false?

That is, if we're existing due to an exception, then we'll already
print some error.  Otherwise, if we're exiting the scope normally,
and that fails, print a warning.

We'll still print if some destructor somewhere decides to do an infcall 
while unwinding due to an exception, and the infcall succeeds (std::uncaught_exception
is true due to the outer exception), which is kind of the reason why
std::uncaught_exceptions (plural) was invented in C++17, but that seems
like a corner case that's not very concerning.  The only downside is
printing too much.

> (IIUC, in GDB 8.2, this was done with a cleanup that was ignoring
> such errors).
> 
> Philippe
> 
> 
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 2d1bb97a28..d20db63103 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -68,7 +68,15 @@ struct infcall_suspend_state_deleter
>  {
>    void operator() (struct infcall_suspend_state *state) const
>    {
> -    restore_infcall_suspend_state (state);
> +    TRY
> +      {
> +	restore_infcall_suspend_state (state);
> +      }
> +    CATCH (e, RETURN_MASK_ALL)
> +      {
> +	warning (_("failed to restore_infcall_suspend_state: %s"), e.message);
> +      }

For current master, you'll need to replace the TRY/CATCH with
try/catch.

Also, the warning text should not refer to gdb internals, since it's
going to be seen by regular users.

Thanks,
Pedro Alves