Closed Bug 682717 Opened 10 years ago Closed 10 years ago

Assignment to a DebugOnly variable is automatically elided in non-debug builds.


(Core :: JavaScript Engine, defect)

Not set





(Reporter: terrence, Assigned: terrence)



(1 file, 1 obsolete file)

On jaegermonkey tip in jsanalyze.cpp ScryptAnalysis::analyzeLifetimes on line 908, assignment to the DebugOnly<bool> variable found is protected by #ifdef DEBUG.

Assignment to a DebugOnly variable results in a call to either the empty 'operator =' or to an empty implicit 1-arg constructor and empty copy constructor when in non-debug mode.  Either of these get inlined and eliminated as dead code above -00.  No code is produced for the statement, so there is no benefit to cluttering the code with this condition.
Attachment #556399 - Flags: review?(bhackett1024)
That may be the case for the specific example you looked at, but can you promise me that will happen on both GCC and MSVC for all possible real uses for modern and future versions?  Answering those with an unequivocal "yes" costs a few #ifdefs in reusable code that gets read maybe once per developer.  I'd rather keep things obvious.
Attachment #556399 - Flags: review?(bhackett1024)
Ah, I thought that was half the point of DebugOnly.  This makes me sad.

In this case, I have to admit that the version with more #ifdef's is indeed the clearer one.  Sorry for the noise.
Closed: 10 years ago
Resolution: --- → INVALID
Attachment #556399 - Flags: review+
Oops, I totally misread comment 0 and failed to look at the patch.  Sorry for the noise.
Resolution: INVALID → ---
Keywords: checkin-needed
In my queue :-)

I've changed the commit message slightly, since there was no bug number and no r= for the reviewer. Also, the pushlog + TBPL only display the first line/80 characters, so a summary in the first line makes it a bit clearer. eg:
"Bug 682717 - Remove unnecessary #ifdef DEBUG around assignment to DebugOnly; r=bhackett"

I've left the more detailed commit message after that - since it does a great job of making the changeset clearer later on, without having to return to the bug - and is something that people don't do often enough, so thanks! :-)
Assignee: general → terrence
Flags: in-testsuite-
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
Version: Other Branch → Trunk
This fails my MSVC2010 local build:


c:\mozilla\repos\inbound\js\src\methodjit/MethodJIT.h(690) : warning C4305: 'argument' : truncation from 'js::MaybeConstruct' to 'bool'

c:\mozilla\repos\obj-inbound\dist\include\mozilla\util.h(177) : error C4716: 'mozilla::DebugOnly<bool>::operator=' : must return a value

In the directory  /c/mozilla/repos/obj-inbound/js/src
The following command failed to execute properly:
c:/mozilla-build/python/python2.7.exe -O c:/mozilla/repos/inbound/js/src/build/ cl -Fojsanalyze.obj -c -DOSTYPE="WINNT6.1" -DOSARCH=WINNT -DEXPORT_JS_API -DIMPL_MFBT -DJS_HAS_CTYPES -DDLL_PREFIX="" -DDLL_SUFFIX=".dll" -Ictypes/libffi/include -I. -I../../../inbound/js/src -I. -I./../../dist/include -I./../../dist/include/nsprpub -Ic:/mozilla/repos/obj-inbound/dist/include/nspr -I../../../inbound/js/src -I../../../inbound/js/src/assembler -I../../../inbound/js/src/yarr -GR- -w14505 -TP -nologo -wd4345 -D_CRT_SECURE_NO_WARNINGS -W3 -Gy -Fdgenerated.pdb -wd4244 -wd4800 -we4553 -DNDEBUG -DTRIMMED -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1 -DENABLE_JIT=1 -MD -FI ./js-confdefs.h -DMOZILLA_CLIENT c:/mozilla/repos/obj-inbound/js/src/../../../inbound/js/src/jsanalyze.cpp
Huh.  I assumed that because this operator= was already present in the tree I would not be the first person using it.  I will look into making MSVC happy.
Tested in linux and windows to ensure that this does correctly inline, with the new operator= body.  Tested in linux with make check on js/src on an optimized build and a full compile of mozilla-central.
Attachment #556399 - Attachment is obsolete: true
Keywords: checkin-needed
Has this been reviewed/looked at again since the changes?
Keywords: checkin-needed
Target Milestone: --- → mozilla9
Meant to add that comment 8 didn't seem necessary looking at the changes + it built locally and on try fine, so pushed to inbound.
There was no second review -- Luke and I discussed the necessary change in IRC.  As you noticed, it should be small enough to fly under the radar.  Sorry I missed such an obvious problem on the first pass.  Once I get access to a try server, I think your workload will drop dramatically :-).
Target Milestone: mozilla9 → ---
Yeah after comment 8 I remembered you discussing it on #jsapi, so thought you had probably got as rs+ over IRC anyway.

I've just spotted that the bug number in the commit message was actually for one of the other bugs you are working on. Could you post a reply in the other bug pointing them this way please (both to help the person merging and for someone looking at hg annotate later). Thanks :-)
Target Milestone: --- → mozilla9
Gah!!!  Posted a notice on 646597.  My error rate is way too high.  I'm making a checklist.
Thanks :-)
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.