Re: [PATCH] gdb: fix darwin-nat.c build / adapt to multi-target

[Simon Marchi <simon dot marchi at efficios dot com>]

On 2020-01-23 1:19 p.m., Pedro Alves wrote:
> On 1/23/20 4:59 PM, Simon Marchi wrote:
>> From: Simon Marchi <smarchi@dev-macosx-01.internal.efficios.com>
>>
>> The darwin-nat.c file doesn't build since the multi-target changes
>> (5b6d1e4f, "Multi-target support").  This patch makes it build.  I have
>> access to a macOS vm, so I am able to build it, but I wasn't able to
>> successfully codesign it and try to actually debug something, so I don't
>> know if it works.  I don't have much more time to put on this to figure
>> it out, so I thought I'd sent the patch anyway, as it's at least a step
>> in the right direction.
>>
>> The bulk of the patch is to change a bunch of functions to be methods of
>> the darwin_nat_target object, so that this can pass `this` to
>> find_inferior_ptid and other functions that now require a
>> process_stratum_target pointer.
>>
>> The darwin_ptrace_him function (renamed to darwin_nat_target::ptrace_him
>> in this patch) is passed to fork_inferior as the `init_trace_fun`
>> parameter.  Since the method can't be passed as a plain function pointer
>> (we need the `this` pointer), I changed the `init_trace_fun` parameter
>> of fork_inferior to be a gdb::function_view, so we can pass a lambda and
>> capture `this`.
> 
> This LGTM.  Thanks for doing this.  
> 
> The use of function_view infork_inferior gave me pause, but it's safe
> since fork_inferior doesn't return until the child either exec'ed or
> exited -- there's no risk that the fork parent returns from the current
> function before the fork child accesses the function_view's closure.

Hmm, but the fork child doesn't use that callback anyway.  I presume you
thought this was about `traceme_fun`.

> Note that this larger patch wasn't really necessary, since for native
> targets there's only ever one process_stratum_target object.  So you
> could instead have referred to the global where you need it.
> But your patch is better.

Oh, I forgot about that.  Well, it's clean this way :).

> 
>> diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
>> index 78f972a7496d..ef5a75292fb1 100644
>> --- a/gdb/nat/fork-inferior.c
>> +++ b/gdb/nat/fork-inferior.c
>> @@ -267,7 +267,8 @@ execv_argv::init_for_shell (const char *exec_file,
>>  pid_t
>>  fork_inferior (const char *exec_file_arg, const std::string &allargs,
>>  	       char **env, void (*traceme_fun) (),
>> -	       void (*init_trace_fun) (int), void (*pre_trace_fun) (),
>> +	       gdb::function_view<void(int)> init_trace_fun,
> 
> There should be a space in "void (int)".

Ack.

I'll push the patch with this fixed, thanks.

Simon