[PATCH] unwind: initialize unwind context only if given tcb is initialized

[Masatake YAMATO]

On Fri, 20 Apr 2018 21:27:49 +0300, "Dmitry V. Levin" <ldv at altlinux.org> wrote:
> On Sat, Apr 21, 2018 at 01:50:46AM +0900, Masatake YAMATO wrote:
>> On Fri, 20 Apr 2018 17:50:50 +0300, "Dmitry V. Levin" <ldv at altlinux.org> wrote:
>> > On Fri, Apr 20, 2018 at 04:38:15AM +0300, Dmitry V. Levin wrote:
>> >> On Fri, Apr 20, 2018 at 09:47:44AM +0900, Masatake YAMATO wrote:
>> >> > On Fri, 20 Apr 2018 01:44:51 +0300, "Dmitry V. Levin" <ldv at altlinux.org> wrote:
>> >> > >> --- a/strace.c
>> >> > >> +++ b/strace.c
>> >> > >> @@ -1783,6 +1783,8 @@ init(int argc, char *argv[])
>> >> > >>  
>> >> > >>  		unwind_init();
>> >> > >>  		for (tcbi = 0; tcbi < tcbtabsize; ++tcbi) {
>> >> > >> +			if (!tcbtab[tcbi]->pid)
>> >> > >> +				continue;
>> >> > >>  			unwind_tcb_init(tcbtab[tcbi]);
>> >> > >>  		}
>> >> > >>  	}
>> >> > > 
>> >> > > Apparently, we don't need this loop at all, there is no use to call
>> >> > > unwind_tcb_init until the tracee is actually attached.  The proper place
>> >> > > for unwind_tcb_init invocation is the same as for newoutf.
>> >> > > I've pushed a few commits to implement this change.
>> >> > 
>> >> > Much better fix.
>> >> > The function name is also improved.
>> >> > 
>> >> > One thing I cannot be convince is the following code block:
>> >> > 
>> >> >     static struct tcb *
>> >> >     maybe_switch_tcbs(struct tcb *tcp, const int pid)
>> >> >     {
>> >> > 	    ...	
>> >> > 	    tcp->pid = pid;
>> >> > 
>> >> > pid of tcb is updated. I wonder whether unwind context can be attached
>> >> > well to the modified(?) tcb.
>> >> 
>> >> Good catch, looks like this code is out of date as more fields were added
>> >> to struct tcb.  I suppose switching whole structures might be a better
>> >> alternative to cherry-picking relevant fields.
>> > 
>> > On second thought, maybe_switch_tcbs already swaps whole structures
>> > except a few pid-related fields.  So the question is, do we have
>> > any more pid-related fields that should be excluded?
>> 
>> What I know are unwind_ctx and mmap_cache.
> 
> I suppose mmap_cache has already been dealt with because it's a syscall
> that has STACKTRACE_INVALIDATE_CACHE flag set.
> 
> We could call unwind_tcb_fin/unwind_tcb_init if the pid is cached
> by unwinder.
> 
>> How about introducing hooks where a subsystem can register itself to
>> like:
>> 
>> diff --git a/strace.c b/strace.c
>> index 7b79b0c5..f3adf547 100644
>> --- a/strace.c
>> +++ b/strace.c
>> @@ -2080,8 +2080,14 @@ maybe_switch_tcbs(struct tcb *tcp, const int pid)
>>  	/* Drop leader, but close execve'd thread outfile (if -ff) */
>>  	droptcb(tcp);
>>  	/* Switch to the thread, reusing leader's outfile and pid */
>> +
>> +	/* Give a chance to cleanup resources attached to tcb to subsystems. */
>> +	run_subsystem_changing_pid_pre_hook(tcp, pid);
> 
> It's too late after droptcb(tcp);
> 
>>  	tcp = execve_thread;
> 
> We might want to call unwind_tcb_fin(tcp) at this point, and ...
> 
>> +	old_pid = tcp->pid;
>>  	tcp->pid = pid;
> 
> ... unwind_tcb_init(tcp) at this point, if necessary.
> 
>> +	/* Give a chance to attach newly allocate resources to subsystems.  */
>> +	run_subsystem_changing_pid_post_hook(tcp, old_pid);
> 
> Do you think we might need this?

I read the code again with considering abve your comment
Yes, what we need about unwind_ctx and mmap_cache is just

+#ifdef ENABLE_STACKTRACE
+	if (stack_trace_enabled)
+		unwind_tcb_init(tcp);
+#endif

as you wrote.

Masatake YAMATO

> 
> -- 
> ldv