[v3] Pass thread_info pointer to various inferior control functions

  From: Simon Marchi <simon.marchi@polymtl.ca>

[ Migrating this from Gerrit: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/321 ]

I noticed that some functions in infcmd and infrun call each other and
all call inferior_thread, while they could just get the thread_info
pointer from their caller.  That means less calls to inferior_thread, so
less reliance on global state, since inferior_thread reads
inferior_ptid.

The paths I am unsure about are:

  - fetch_inferior_event calls...
  - step_command_fsm::should_stop calls...
  - prepare_one_step

and

 - process_event_stop_test calls...
 - set_step_info

Before this patch, prepare_one_step gets the thread pointer using
inferior_thread.  After this patch, it gets it from the
execution_control_state structure in fetch_inferior_event.  Are we sure
that the thread from the execution_control_state structure is the same
as the one inferior_thread would return?  This code path is used when a
thread completes a step, but the user had specified a step count (e.g.
"step 5") so we decide to do one more step.  It would be strange (and
even a bug I suppose) if the thread in the ecs structure in
fetch_inferior_event was not the same thread that is prepared to stepped
by prepare_one_step.  So I believe passing the ecs thread is fine.

The same logic applies to process_event_stop_test calling
set_step_info.

gdb/ChangeLog:

	* infrun.h: Forward-declare thread_info.
	(set_step_info): Add thread_info parameter, add doc.
	* infrun.c (set_step_info): Add thread_info parameter, move doc
	to header.
	* infrun.c (process_event_stop_test): Pass thread to
	set_step_info call.
	* infcmd.c (set_step_frame): Add thread_info pointer, pass it to
	set_step_info.
	(prepare_one_step): Add thread_info parameter, pass it to
	set_step_frame and prepare_one_step (recursive) call.
	(step_1): Pass thread to prepare_one_step call.
	(step_command_fsm::should_stop): Pass thread to
	prepare_one_step.
	(until_next_fsm): Pass thread to set_step_frame call.
	(finish_command): Pass thread to set_step_info call.
---
 gdb/infcmd.c | 29 +++++++++++------------------
 gdb/infrun.c | 10 +++++-----
 gdb/infrun.h |  5 ++++-
 3 files changed, 20 insertions(+), 24 deletions(-)
  

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> I noticed that some functions in infcmd and infrun call each other and
Simon> all call inferior_thread, while they could just get the thread_info
Simon> pointer from their caller.  That means less calls to inferior_thread, so
Simon> less reliance on global state, since inferior_thread reads
Simon> inferior_ptid.

...

Simon> -set_step_frame (void)
Simon> +set_step_frame (thread_info *tp)
Simon>  {
Simon>    frame_info *frame = get_current_frame ();

I like the idea of passing parameters rather than relying on global
state.  However, in its current form, I think this patch may lull the
reader into a false sense of security.

That is, it makes it seems like these functions operate on a thread that
you pass in.  However, they don't actually.  For example, in this one, I
think get_current_frame must be relying on the global inferior_ptid.
So, this would change the function from something that obviously has to
be relying on globals to something that un-obviously is.

I don't know whether that's a reason to reject it or not.
It just seemed like maybe a future source of bugs.

I do welcome changes to reduce our reliance on globals.
Perhaps there's an argument that we can only achieve this incrementally
like this.  Though, the follow-on question there is whether this is
something that will actually happen.

Let me know what you think of this.

thanks,
Tom
  

On 2020-02-18 3:52 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> I noticed that some functions in infcmd and infrun call each other and
> Simon> all call inferior_thread, while they could just get the thread_info
> Simon> pointer from their caller.  That means less calls to inferior_thread, so
> Simon> less reliance on global state, since inferior_thread reads
> Simon> inferior_ptid.
> 
> ...
> 
> Simon> -set_step_frame (void)
> Simon> +set_step_frame (thread_info *tp)
> Simon>  {
> Simon>    frame_info *frame = get_current_frame ();
> 
> I like the idea of passing parameters rather than relying on global
> state.  However, in its current form, I think this patch may lull the
> reader into a false sense of security.
> 
> That is, it makes it seems like these functions operate on a thread that
> you pass in.  However, they don't actually.  For example, in this one, I
> think get_current_frame must be relying on the global inferior_ptid.
> So, this would change the function from something that obviously has to
> be relying on globals to something that un-obviously is.

I agree.

> I don't know whether that's a reason to reject it or not.
> It just seemed like maybe a future source of bugs.
> 
> I do welcome changes to reduce our reliance on globals.
> Perhaps there's an argument that we can only achieve this incrementally
> like this.

That's what I think, we have to start somewhere, and for some time be in this
weird state where we pass in things by parameter but also rely on globals.
Perhaps a good way would be to add this parameter, but also assert at the
entry of the function that the passed in parameter matches the global (of
course, with a comment that says why).  When we are convinced the code does
not rely on globals anymore, we can remove the assertion.

There's a precedent for such a thing here (altough I added it, so it might
not count):

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/target.c;h=470ef51d69ef5d712fc51a54f426c1cd3b79c977;hb=HEAD#l2020

So here, I would add this at the beginning of step_step_info, set_step_frame
and prepare_one_step:

  /* This can be removed once this function does not rely on global state
     anymore.  */
  gdb_assert (inferior_ptid == tp->ptid);

>Though, the follow-on question there is whether this is
> something that will actually happen.

I want to believe.

> 
> Let me know what you think of this.

Simon
  

On 2020-02-18 4:12 p.m., Simon Marchi wrote:
> On 2020-02-18 3:52 p.m., Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
>>
>> Simon> I noticed that some functions in infcmd and infrun call each other and
>> Simon> all call inferior_thread, while they could just get the thread_info
>> Simon> pointer from their caller.  That means less calls to inferior_thread, so
>> Simon> less reliance on global state, since inferior_thread reads
>> Simon> inferior_ptid.
>>
>> ...
>>
>> Simon> -set_step_frame (void)
>> Simon> +set_step_frame (thread_info *tp)
>> Simon>  {
>> Simon>    frame_info *frame = get_current_frame ();
>>
>> I like the idea of passing parameters rather than relying on global
>> state.  However, in its current form, I think this patch may lull the
>> reader into a false sense of security.
>>
>> That is, it makes it seems like these functions operate on a thread that
>> you pass in.  However, they don't actually.  For example, in this one, I
>> think get_current_frame must be relying on the global inferior_ptid.
>> So, this would change the function from something that obviously has to
>> be relying on globals to something that un-obviously is.
> 
> I agree.
> 
>> I don't know whether that's a reason to reject it or not.
>> It just seemed like maybe a future source of bugs.
>>
>> I do welcome changes to reduce our reliance on globals.
>> Perhaps there's an argument that we can only achieve this incrementally
>> like this.
> 
> That's what I think, we have to start somewhere, and for some time be in this
> weird state where we pass in things by parameter but also rely on globals.
> Perhaps a good way would be to add this parameter, but also assert at the
> entry of the function that the passed in parameter matches the global (of
> course, with a comment that says why).  When we are convinced the code does
> not rely on globals anymore, we can remove the assertion.
> 
> There's a precedent for such a thing here (altough I added it, so it might
> not count):
> 
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/target.c;h=470ef51d69ef5d712fc51a54f426c1cd3b79c977;hb=HEAD#l2020
> 
> So here, I would add this at the beginning of step_step_info, set_step_frame
> and prepare_one_step:
> 
>   /* This can be removed once this function does not rely on global state
>      anymore.  */
>   gdb_assert (inferior_ptid == tp->ptid);
> 
>> Though, the follow-on question there is whether this is
>> something that will actually happen.
> 
> I want to believe.
> 
>>
>> Let me know what you think of this.
> 
> Simon
> 

I've pushed the patch after adding these assertions.

Simon