svn commit: r282744 - in head/sys/cddl/dev/dtrace: amd64 i386

[Mark Johnston]

On Mon, May 11, 2015 at 09:55:25AM +0800, Julian Elischer wrote:
> 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.

I don't agree with that assertion either. The truly upstream DTrace
code, i.e. the DTrace code that is updated by merges from the vendor
branch, lives under sys/cddl/contrib/opensolaris. When DTrace was
imported, some parts of it had to be modified significantly enough that
they were placed outside the vendor import dir, in sys/cddl/dev/.
In particular, this code isn't modified by MFVs and is maintained solely
within FreeBSD.

I try quite hard to keep the upstream code around for changes under the
former directory, but I don't see the value in keeping upstream code
under the latter. In many cases it is substantially different from what
we actually use and just ends up being confusing. It also becomes
inconsistent as different arches gain DTrace support and add varying
amounts of upstream code to their MD DTrace files. See r281916 for an
example where the (dead) upstream code was kept for years and copied
around before it was realized that it wasn't needed.

This split isn't ideal, but upstream DTrace development has been inactive
enough that it doesn't end up being too painful an issue, IMHO.

> 
> 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 caught several cases (specifically in merging DTrace code) where the
upstream code block is so large the merger didn't even notice the
#ifndef __FreeBSD__ was present and thought that the merge was fine because
the upstream diff applied cleanly.

> 
> 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.