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

[Dmitry V. Levin]

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?


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20180420/f4184a75/attachment.bin>