Re: [PATCH 2/3] Pass inferior down to target_detach and to_detach

[Pedro Alves <palves@redhat.com>]

On 12/31/2017 05:50 AM, Simon Marchi wrote:
> The to_detach target_ops method implementations are currently expected
> to work on current_inferior/inferior_ptid.  In order to make things more
> explicit, and remove some "shadow" parameter passing through globals,
> this patch adds an "inferior" parameter to to_detach.  Implementations
> will be expected to use this instead of relying on the global.  However,
> to keep things simple, this patch only does the minimum that is
> necessary to add the parameter.  The following patch gives an example of
> how one such implementation would be adapted.  If the approach is deemed
> good, we can then look into adapting more implementations.  Until then,
> they'll continue to work as they do currently.

On the multi-target work/branch, I ended up hanging the current
target stack to the current inferior.  Which means that in practice
we have to switch the current inferior/thread before calling any target
method.  I can't see any other practical way.  I've pondered making
every target method take an inferior or some kind of "execution
context" object as parameter, but that's a massive undertaking.
The result is still better for relying more on thread pointers
instead of inferior_ptid / ptid_t, though, IMO.

Note that in practice, even with your patch (in master) we still have
to switch the current inferior before calling target_detach anyway,
for making sure things like the current program space is set correctly,
target_gdbarch(), target description, removing breakpoints, accessing memory,
registers, etc., like here:

>        /* Note that the detach above makes PARENT_INF dangling.  */
> @@ -976,7 +976,7 @@ handle_vfork_child_exec_or_exit (int exec)
>  		}
>  	    }
>  
> -	  target_detach (0);
> +	  target_detach (inf->vfork_parent, 0);

... this still relies on the switch_to_thread call a bit above.

Or look at the prepare_to_detach call in target_detach.

All that said, I'm totally fine with incremental progress.  Actually,
it's probably the only way to go!

So I'm fine with the patch, though, I think we should add a
temporary assertion in target_detach, like:

  gdb_assert (inf == current_inferior ());

until everything beneath either uses an explicit inferior,
or switches the current inferior temporarily.

Thanks,
Pedro Alves