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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: Remove unnecessary #ifdef DEBUG around assignment to DebugOnly.

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.

Comment 1

[Luke Wagner [:luke]]

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.

Comment 2

[Terrence Cole [:terrence]]

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.

Comment 3

[Luke Wagner [:luke]]

Oops, I totally misread comment 0 and failed to look at the patch.  Sorry for the noise.

Comment 4

[Ed Morley [:emorley]]

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! :-)

Comment 5

[Ed Morley [:emorley]]

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
}

Comment 6

[Terrence Cole [:terrence]]

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.

Comment 7

[Terrence Cole [:terrence]]

Attachment: Remove unneeded #ifdef DEBUG: use DebugOnly instead.

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.

Comment 8

[Ed Morley [:emorley]]

Has this been reviewed/looked at again since the changes?

Comment 9

[Ed Morley [:emorley]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a0a6800d3744

Comment 10

[Ed Morley [:emorley]]

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.

Comment 11

[Terrence Cole [:terrence]]

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

Comment 12

[Ed Morley [:emorley]]

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

Comment 13

[Terrence Cole [:terrence]]

Gah!!!  Posted a notice on 646597.  My error rate is way too high.  I'm making a checklist.

Comment 14

[Ed Morley [:emorley]]

Thanks :-)

http://hg.mozilla.org/mozilla-central/rev/a0a6800d3744