[PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves]

Hi Tom,

Sorry for the delay in responding.  I hate when I end up doing this.

On 4/17/20 7:53 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Pedro> My next attempt is the one that I'm proposing, which is to bite the
> Pedro> bullet and break the connection between inferior_ptid and
> Pedro> inferior_thread(), aka the current thread.  I.e., make the current
> Pedro> thread be a global thread_info pointer that is written to directly by
> Pedro> switch_to_thread, etc., and making inferior_thread() return that
> Pedro> pointer, instead of having inferior_thread() lookup up the
> Pedro> inferior_ptid thread, by ptid_t.
> 
> Does this mean that now the current thread and current inferior can get
> out of sync?  

They already could, because the current thread was based on inferior_ptid,
while the current inferior is stored in a global "inferior *current_inferior_"
pointer.

> Or that these can be out of sync with inferior_ptid?

Yes, before, inferior_ptid could get out of sync with the current
inferior, but not with the current thread.  Afterwards, the current
"thread_info *" can also get out of sync with inferior_ptid.  That's
why most of the series eliminates writes to inferior_ptid directly.

Over time, inferior_ptid usages have been disappearing, replaced by
references to "thread_info *" or "inferior *" objects directly.  

I think that's much better than referring to objects via integers that may
be ambiguous between targets, and worrying about what target_ops owns what
in the ptid_t fields, how to extend ptid for different types of execution
objects, etc.  I think that ideally, in the GDB core, references to
inferior_ptid would virtually disappear.  I see this series as another step
in that direction.

> 
> This patch looks like it tries to avoid that when writing to the current
> thread -- but in the cover letter you mentioned that there are other
> assignments to inferior_ptid.

I think you are referring to this comment:

> "After this, inferior_ptid still exists, but it is mostly read-only and
> mainly used by target backend code.  It is also relied on by a number
> of target methods as a global input argument.  E.g., the target_resume
> interface and the memory reading routines -- we still need it there
> because we need to be able to access memory off of processes for which
> we don't have a corresponding inferior/thread object, like when
> handling forks.  Maybe we could pass down a context explicitly to
> target_read_memory, etc."

In the target_resume case, we always switch inferior_ptid using
switch_to_thread or similar, so not a big deal.

The worrying case I found is the memory access routines.  TBC, that's a case
where the connection between current thread and inferior_ptid was already
being broken previously.  Let me expand on the "handling forks" bit:

When we detach breakpoints from a fork child that we're about to
detach from due to "set detach-on-fork off", we need to uninstall memory
breakpoints from the child, because breakpoint instructions that were
inserted on the parent end up in the child as well, because the child's
address space is just a copy of the parent's.  So to do that, we switch
inferior_ptid to the fork child, and call the breakpoint removal target
method, so that it applies to the fork child.  Note that there are no
thread_info or inferior objects for that fork child in the thread/inferior
lists at all.  So if something in those code breakpoint removal paths (all
the way down the the memory xfer routines) calls inferior_thread() it would
already assert for not finding the thread in the list.  The patch series
doesn't change that.

> I wouldn't block the patches based on this but I'd like to understand
> the direction.  I guess I'd prefer to remove globals and possibilities
> for divergence if it's possible.

Me too, but it's just that inferior_ptid is so pervasive that it's
impossible to eliminate all in one go.

Thanks,
Pedro Alves