704400 - Implement --enable-dmd

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 676724 is about DMD, and it contains two patches:  one for DMD itself (a patch for Valgrind) and one for Firefox which integrates it with DMD.  The purpose of this bug is to land the latter changes into the tree, which will make DMD easier to use and less likely to bit-rot.  The changes will be hidden behind an --enable-dmd configure option and a corresponding MOZ_DMD constant.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch

Kyle, asking you for normal technical review.

----

Gerv, I have to ask you about licensing.  This patch is a bit of a strange 
case.  DMD is a tool build with Valgrind.  Valgrind and Valgrind tools are
GPLv2, with one exception:  they usually include a header file that is
designed to be #included into other programs, and that file is deliberately
licenses under a 4-clause BSD license so that can be #included pretty much
anywhere.  In DMD's case, this header is called dmd.h.

Now, DMD is a Valgrind tool, but it's not going to become part of the 
Valgrind distribution because it's Firefox-specific.  So we have to put 
dmd.h in the Mozilla tree (which this patch does).  What license should I 
use for it?  I could use the existing 4-clause BSD, assuming it's
compatible with the MPL.  Or I could make it MPL;  that's odd for a Valgrind 
tool but I don't see why it's a problem.  Either way, I should change the
existing "Copyright Nicholas Nethercote" line so it's copyright Mozilla
Foundation instead.  And do I need to do anything else, like add it to our
licensing list?  Maybe not, since I wrote DMD for Mozilla and it's only
available as a patch against Valgrind in bug 676724.

Sorry it's so confusing, please ask if anything is unclear.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: patch v2

Right version of the patch this time.

Comment 3

[Gervase Markham [:gerv]]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> Gerv, I have to ask you about licensing.  This patch is a bit of a strange 
> case.

Hi Nicholas,

Thanks for asking. I think the right thing to do is for dmd.h to be under the same licence that similar files are for other tools. You don't need to add it to about:license because a ) we aren't shipping dmd.h as part of a Firefox binary, and b) this license doesn't actually have the clause requiring that.

Yes, you should change the copyright, as with all code written by MoFo/MoCo employees and contractors.

One note, though: although logically accurate, calling this a "4-clause BSD licence" is misleading, because normally when people use that phrase they are referring to the one with the non-free advertising clause.

Gerv

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: patch v3

Some minor tweaks, including putting MoFo as the copyright holder for dmd.h.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 577460 [details] [diff] [review]
patch v3

Review of attachment 577460 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +2121,5 @@
>  dnl ========================================================
> +dnl = Use DMD
> +dnl ========================================================
> +MOZ_ARG_ENABLE_BOOL(dmd,
> +[  --enable-dmd            Enable DMD;  --disable-jemalloc must also be specified (default=no)],

We should check for and error out in the --enable-dmd --enable-jemalloc case.

@@ +2128,5 @@
> +if test -n "$MOZ_DMD"; then
> +    MOZ_CHECK_HEADER([valgrind/valgrind.h], [],
> +        AC_MSG_ERROR(
> +            [--enable-dmd specified but Valgrind is not installed]))
> +    AC_DEFINE(MOZ_DMD)

Is it possible to --enable-dmd without --enable-valgrind?  If not, we should ensure that --enable-valgrind was passed too.

@@ +2129,5 @@
> +    MOZ_CHECK_HEADER([valgrind/valgrind.h], [],
> +        AC_MSG_ERROR(
> +            [--enable-dmd specified but Valgrind is not installed]))
> +    AC_DEFINE(MOZ_DMD)
> +    MOZ_DMD=1

No need to set MOZ_DMD=1 again.

::: memory/mozalloc/Makefile.in
@@ +58,5 @@
>  else
>  FORCE_SHARED_LIB= 1
>  endif
>  
> +# TODO: we do this in crashreporter and storage/src too, should be centralized

Indeed :-/

::: memory/mozalloc/mozalloc.cpp
@@ +258,5 @@
>          return 0;
>  
>  #if defined(XP_MACOSX)
>      return malloc_size(ptr);
> +#elif defined(MOZ_MEMORY) || defined(XP_LINUX)  // njn: GNU/Linux only?

It's available on at least some of the BSDs too, but I don't think we need to worry about that.

::: storage/src/mozStorageService.cpp
@@ +312,5 @@
>  {
>    NS_IF_RELEASE(sXPConnect);
>  }
>  
> +sqlite3_vfs *ConstructTelemetryVFS();

There's no need for any of the changes in this file, is there?

::: xpcom/base/dmd.h
@@ +1,1 @@
> +/*

I didn't actually review this file :-)

Comment 6

[Nicholas Nethercote [inactive]]

In the final patch I made it so that you just have to specify "--enable-dmd" and the valgrind/jemalloc stuff works itself out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8196c95d4c9

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a8196c95d4c9

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)
> ::: storage/src/mozStorageService.cpp
> @@ +312,5 @@
> >  {
> >    NS_IF_RELEASE(sXPConnect);
> >  }
> >  
> > +sqlite3_vfs *ConstructTelemetryVFS();
> 
> There's no need for any of the changes in this file, is there?

Indeed, these changes are wrong. Why did you ignore this comment and the style guide?

Comment 9

[Nicholas Nethercote [inactive]]

> Indeed, these changes are wrong. Why did you ignore this comment and the
> style guide?

Two reasons.  First, this is preferred style for the storage module:  http://hg.mozilla.org/mozilla-central/annotate/ac667309bea6/storage/style.txt#l45
Second, I consulted with khuey on IRC and he wasn't fussed either way.