Closed Bug 682717 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: terrence, Assigned: terrence)

Details

Attachments

(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.
Status: NEW → RESOLVED
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.
Status: RESOLVED → REOPENED
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
Status: REOPENED → ASSIGNED
Flags: in-testsuite-
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
Version: Other Branch → Trunk
This fails my MSVC2010 local build:

{
jsanalyze.cpp

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.py 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 :-)

http://hg.mozilla.org/mozilla-central/rev/a0a6800d3744
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.