Don't print $DMD = '1' when enabling DMD

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glandium, Assigned: njn)

Tracking

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Spawned from bug 1097507 comment 11 as per IRC:

@@ +1329,3 @@
>  
> +  if (!e) {
> +    e = "1";

This causes a "$DMD = '1'" message to be printed out immediately afterwards, which is misleading. Can you instead leave |e| as nullptr, and then do this:

> if (e) {
>   StatusMsg("$DMD = '%s'\n", e);
> } else {
>   StatusMsg("$DMD is undefined\n", e);
> }

and then make Options::Options() handle the aDMDEnvVar==nullptr case? Thanks.
I'm not sure I understand the background of this, but previously DMD wouldn't be enabled if $DMD isn't set. Is that no longer the case? If so perhaps a more explicit message would be useful:

  - Default options: "DMD is enabled"
  - Options via $DMD: "DMD is enabled: ${DMD}"
With the new setup just specifying the DMD library via LD_PRELOAD/MOZ_REPLACE_MALLOC_LIB/whatever is enough to enable it.
Attachment #8530068 - Flags: review?(mh+mozilla)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment on attachment 8530068 [details] [diff] [review]
Don't lie when printing $DMD's value at start-up

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

I'm surprised you don't change the test files. The reason I didn't do it originally was that the tests were looking for $DMD = '1' in the output.
Attachment #8530068 - Flags: review?(mh+mozilla) → review+
You asked for it! :)  Here's a new patch that does more stuff to remove almost
all traces of $DMD=1.
Attachment #8534140 - Flags: review?(mh+mozilla)
Attachment #8530068 - Attachment is obsolete: true
Comment on attachment 8534140 [details] [diff] [review]
Tweak DMD to account for the fact that $DMD can now be undefined

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

::: memory/replace/dmd/DMD.h
@@ +150,5 @@
>  //   // - 1: The original format. Implemented in bug 1044709.
> +//   // - 2: Added the "mode" property under "invocation". Added in bug 1094552.
> +//   // - 3: The "dmdEnvVar" property under "invocation" can now be |null| if
> +//   //      the |DMD| environment variable is not defined. Done in bug 1100851.
> +//   "version": 3,

Is that actually a backwards incompatible change that requires bumping the version number?

::: memory/replace/dmd/test/script-diff-dark-matter1.json
@@ +3,2 @@
>   "invocation": {
> +  "dmdEnvVar": "--mode=dark-matter --sample-below=127",

Adding an argument would seem to be unrelated.

::: memory/replace/dmd/test/script-diff-live-expected.txt
@@ +1,5 @@
>  #-----------------------------------------------------------------
>  # dmd.py --filter-stacks-for-testing -o script-diff-live-actual.txt script-diff-live1.json script-diff-live2.json
>  
>  Invocation 1 {
> +  $DMD = '--mode=live --sample-below=127'

Likewise
Attachment #8534140 - Flags: review?(mh+mozilla) → review+
This is useful.
Attachment #8534142 - Flags: review?(continuation)
> Is that actually a backwards incompatible change that requires bumping the
> version number?

No, but it gives us an opportunity to document that it happened, and DMD version changing is not a big deal, because the output files aren't intended to have a long life.

> Adding an argument would seem to be unrelated.

Yeah. I was changing some of the $DMD=1 values so I thought I'd fix some inconsistencies and it didn't seem worth separating these into a separate patch.
Attachment #8534142 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/edd717f27f37
https://hg.mozilla.org/mozilla-central/rev/bd3c5d90a40c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.