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

[Andrew Burgess]

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);

[Simon Marchi]

Post by Andrew Burgess
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
(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.
* 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

[Andrew Burgess]

Post by Simon Marchi

Post by Andrew Burgess
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
(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.
* 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.
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?

Yes it does.

In this new series, patch #1 makes the prepare for inferior function
call process more resistant to errors during the preparation phase.
After this patch the case I addressed above would fail with an error
(better than an assertion).

In patch #2 I then handle the specific case I am encountering better,
so that for the case that a register can't be read, GDB still performs
the inferior function call.

And patch #3 is a random fix I hit while testing the above patches.

How does this look?

Thanks,
Andrew

---

Andrew Burgess (3):
gdb/infcall: Make infcall_suspend_state more class like
gdb/regcache: When saving, ignore registers that can't be read
gdb: Update test pattern to deal with native-extended-gdbserver

gdb/ChangeLog | 25 +++++++
gdb/infrun.c | 132 ++++++++++++++++++++++---------------
gdb/regcache.c | 12 +++-
gdb/testsuite/ChangeLog | 4 ++
gdb/testsuite/gdb.base/annota1.exp | 23 ++++++-
5 files changed, 141 insertions(+), 55 deletions(-)

[Andrew Burgess]

I ran into a situation where attempting to make an inferior function
call would trigger an assertion, 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)

The problem that triggers the assertion is that in the function
save_infcall_suspend_state, we basically did this:

1. Create empty infcall_suspend_state object.
2. Fill fields of infcall_suspend_state object.

The problem is causes is that if filling any of the fields triggered
an exception then the infcall_suspend_state object would be deleted
while in a partially filled in state.

In the specific case I encountered, I had a remote RISC-V target that
claimed in its target description to support floating point registers.
However, this was not true, and when GDB tried to read a floating
point register the remote sent back an error. This error would cause
an exception to be thrown while creating the
readonly_detached_regcache, which in turn caused GDB to try and delete
an infcall_suspend_state which didn't have any register state, and
this triggered the assertion.

To prevent this problem we have two possibilities, either, rewrite the
restore code the handle partially initialised infcall_suspend_state
objects, or, prevent partially initialised infcall_suspend_state
objects from existing. The second of these seems like a better
solution.

So, in this patch, I move the filling in of the different
infcall_suspend_state fields within a new constructor for
infcall_suspend_state. Now, if generating one of those fields fails
the destructor for infcall_suspend_state will not be executed and GDB
will not try to restore the partially saved state.

With this patch in place GDB now behaves like this:

(gdb) call some_inferior_function ()
Could not fetch register "ft0"; remote failure reply 'E99'
(gdb)

The inferior function call is aborted due to the error.

This has been tested against x86-64/Linux native, native-gdbserver,
and native-extended-gdbserver with no regressions. I've manually
tested this against my baddly behaving target and confirmed the
inferior function call is aborted as described above.

gdb/ChangeLog:

* infrun.c (infcall_suspend_state::infcall_suspend_state): New.
(infcall_suspend_state::get_registers): New.
(infcall_suspend_state::restore): New.
(infcall_suspend_state::thread_suspend): Rename to...
(infcall_suspend_state::m_thread_suspend): ...this.
(infcall_suspend_state::registers): Rename to...
(infcall_suspend_state::m_registers): ...this.
(infcall_suspend_state::siginfo_gdbarch): Rename to...
(infcall_suspend_state::m_siginfo_gdbarch): ...this.
(infcall_suspend_state::siginfo_data): Rename to...
(infcall_suspend_state::m_siginfo_data): ...this.
(save_infcall_suspend_state): Rewrite to use infcall_suspend_state
constructor.
(restore_infcall_suspend_state): Rewrite to use
infcall_suspend_state::restore method.
(get_infcall_suspend_state_regcache): Use
infcall_suspend_state::get_registers method.
---
gdb/ChangeLog | 20 +++++++++
gdb/infrun.c | 132 +++++++++++++++++++++++++++++++++++-----------------------
2 files changed, 100 insertions(+), 52 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 46a8985f860..a9d7fa17aaf 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8742,18 +8742,85 @@ siginfo_make_value (struct gdbarch *gdbarch, struct internalvar *var,

struct infcall_suspend_state
{
- struct thread_suspend_state thread_suspend;
+public:
+ /* Capture state from GDBARCH, TP, and REGCACHE that must be restored
+ once the inferior function call has finished. */
+ infcall_suspend_state (struct gdbarch *gdbarch,
+ const struct thread_info *tp,
+ struct regcache *regcache)
+ : m_thread_suspend (tp->suspend),
+ m_registers (new readonly_detached_regcache (*regcache))
+ {
+ gdb::unique_xmalloc_ptr<gdb_byte> siginfo_data;

- /* Other fields: */
- std::unique_ptr<readonly_detached_regcache> registers;
+ if (gdbarch_get_siginfo_type_p (gdbarch))
+ {
+ struct type *type = gdbarch_get_siginfo_type (gdbarch);
+ size_t len = TYPE_LENGTH (type);
+
+ siginfo_data.reset ((gdb_byte *) xmalloc (len));
+
+ if (target_read (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
+ siginfo_data.get (), 0, len) != len)
+ {
+ /* Errors ignored. */
+ siginfo_data.reset (nullptr);
+ }
+ }
+
+ if (siginfo_data)
+ {
+ m_siginfo_gdbarch = gdbarch;
+ m_siginfo_data = std::move (siginfo_data);
+ }
+ }
+
+ /* Return a pointer to the stored register state. */
+
+ readonly_detached_regcache * get_registers () const
+ {
+ return m_registers.get ();
+ }
+
+ /* Restores the stored state into GDBARCH, TP, and REGCACHE. */
+
+ void restore (struct gdbarch *gdbarch,
+ struct thread_info *tp,
+ struct regcache *regcache) const
+ {
+ tp->suspend = m_thread_suspend;
+
+ if (m_siginfo_gdbarch == gdbarch)
+ {
+ struct type *type = gdbarch_get_siginfo_type (gdbarch);
+
+ /* Errors ignored. */
+ target_write (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
+ m_siginfo_data.get (), 0, TYPE_LENGTH (type));
+ }
+
+ /* The inferior can be gone if the user types "print exit(0)"
+ (and perhaps other times). */
+ if (target_has_execution)
+ /* NB: The register write goes through to the target. */
+ regcache->restore (get_registers ());
+ }
+
+private:
+ /* How the current thread stopped before the inferior function call was
+ executed. */
+ struct thread_suspend_state m_thread_suspend;
+
+ /* The registers before the inferior function call was executed. */
+ std::unique_ptr<readonly_detached_regcache> m_registers;

/* Format of SIGINFO_DATA or NULL if it is not present. */
- struct gdbarch *siginfo_gdbarch = nullptr;
+ struct gdbarch *m_siginfo_gdbarch = nullptr;

/* The inferior format depends on SIGINFO_GDBARCH and it has a length of
TYPE_LENGTH (gdbarch_get_siginfo_type ()). For different gdbarch the
content would be invalid. */
- gdb::unique_xmalloc_ptr<gdb_byte> siginfo_data;
+ gdb::unique_xmalloc_ptr<gdb_byte> m_siginfo_data;
};

infcall_suspend_state_up
@@ -8762,39 +8829,16 @@ save_infcall_suspend_state ()
struct thread_info *tp = inferior_thread ();
struct regcache *regcache = get_current_regcache ();
struct gdbarch *gdbarch = regcache->arch ();
- gdb::unique_xmalloc_ptr<gdb_byte> siginfo_data;
-
- if (gdbarch_get_siginfo_type_p (gdbarch))
- {
- struct type *type = gdbarch_get_siginfo_type (gdbarch);
- size_t len = TYPE_LENGTH (type);
-
- siginfo_data.reset ((gdb_byte *) xmalloc (len));
-
- if (target_read (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
- siginfo_data.get (), 0, len) != len)
- {
- /* Errors ignored. */
- siginfo_data.reset (nullptr);
- }
- }
-
- infcall_suspend_state_up inf_state (new struct infcall_suspend_state);

- if (siginfo_data)
- {
- inf_state->siginfo_gdbarch = gdbarch;
- inf_state->siginfo_data = std::move (siginfo_data);
- }
-
- inf_state->thread_suspend = tp->suspend;
+ infcall_suspend_state_up inf_state
+ (new struct infcall_suspend_state (gdbarch, tp, regcache));

- /* run_inferior_call will not use the signal due to its `proceed' call with
- GDB_SIGNAL_0 anyway. */
+ /* Having saved the current state, adjust the thread state, discarding
+ any stop signal information, this is not useful when starting an
+ inferior function call and run_inferior_call will not use the signal
+ due to its `proceed' call with GDB_SIGNAL_0. */
tp->suspend.stop_signal = GDB_SIGNAL_0;

- inf_state->registers.reset (new readonly_detached_regcache (*regcache));
-
return inf_state;
}

@@ -8807,23 +8851,7 @@ restore_infcall_suspend_state (struct infcall_suspend_state *inf_state)
struct regcache *regcache = get_current_regcache ();
struct gdbarch *gdbarch = regcache->arch ();

- tp->suspend = inf_state->thread_suspend;
-
- if (inf_state->siginfo_gdbarch == gdbarch)
- {
- struct type *type = gdbarch_get_siginfo_type (gdbarch);
-
- /* Errors ignored. */
- target_write (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
- inf_state->siginfo_data.get (), 0, TYPE_LENGTH (type));
- }
-
- /* The inferior can be gone if the user types "print exit(0)"
- (and perhaps other times). */
- if (target_has_execution)
- /* NB: The register write goes through to the target. */
- regcache->restore (inf_state->registers.get ());
-
+ inf_state->restore (gdbarch, tp, regcache);
discard_infcall_suspend_state (inf_state);
}

@@ -8836,7 +8864,7 @@ discard_infcall_suspend_state (struct infcall_suspend_state *inf_state)
readonly_detached_regcache *
get_infcall_suspend_state_regcache (struct infcall_suspend_state *inf_state)
{
- return inf_state->registers.get ();
+ return inf_state->get_registers ();
}

/* infcall_control_state contains state regarding gdb's control of the

[Andrew Burgess]

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

diff --git a/gdb/regcache.c b/gdb/regcache.c
index 6e0e8c3e7e0..c9503295f59 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);

[Pedro Alves]

Post by Andrew Burgess
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.

Post by Andrew Burgess
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?

Post by Andrew Burgess
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

[Andrew Burgess]

Post by Pedro Alves

Post by Andrew Burgess
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.

Post by Andrew Burgess
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?

Post by Andrew Burgess
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

[Pedro Alves]

Post by Andrew Burgess
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.

Post by Andrew Burgess
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

[Andrew Burgess]

When running the test gdb.base/annota1.exp with:

make check-gdb RUNTESTFLAGS="--target_board=native-extended-gdbserver gdb.base/annota1.exp"

I would see a failure due to some unexpecte lines in GDB's output.
The extra lines (when compared with a native run) were about file
transfer from the remote back to GDB.

This commit extends the regexp for this test to allow for these extra
lines, and also splits the rather long regexp up into a list of parts.

With this change in place I see no failures for gdb.base/annota1.exp
when using the native-extended-gdbserver target board, nor with a
native run on X86-64/Linux.

gdb/testsuite/ChangeLog:

* gdb.base/annota1.exp: Update a test regexp.
---
gdb/testsuite/ChangeLog | 4 ++++
gdb/testsuite/gdb.base/annota1.exp | 23 +++++++++++++++++++++--
2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.base/annota1.exp b/gdb/testsuite/gdb.base/annota1.exp
index 4b34aa84f29..b5a0e87c3ad 100644
--- a/gdb/testsuite/gdb.base/annota1.exp
+++ b/gdb/testsuite/gdb.base/annota1.exp
@@ -127,8 +127,27 @@ gdb_test_multiple "info break" "breakpoint info" {
#exp_internal 1
set binexp [string_to_regexp $binfile]
gdb_test_multiple "run" "run until main breakpoint" {
- -re "\r\n\032\032post-prompt\r\nStarting program: $binexp \(\r\nwarning: Skipping \[^\r\n\]+ .gdb_index section in \[^\r\n\]+\r\nDo \"set use-deprecated-index-sections on\" before the file is read\r\nto use the section anyway\\.\)?\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)*\r\n\r\n\032\032starting\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)*\r\n\r\n\032\032breakpoint 1\r\n\r\nBreakpoint 1, \r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nmain\r\n\032\032frame-args\r\n \\(\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n.*annota1.c\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n$main_line\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source.*$srcfile:$main_line:.*:beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped.*$gdb_prompt$" {
- pass "run until main breakpoint"
+ -re [join { "\r\n\032\032post-prompt\r\nStarting program: $binexp " \
+ "\(\(\r\nReading \[^\r\n\]+\)|\(\r\nwarning: File transfers from remote targets can be slow\[^\r\n\]+\)\)*" \
+ "\(\r\nwarning: Skipping \[^\r\n\]+ .gdb_index section in \[^\r\n\]+\r\nDo \"set use-deprecated-index-sections on\" before the file is read\r\nto use the section anyway\\.\)?" \
+ "\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)*\r\n\r\n" \
+ "\032\032starting\(\(\r\nReading \[^\r\n\]+\)|\(\r\nwarning: File transfers from remote targets can be slow\[^\r\n\]+\)\)*" \
+ "\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)*\r\n\r\n" \
+ "\032\032breakpoint 1\r\n\r\n" \
+ "Breakpoint 1, \r\n" \
+ "\032\032frame-begin 0 $hex\r\n\r\n" \
+ "\032\032frame-function-name\r\n" \
+ "main\r\n" \
+ "\032\032frame-args\r\n \\(\\)\r\n" \
+ "\032\032frame-source-begin\r\n at \r\n" \
+ "\032\032frame-source-file\r\n.*annota1.c\r\n" \
+ "\032\032frame-source-file-end\r\n:\r\n" \
+ "\032\032frame-source-line\r\n$main_line\r\n" \
+ "\032\032frame-source-end\r\n\r\n\r\n" \
+ "\032\032source.*$srcfile:$main_line:.*:beg:$hex\r\n\r\n" \
+ "\032\032frame-end\r\n\r\n" \
+ "\032\032stopped.*$gdb_prompt$" } ] {
+ pass "run until main breakpoint"
}
}
#exp_internal 0