Re: [PATCH] gdb/regcache: When saving, ignore registers that can't be read

[Simon Marchi <simark@simark.ca>]

On 2018-11-21 1:17 p.m., Andrew Burgess wrote:
> If during a call to reg_buffer::save GDB encounters an error trying to
> read a register then this should not cause GDB to crash, nor should it
> force the save to quit.  Instead, GDB should just treat the register
> as unavailable and push on.
> 
> The specific example I encountered was a RISC-V remote target that
> claimed in its target description to have floating point register
> support.  However, this was not true, when GDB tried to read a
> floating point register the remote sent back an error.
> 
> Mostly this was fine, the program I was testing were integer only,
> however, when trying to make an inferior call, GDB would try to
> preserve the current values of the floating point registers, this
> result in a read of a register that threw an error, and GDB would
> crash like this:
> 
>     (gdb) call some_inferior_function ()
>     ../../src/gdb/regcache.c:310: internal-error: void regcache::restore(readonly_detached_regcache*): Assertion `src != NULL' failed.
>     A problem internal to GDB has been detected,
>     further debugging may prove unreliable.
>     Quit this debugging session? (y or n)
> 
> I acknowledge that the target description sent back in this case is
> wrong, and the target should be fixed.  However, I think that GDB
> should, at a minimum, not crash and burn in this case, and better, I
> think GDB can probably just push on, ignoring the registers that can't
> be read.
> 
> The solution I propose in this patch is to catch errors in
> reg_buffer::save while calling cooked_read, and register that throws
> an error should be considered unavailable.  GDB will not try to
> restore these registers after the inferior call.
> 
> What I haven't done in this commit is provide any user feedback that
> GDB would like to backup a particular register, but can't.  Right now
> I figure that if the user cares about this they would probably try 'p
> $reg_name' themselves, at which point it becomes obvious that the
> register can't be read.  That said, I'm open to adding a warning that
> the regiter failed to save if that is thought important.
> 
> I've tested this using on X86-64/Linux native, and for
> native-gdbserver with no regressions.  Against my miss-behaving target
> I can now make inferior calls without any problems.
> 
> gdb/ChangeLog:
> 
> 	* regcache.c (reg_buffer::save): When saving the current register
> 	state, ignore registers that can't be read.
> ---
>  gdb/ChangeLog  |  5 +++++
>  gdb/regcache.c | 12 +++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 946035ae67a..b89be24ccb6 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -277,7 +277,17 @@ reg_buffer::save (register_read_ftype cooked_read)
>        if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
>  	{
>  	  gdb_byte *dst_buf = register_buffer (regnum);
> -	  enum register_status status = cooked_read (regnum, dst_buf);
> +	  enum register_status status;
> +
> +	  TRY
> +	    {
> +	      status = cooked_read (regnum, dst_buf);
> +	    }
> +	  CATCH (ex, RETURN_MASK_ERROR)
> +	    {
> +	      status = REG_UNAVAILABLE;
> +	    }
> +	  END_CATCH
>  
>  	  gdb_assert (status != REG_UNKNOWN);
>  
> 


Hi Andrew,

I think your fix makes sense.

About the assertion you hit, I think it shows a weakness in infcall_suspend_state_up.
The deleter is not able to handle the state of infcall_suspend_state where the
registers field is NULL.

So either:

1. We decide that an infcall_suspend_state with a NULL registers field is an invalid
   state and we make sure to never have one in this state.
2. We change the deleter (consequently restore_infcall_suspend_state) to have it
   handle the possibility of registers == NULL.

This means that even without your fix, GDB should ideally be able to handle the failure
more gracefully than it does now.  The infcall should just be aborted and an error message
shown.

Does that make sense?

Simon