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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-09-07 10:10:07 -0400]:

> 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

Thanks, I pushed this patch with that change in place.

Andrew