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

[Simon Marchi]

On 2018-01-18 11:41, Pedro Alves wrote:
> 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.

Ok.

> 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!

Yes this is the idea.  The ramifications go very deep, so it's hard to 
know what still relies on the current inferior being set.  This is just 
one step in the right direction, and it's easier to take smaller steps.

> 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.

That's a good idea, this way it will also check if I passed the wrong 
thing to target_detach.

Simon