Implement --enable-dmd

RESOLVED FIXED in mozilla11

Status

()

Core
General
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla11
All
Linux
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 676724
Depends on: 698968
No longer depends on: 676724
(Assignee)

Updated

6 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 1

6 years ago
Created attachment 577179 [details] [diff] [review]
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.
Attachment #577179 - Flags: review?(khuey)
Attachment #577179 - Flags: feedback?(gerv)
(Assignee)

Comment 2

6 years ago
Created attachment 577180 [details] [diff] [review]
patch v2

Right version of the patch this time.
Attachment #577179 - Attachment is obsolete: true
Attachment #577179 - Flags: review?(khuey)
Attachment #577179 - Flags: feedback?(gerv)
Attachment #577180 - Flags: review?(khuey)
Attachment #577180 - Flags: feedback?(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
(Assignee)

Comment 4

6 years ago
Created attachment 577460 [details] [diff] [review]
patch v3

Some minor tweaks, including putting MoFo as the copyright holder for dmd.h.
Attachment #577180 - Attachment is obsolete: true
Attachment #577180 - Flags: review?(khuey)
Attachment #577180 - Flags: feedback?(gerv)
Attachment #577460 - Flags: review?(khuey)
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 :-)
Attachment #577460 - Flags: review?(khuey) → review+
(Assignee)

Comment 6

6 years ago
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

6 years ago
https://hg.mozilla.org/mozilla-central/rev/a8196c95d4c9
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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?
(Assignee)

Comment 9

6 years ago
> 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.
Keywords: dev-doc-needed

Updated

4 years ago
Blocks: 858929
You need to log in before you can comment on or make changes to this bug.