This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 01/16 v2] Refactor native follow-fork


One clarification...

On 9/5/2014 11:56 AM, Breazeal, Don wrote:
> Hi Pedro,
> Thanks for reviewing this.
> 
> On 9/5/2014 7:20 AM, Pedro Alves wrote:
>> linux_child_follow_fork ends up with:
>>
>> static int
>> linux_child_follow_fork (struct target_ops *ops, int follow_child,
>> 			 int detach_fork)
>> {
>>   int has_vforked;
>>   int parent_pid, child_pid;
>>
>>   has_vforked = (inferior_thread ()->pending_follow.kind
>> 		 == TARGET_WAITKIND_VFORKED);
>>   parent_pid = ptid_get_lwp (inferior_ptid);
>>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>   if (parent_pid == 0)
>>     parent_pid = ptid_get_pid (inferior_ptid);
>>   child_pid
>>     = ptid_get_pid (inferior_thread ()->pending_follow.value.related_pid);
>>
>>   if (!follow_child)
>>     {
>> ...
>>     }
>>   else
>>     {
>>       struct lwp_info *child_lp;
>>
>>       child_lp = add_lwp (inferior_ptid);
>>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>       child_lp->stopped = 1;
>>       child_lp->last_resume_kind = resume_stop;
>>
>>       /* Let the thread_db layer learn about this new process.  */
>>       check_for_thread_db ();
>>     }
>> }
>>
>> Nothing appears to switch inferior_ptid to the child, so seems
>> like we're adding the child_lp with the wrong lwp (and calling
>> check_for_thread_db in the wrong context) ?  Is this managing
>> to work by chance because follow_fork_inferior leaves inferior_ptid
>> pointing to the child?  
> 
> Yes, follow_fork_inferior always sets inferior_ptid to the followed
> inferior.  On entry, linux_child_follow_fork expects inferior_ptid to be
> the followed inferior.  So I think it is getting the correct inferior
> from inferior_ptid in these cases.  I can change that if you prefer; see
> my question below about acceptable solutions.

Er... I can change how inferior_ptid is passed to
linux_child_follow_fork, not whether the correct ptid is used. :-P

> 
> Regarding check_for_thread_db, there is something unrelated that I don't
> understand here.  If we have reached this function, then aren't we
> guaranteed that PTRACE_O_TRACECLONE is supported, and that we are using
> that instead of libthread_db for detecting thread events?  If so, why do
> we need to call check_for_thread_db at all?
> 
> Then this at the top uses the wrong
>> inferior_thread ():
>>
>>   has_vforked = (inferior_thread ()->pending_follow.kind
>> 		 == TARGET_WAITKIND_VFORKED);
>>
>>
>> and we're lucky that nothing end up using has_vforked in the
>> follow child path?
> 
> You are right, this is incorrect and unnecessary in the case where we
> are following the child.
> 
>>
>> I'd much rather we don't have these assumptions in place.
> 
> Would an acceptable solution be to move the definitions and assignments
> of has_vforked, parent_pid, and child_pid into the follow-parent case,
> as below?
> 
> Would you also prefer that on entry to linux_child_follow_fork,
> inferior_ptid is set to the parent like it was before, or would a
> comment explaining that inferior_ptid is expected to be the followed
> inferior be sufficient?
> 
> static int
> linux_child_follow_fork (struct target_ops *ops, int follow_child,
>                          int detach_fork)
> {
>   if (!follow_child)
>     {
>       struct lwp_info *child_lp = NULL;
>       int status = W_STOPCODE (0);
>       struct cleanup *old_chain;
>       int has_vforked;
>       int parent_pid, child_pid;
> 
>       has_vforked = (inferior_thread ()->pending_follow.kind
>                      == TARGET_WAITKIND_VFORKED);
>       parent_pid = ptid_get_lwp (inferior_ptid);
>       if (parent_pid == 0)
>         parent_pid = ptid_get_pid (inferior_ptid);
>       child_pid
>         = ptid_get_pid (inferior_thread
> ()->pending_follow.value.related_pid);
> 
>>
>> These files / targets also have to_follow_fork implementations:
>>
>>  inf-ptrace.c:  t->to_follow_fork = inf_ptrace_follow_fork;
>>  inf-ttrace.c:  t->to_follow_fork = inf_ttrace_follow_fork;
>>
>> which will break if we don't adjust them as well.  Did you
>> check whether the refactored code (follow_fork_inferior)
>> makes sense for those?
> 
> I completely missed these; sorry about that.  In theory I should be able
> to make similar changes to these that maintains the existing
> functionality.  I don't currently have a way (that I know of) to test
> either of them.  Testing requires a non-Linux version of Unix and an
> HP-UX system, correct?  I'll start work on the changes in spite of that.
> 
>>
>> Thanks,
>> Pedro Alves
>>
> 
> Thanks,
> --Don
> 
> 



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]