Re: [PATCH 2/3] gdb: make thread_suspend_state::stop_pc optional

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-08-30 4:03 p.m., Andrew Burgess wrote:
> Currently the stop_pc field of thread_suspect_state is a CORE_ADDR and
> when we want to indicate that there is no stop_pc available we set
> this field back to a special value.
> 
> There are actually two special values used, in post_create_inferior
> the stop_pc is set to 0.  This is a little unfortunate, there are
> plenty of embedded targets where 0 is a valid pc address.  The more
> common special value for stop_pc was set in
> thread_info::set_executing, where the value (~(CORE_ADDR) 0) was used.
> 
> This commit changes things so that the stop_pc is instead a
> gdb::optional.  We can now explicitly reset the field to an
> uninitialised state, we also have (when compiling with _GLIBCXX_DEBUG
> defined) asserts that we don't read the stop_pc when its in an
> uninitialised state (see gdbsupport/gdb_optional.h).

Thanks, I think it's a good idea.

> One situation where a thread will not have a stop_pc value is when the
> thread is stopped as a consequence of GDB being in all stop mode, and
> some other thread stopped at an interesting event.  When GDB brings
> all the other threads to a stop those other threads will not have a
> stop_pc set (thus avoiding an unnecessary read of $pc).
> 
> Previously, when GDB passed through handle_one (in infrun.c) the
> threads executing flag was set to false and the stop_pc field was left
> unchanged, i.e. it would (previous) have been left as ~0.
> 
> Now, handle_one leaves the stop_pc with no value.
> 
> This caused a problem when we later try to set these threads running
> again, in proceed() we compare the current pc with the cached
> stop_pc.  If the thread was stopped in via handle_one then the stop_pc
> would have been left as ~0, and the compare (in proceed)
> would (likely) fail.  Now however, this compare tries to read the
> stop_pc when it has no value, this would trigger an assert.
> 
> To resolve this I've added thread_info::stop_pc_p() which returns true
> if the thread has a cached stop_pc.  We should only ever call
> thread_info::stop_pc() if we know that there is a cached stop_pc.

We could also make stop_pc return gdb::optional<CORE_ADDR>.  I think it
would be slightly better, since anybody calling stop_pc would see that
it returns an optional and be forced to consider that.  Otherwise, one
could call stop_pc and not know that stop_pc_p exists.  But otherwise
it's the same.

LGTM in any case.

Simon