[PATCHv2,2/3] gdb/regcache: When saving, ignore registers that can't be read

  The previous commit addressed an assertion that could trigger if a
target threw an error while saving state ahead of an inferior function
call.

The specific case that highlighted this issue was a RISC-V target that
claimed to support floating point registers, but when GDB tried to
read a floating point register the remote sent back an error.

With the previous commit we no longer see an assertion for this
target, now GDB abandons the inferior function call.

Although this is slightly better, it feels like for this specific case
GDB could do even better.  If during a call to reg_buffer::save GDB
encounters an error trying to read a register then GDB should simply
mark the register as unavailable and carry on.  The consequence of
marking the register unavailable is that GDB will not then try to
restore the register once the inferior function call is complete.

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 register 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(-)
  

On 11/27/2018 11:13 AM, Andrew Burgess wrote:
> The previous commit addressed an assertion that could trigger if a
> target threw an error while saving state ahead of an inferior function
> call.
> 
> The specific case that highlighted this issue was a RISC-V target that
> claimed to support floating point registers, but when GDB tried to
> read a floating point register the remote sent back an error.
> 
> With the previous commit we no longer see an assertion for this
> target, now GDB abandons the inferior function call.
> 
> Although this is slightly better, it feels like for this specific case
> GDB could do even better.  If during a call to reg_buffer::save GDB
> encounters an error trying to read a register then GDB should simply
> mark the register as unavailable and carry on.  The consequence of
> marking the register unavailable is that GDB will not then try to
> restore the register once the inferior function call is complete.

I'm skeptical about this.  It sounds risky to me.  An infcall is
potentially state-destructive, and silencing errors just seems like asking
for trouble.  Particularly, while you're observing one specific error,
you're swallowing all kinds of errors.

> 
> 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, 

How is the user to know to do that without any kind of indication?

> at which point it becomes obvious that the
> register can't be read.  That said, I'm open to adding a warning that
> the register 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.
> 

I'm really not sure this is a good trade off.  

How could such a stub with this kind of problem end up in production?
It sounds like it can't have seen much wild use without someone running
into this.  Making GDB handle this scenario "gracefully" can only be useful
if this is really a kind of problem that can go undetected for a long
while and you plan on continuing to let users use the "bad" stub.
But what's the real scenario that would lead to that happening?

Thanks,
Pedro Alves
  

* Pedro Alves <palves@redhat.com> [2018-11-27 12:41:51 +0000]:

> On 11/27/2018 11:13 AM, Andrew Burgess wrote:
> > The previous commit addressed an assertion that could trigger if a
> > target threw an error while saving state ahead of an inferior function
> > call.
> > 
> > The specific case that highlighted this issue was a RISC-V target that
> > claimed to support floating point registers, but when GDB tried to
> > read a floating point register the remote sent back an error.
> > 
> > With the previous commit we no longer see an assertion for this
> > target, now GDB abandons the inferior function call.
> > 
> > Although this is slightly better, it feels like for this specific case
> > GDB could do even better.  If during a call to reg_buffer::save GDB
> > encounters an error trying to read a register then GDB should simply
> > mark the register as unavailable and carry on.  The consequence of
> > marking the register unavailable is that GDB will not then try to
> > restore the register once the inferior function call is complete.
> 
> I'm skeptical about this.  It sounds risky to me.  An infcall is
> potentially state-destructive, and silencing errors just seems like asking
> for trouble.  Particularly, while you're observing one specific error,
> you're swallowing all kinds of errors.
> 
> > 
> > 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, 
> 
> How is the user to know to do that without any kind of indication?
> 
> > at which point it becomes obvious that the
> > register can't be read.  That said, I'm open to adding a warning that
> > the register 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.
> > 
> 
> I'm really not sure this is a good trade off.  
> 
> How could such a stub with this kind of problem end up in production?
> It sounds like it can't have seen much wild use without someone running
> into this.  Making GDB handle this scenario "gracefully" can only be useful
> if this is really a kind of problem that can go undetected for a long
> while and you plan on continuing to let users use the "bad" stub.
> But what's the real scenario that would lead to that happening?

Pedro,

Thanks for taking the time to review this patch.

Just wanted to confirm that (subject to review) the above feedback
doesn't prevent patch #1 or #3 being merged, correct?

Patch #1 specifically makes the inferior call error rather than
assert, which feels like it doesn't raise the same concerns you
discuss above.

Thanks,
Andrew
  

On 11/27/2018 03:30 PM, Andrew Burgess wrote:

> Thanks for taking the time to review this patch.
> 
> Just wanted to confirm that (subject to review) the above feedback
> doesn't prevent patch #1 or #3 being merged, correct?

Correct.

> Patch #1 specifically makes the inferior call error rather than
> assert, which feels like it doesn't raise the same concerns you
> discuss above.
Absolutely.

Thanks,
Pedro Alves