Re: PR/52117 CVS commit: src/tests/kernel

[Kamil Rytarowski <krytarowski%gmail.com@localhost>]

The following reply was made to PR kern/52117; it has been noted by GNATS.

From: Kamil Rytarowski <krytarowski%gmail.com@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: 
Subject: Re: PR/52117 CVS commit: src/tests/kernel
Date: Tue, 28 Mar 2017 23:40:41 +0200

 On 28.03.2017 22:00, Martin Husemann wrote:
 > The following reply was made to PR kern/52117; it has been noted by GNATS.
 > 
 > From: Martin Husemann <martin%duskware.de@localhost>
 > To: gnats-bugs%NetBSD.org@localhost
 > Cc: n54%gmx.com@localhost
 > Subject: Re: PR/52117 CVS commit: src/tests/kernel
 > Date: Tue, 28 Mar 2017 21:55:03 +0200
 > 
 >  Ok, here is an alternative patch which seems to work for me:
 >  
 >  [..]
 >  Before calling wait4() for the child 38
 >  Reported PTRACE_FORK event with forkee 332
 >  Before calling wait4() for the forkee 332 of the child 38
 >  Before resuming the forkee process where it left off and without signal to be sent
 >  Before resuming the child process where it left off and without signal to be sent
 >  Before calling wait4() for the forkee - expected exited
 >  Before calling wait4() for the forkee - expected no process
 >  Before calling wait4() for the child - expected stopped SIGCHLD
 >  Before resuming the child process where it left off and without signal to be sent
 >  Before exiting of the child process
 >  Before calling wait4() for the child - expected exited
 >  Before calling wait4() for the child - expected no process
 >  failed: Test case was expecting a failure but none were raised
 >  
 >  
 >  It has the advantage of not touching all existing child_return, but doing the
 >  work earlier. It is slightly hackish, but kpsignal2 already has special case
 >  code for early signals, so I'd consider it OK.
 >  
 >  What do others think?
 >  
 >  Patch below, obvious removal from x86/x86/syscall.c ommited.
 >  
 >  Martin
 >  
 >  Index: kern_fork.c
 >  ===================================================================
 >  RCS file: /cvsroot/src/sys/kern/kern_fork.c,v
 >  retrieving revision 1.199
 >  diff -u -p -r1.199 kern_fork.c
 >  --- kern_fork.c	13 Jan 2017 23:00:35 -0000	1.199
 >  +++ kern_fork.c	28 Mar 2017 19:53:34 -0000
 >  @@ -500,6 +500,16 @@ fork1(struct lwp *l1, int flags, int exi
 >   	(*p2->p_emul->e_syscall_intern)(p2);
 >   #endif
 >   
 >  +	/* if we are being traced, give the owner a chance to interfere */
 >  +	if (p2->p_slflag & PSL_TRACED) {
 >  +		ksiginfo_t ksi;
 >  +
 >  +                KSI_INIT_EMPTY(&ksi);
 >  +		ksi.ksi_signo = SIGTRAP;
 >  +		ksi.ksi_lid = l2->l_lid;
 >  +		kpsignal(p2, &ksi, NULL);
 >  +	}
 >  +
 >   	/*
 >   	 * Update stats now that we know the fork was successful.
 >   	 */
 >  Index: kern_sig.c
 >  ===================================================================
 >  RCS file: /cvsroot/src/sys/kern/kern_sig.c,v
 >  retrieving revision 1.334
 >  diff -u -p -r1.334 kern_sig.c
 >  --- kern_sig.c	24 Mar 2017 17:40:44 -0000	1.334
 >  +++ kern_sig.c	28 Mar 2017 19:53:34 -0000
 >  @@ -1224,7 +1224,7 @@ kpsignal2(struct proc *p, ksiginfo_t *ks
 >   	ksiginfo_t *kp;
 >   	lwpid_t lid;
 >   	sig_t action;
 >  -	bool toall;
 >  +	bool toall, debtrap = false;
 >   	int error = 0;
 >   
 >   	KASSERT(!cpu_intr_p());
 >  @@ -1237,8 +1237,13 @@ kpsignal2(struct proc *p, ksiginfo_t *ks
 >   	 * If the process is being created by fork, is a zombie or is
 >   	 * exiting, then just drop the signal here and bail out.
 >   	 */
 >  -	if (p->p_stat != SACTIVE && p->p_stat != SSTOP)
 >  +	if (p->p_stat == SIDL && signo == SIGTRAP
 >  +	    && (p->p_slflag & PSL_TRACED)) {
 >  +		/* allow an initial SIGTRAP for traced processes */
 >  +		debtrap = true;
 >  +	} else if (p->p_stat != SACTIVE && p->p_stat != SSTOP) {
 >   		return 0;
 >  +	}
 >   
 >   	/* XXX for core dump/debugger */
 >   	p->p_sigctx.ps_lwp = ksi->ksi_lid;
 >  @@ -1353,7 +1358,13 @@ kpsignal2(struct proc *p, ksiginfo_t *ks
 >   	 * the signal to it.
 >   	 */
 >   	if (lid != 0) {
 >  -		l = lwp_find(p, lid);
 >  +		if (__predict_false(debtrap)) {
 >  +			l = LIST_FIRST(&p->p_lwps);
 >  +			if (l->l_lid != lid)
 >  +				l = NULL;
 >  +		} else {
 >  +			l = lwp_find(p, lid);
 >  +		}
 >   		if (l != NULL) {
 >   			if ((error = sigput(&l->l_sigpend, p, kp)) != 0)
 >   				goto out;
 >  
 > 
 
 I'm fine with both.
 
 For consistency I would add "ksi.ksi_signo = TRAP_CHLD;". My motivation
 is to attribute every SIGTRAP in the kernel with appropriate si_code,
 even if it works for some reasons in some scenarios without them.
 
 From user-space point of view we want a deterministic order of receiving
 signals by a debugger. Signal for "child" and next "child2".
 
 I prefer the <sys/childreturn.h> version as it has bit less overhead...
 but it might be just a microoptimization not worth modifying the
 child_return code in all ports.