Re: [PATCH v3] Pass thread_info pointer to various inferior control functions

[Simon Marchi <simon.marchi@polymtl.ca>]

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