Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 11 May 2015 09:55:25 +0800
From:      Julian Elischer <julian@freebsd.org>
To:        Mark Johnston <markj@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   Re: svn commit: r282744 - in head/sys/cddl/dev/dtrace: amd64 i386
Message-ID:  <55500C0D.7090802@freebsd.org>
In-Reply-To: <201505102227.t4AMRn1g007294@svn.freebsd.org>
References:  <201505102227.t4AMRn1g007294@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On 5/11/15 6:27 AM, Mark Johnston wrote:
> Author: markj
> Date: Sun May 10 22:27:48 2015
> New Revision: 282744
> URL: https://svnweb.freebsd.org/changeset/base/282744
>
> Log:
>    Remove some commented-out upstream code for handling traps from usermode
>    DTrace probes. This handling is already done in trap() on i386 and amd64.

I take issue with the implied assertion that upstream code that is
not needed for FreeBSD should be removed.

Commented out upstream code makes it easier to merge in changes from 
upstream.
it allows patches to be applied and for a lot less worry on the part 
of the merger.
If the code is not there, and there is an upstream change to it, one 
has to ask
"is there equivalent code somewhere else that needs to be patched 
instead?"
But if there is just a

#ifndef  __FreeBSD__ /* this code is not needed in FreeBSD because ...*/
  blah
#endif /*__FreeBSD__  this code is not needed in FreeBSD */

then the merger has a much easier task.

I've just gone through large code merge (where FreeBSD was "upstream") 
and the cases
there the original FreebSD code was still present but #Ifdef'd out 
were SO MUCH easier
to handle.

>
> Modified:
>    head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c
>    head/sys/cddl/dev/dtrace/i386/dtrace_subr.c
>
> Modified: head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c
> ==============================================================================
> --- head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c	Sun May 10 22:21:00 2015	(r282743)
> +++ head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c	Sun May 10 22:27:48 2015	(r282744)
> @@ -142,114 +142,6 @@ dtrace_sync(void)
>   }
>   
>   #ifdef notyet
> -int (*dtrace_pid_probe_ptr)(struct regs *);
> -int (*dtrace_return_probe_ptr)(struct regs *);
> -
> -void
> -dtrace_user_probe(struct regs *rp, caddr_t addr, processorid_t cpuid)
> -{
> -	krwlock_t *rwp;
> -	proc_t *p = curproc;
> -	extern void trap(struct regs *, caddr_t, processorid_t);
> -
> -	if (USERMODE(rp->r_cs) || (rp->r_ps & PS_VM)) {
> -		if (curthread->t_cred != p->p_cred) {
> -			cred_t *oldcred = curthread->t_cred;
> -			/*
> -			 * DTrace accesses t_cred in probe context.  t_cred
> -			 * must always be either NULL, or point to a valid,
> -			 * allocated cred structure.
> -			 */
> -			curthread->t_cred = crgetcred();
> -			crfree(oldcred);
> -		}
> -	}
> -
> -	if (rp->r_trapno == T_DTRACE_RET) {
> -		uint8_t step = curthread->t_dtrace_step;
> -		uint8_t ret = curthread->t_dtrace_ret;
> -		uintptr_t npc = curthread->t_dtrace_npc;
> -
> -		if (curthread->t_dtrace_ast) {
> -			aston(curthread);
> -			curthread->t_sig_check = 1;
> -		}
> -
> -		/*
> -		 * Clear all user tracing flags.
> -		 */
> -		curthread->t_dtrace_ft = 0;
> -
> -		/*
> -		 * If we weren't expecting to take a return probe trap, kill
> -		 * the process as though it had just executed an unassigned
> -		 * trap instruction.
> -		 */
> -		if (step == 0) {
> -			tsignal(curthread, SIGILL);
> -			return;
> -		}
> -
> -		/*
> -		 * If we hit this trap unrelated to a return probe, we're
> -		 * just here to reset the AST flag since we deferred a signal
> -		 * until after we logically single-stepped the instruction we
> -		 * copied out.
> -		 */
> -		if (ret == 0) {
> -			rp->r_pc = npc;
> -			return;
> -		}
> -
> -		/*
> -		 * We need to wait until after we've called the
> -		 * dtrace_return_probe_ptr function pointer to set %pc.
> -		 */
> -		rwp = &CPU->cpu_ft_lock;
> -		rw_enter(rwp, RW_READER);
> -		if (dtrace_return_probe_ptr != NULL)
> -			(void) (*dtrace_return_probe_ptr)(rp);
> -		rw_exit(rwp);
> -		rp->r_pc = npc;
> -
> -	} else if (rp->r_trapno == T_BPTFLT) {
> -		uint8_t instr;
> -		rwp = &CPU->cpu_ft_lock;
> -
> -		/*
> -		 * The DTrace fasttrap provider uses the breakpoint trap
> -		 * (int 3). We let DTrace take the first crack at handling
> -		 * this trap; if it's not a probe that DTrace knowns about,
> -		 * we call into the trap() routine to handle it like a
> -		 * breakpoint placed by a conventional debugger.
> -		 */
> -		rw_enter(rwp, RW_READER);
> -		if (dtrace_pid_probe_ptr != NULL &&
> -		    (*dtrace_pid_probe_ptr)(rp) == 0) {
> -			rw_exit(rwp);
> -			return;
> -		}
> -		rw_exit(rwp);
> -
> -		/*
> -		 * If the instruction that caused the breakpoint trap doesn't
> -		 * look like an int 3 anymore, it may be that this tracepoint
> -		 * was removed just after the user thread executed it. In
> -		 * that case, return to user land to retry the instuction.
> -		 */
> -		if (fuword8((void *)(rp->r_pc - 1), &instr) == 0 &&
> -		    instr != FASTTRAP_INSTR) {
> -			rp->r_pc--;
> -			return;
> -		}
> -
> -		trap(rp, addr, cpuid);
> -
> -	} else {
> -		trap(rp, addr, cpuid);
> -	}
> -}
> -
>   void
>   dtrace_safe_synchronous_signal(void)
>   {
>
> Modified: head/sys/cddl/dev/dtrace/i386/dtrace_subr.c
> ==============================================================================
> --- head/sys/cddl/dev/dtrace/i386/dtrace_subr.c	Sun May 10 22:21:00 2015	(r282743)
> +++ head/sys/cddl/dev/dtrace/i386/dtrace_subr.c	Sun May 10 22:27:48 2015	(r282744)
> @@ -143,122 +143,6 @@ dtrace_sync(void)
>   }
>   
>   #ifdef notyet
> -int (*dtrace_fasttrap_probe_ptr)(struct regs *);
> -int (*dtrace_pid_probe_ptr)(struct regs *);
> -int (*dtrace_return_probe_ptr)(struct regs *);
> -
> -void
> -dtrace_user_probe(struct regs *rp, caddr_t addr, processorid_t cpuid)
> -{
> -	krwlock_t *rwp;
> -	proc_t *p = curproc;
> -	extern void trap(struct regs *, caddr_t, processorid_t);
> -
> -	if (USERMODE(rp->r_cs) || (rp->r_ps & PS_VM)) {
> -		if (curthread->t_cred != p->p_cred) {
> -			cred_t *oldcred = curthread->t_cred;
> -			/*
> -			 * DTrace accesses t_cred in probe context.  t_cred
> -			 * must always be either NULL, or point to a valid,
> -			 * allocated cred structure.
> -			 */
> -			curthread->t_cred = crgetcred();
> -			crfree(oldcred);
> -		}
> -	}
> -
> -	if (rp->r_trapno == T_DTRACE_RET) {
> -		uint8_t step = curthread->t_dtrace_step;
> -		uint8_t ret = curthread->t_dtrace_ret;
> -		uintptr_t npc = curthread->t_dtrace_npc;
> -
> -		if (curthread->t_dtrace_ast) {
> -			aston(curthread);
> -			curthread->t_sig_check = 1;
> -		}
> -
> -		/*
> -		 * Clear all user tracing flags.
> -		 */
> -		curthread->t_dtrace_ft = 0;
> -
> -		/*
> -		 * If we weren't expecting to take a return probe trap, kill
> -		 * the process as though it had just executed an unassigned
> -		 * trap instruction.
> -		 */
> -		if (step == 0) {
> -			tsignal(curthread, SIGILL);
> -			return;
> -		}
> -
> -		/*
> -		 * If we hit this trap unrelated to a return probe, we're
> -		 * just here to reset the AST flag since we deferred a signal
> -		 * until after we logically single-stepped the instruction we
> -		 * copied out.
> -		 */
> -		if (ret == 0) {
> -			rp->r_pc = npc;
> -			return;
> -		}
> -
> -		/*
> -		 * We need to wait until after we've called the
> -		 * dtrace_return_probe_ptr function pointer to set %pc.
> -		 */
> -		rwp = &CPU->cpu_ft_lock;
> -		rw_enter(rwp, RW_READER);
> -		if (dtrace_return_probe_ptr != NULL)
> -			(void) (*dtrace_return_probe_ptr)(rp);
> -		rw_exit(rwp);
> -		rp->r_pc = npc;
> -
> -	} else if (rp->r_trapno == T_DTRACE_PROBE) {
> -		rwp = &CPU->cpu_ft_lock;
> -		rw_enter(rwp, RW_READER);
> -		if (dtrace_fasttrap_probe_ptr != NULL)
> -			(void) (*dtrace_fasttrap_probe_ptr)(rp);
> -		rw_exit(rwp);
> -
> -	} else if (rp->r_trapno == T_BPTFLT) {
> -		uint8_t instr;
> -		rwp = &CPU->cpu_ft_lock;
> -
> -		/*
> -		 * The DTrace fasttrap provider uses the breakpoint trap
> -		 * (int 3). We let DTrace take the first crack at handling
> -		 * this trap; if it's not a probe that DTrace knowns about,
> -		 * we call into the trap() routine to handle it like a
> -		 * breakpoint placed by a conventional debugger.
> -		 */
> -		rw_enter(rwp, RW_READER);
> -		if (dtrace_pid_probe_ptr != NULL &&
> -		    (*dtrace_pid_probe_ptr)(rp) == 0) {
> -			rw_exit(rwp);
> -			return;
> -		}
> -		rw_exit(rwp);
> -
> -		/*
> -		 * If the instruction that caused the breakpoint trap doesn't
> -		 * look like an int 3 anymore, it may be that this tracepoint
> -		 * was removed just after the user thread executed it. In
> -		 * that case, return to user land to retry the instuction.
> -		 */
> -		if (fuword8((void *)(rp->r_pc - 1), &instr) == 0 &&
> -		    instr != FASTTRAP_INSTR) {
> -			rp->r_pc--;
> -			return;
> -		}
> -
> -		trap(rp, addr, cpuid);
> -
> -	} else {
> -		trap(rp, addr, cpuid);
> -	}
> -}
> -
>   void
>   dtrace_safe_synchronous_signal(void)
>   {
>
>




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?55500C0D.7090802>