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

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

On 2021-09-07 9:21 a.m., Andrew Burgess wrote:
> * Simon Marchi <simon.marchi@polymtl.ca> [2021-09-01 10:23:32 -0400]:
> 
>>
>>
>> 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.
> 
> I did consider that initially, but most of the places where
> thread_info::stop_pc is called the value is being immediately passed
> through to some other function, here's an example pulled randomly from
> infrun.c:
> 
>       ecs->event_thread->control.stop_bpstat
> 	= bpstat_stop_status (get_current_regcache ()->aspace (),
> 			      ecs->event_thread->stop_pc (),
> 			      ecs->event_thread, &ecs->ws);
> 
> if we are returned a gdb::optional<> then we might change the code to
> do this:
> 
>       ecs->event_thread->control.stop_bpstat
> 	= bpstat_stop_status (get_current_regcache ()->aspace (),
> 			      *ecs->event_thread->stop_pc (),
> 			      ecs->event_thread, &ecs->ws);
> 
> Or maybe, like this:
> 
>       auto stop_pc = ecs->event_thread->stop_pc ();
>       gdb_assert (stop_pc.has_value ());
>       ecs->event_thread->control.stop_bpstat
> 	= bpstat_stop_status (get_current_regcache ()->aspace (),
> 			      *stop_pc,
> 			      ecs->event_thread, &ecs->ws);
> 
> In the first case, it doesn't feel like we've gained much over my
> patch, where thead_info::stop_pc() accesses the value for us.
> Further, once we've normalised the pattern of accessing the stop_pc as
> `*ecs->event_thread->stop_pc ()`, I worry people still wouldn't
> actually consider whether the stop_pc value was valid or not, they'd
> just duplicate the existing code.
> 
> The second case seems excessively verbose, so much so, that you might
> even be tempted to write a wrapper, say thread_info::stop_pc_value(),
> which kind lands us back on my original patch...
> 
> Initially, I'd relied on the asserts within gdb::optional to ensure
> that we didn't access the stop_pc when it had no value, but these
> asserts are only present when compiling with _GLIBCXX_DEBUG defined -
> I do this, but it's certainly not going to be standard in a release
> build of GDB.  So, I wonder if this would be a good change:
> 
>   /* Return this thread's stop PC.  This should only be called when it is
>      known that stop_pc has a value.  If this function is being used in a
>      situation where a thread may not have had a stop_pc assigned, then
>      stop_pc_p() can be used to check if the stop_pc is defined.  */
> 
>   CORE_ADDR stop_pc () const
>   {
>     gdb_assert (m_suspend.stop_pc.has_value ());
>     return *m_suspend.stop_pc;
>   }

That one looks good to me, stating preconditions in the documentation
and enforcing them using assertions.  That's similar to how I designed
other things, like dynamic_prop.

Simon