475479 - TM: Valgrind - Conditional jump or move depends on uninitialised value(s) at 0x8156F31: js_CloseLoop(JSContext*) (jstracer.cpp:3489)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Nicholas Nethercote [inactive]]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.5) Gecko/2008121623 Ubuntu/8.10 (intrepid) Firefox/3.0.5
Build Identifier: 

Run TraceMonkey on trace-test.js like so:

  valgrind --smc-check=all --track-origins=yes js-debug/js -j -f a.js

(This assumes that 'js' is in an objdir called "js-debug".  It also assumes you have Valgrind 3.4.0, which is necessary for the --track-origins=yes flag.)

The output on the "testClosingRecursion" test case is this:

==23269== Conditional jump or move depends on uninitialised value(s)
==23269==    at 0x8156F31: js_CloseLoop(JSContext*) (jstracer.cpp:3489)
==23269==    by 0x8157014: TraceRecorder::monitorRecording(JSContext*, TraceRecorder*, JSOp) (jstracer.cpp:4156)
==23269==    by 0x81A8804: js_Interpret (jsinterp.cpp:2834)
==23269==    by 0x80AD0CD: js_Execute (jsinterp.cpp:1562)
==23269==    by 0x8057FF8: JS_ExecuteScript (jsapi.cpp:5095)
==23269==    by 0x8050F78: Process(JSContext*, JSObject*, char*, int) (js.cpp:429)
==23269==    by 0x8051A9D: ProcessArgs(JSContext*, JSObject*, char**, int) (js.cpp:755)
==23269==    by 0x8051E8E: main (js.cpp:4569)
==23269==  Uninitialised value was created by a stack allocation
==23269==    at 0x8156E35: js_CloseLoop(JSContext*) (jstracer.cpp:3463)

The root cause is the variable 'demote' in 'js_CloseLoop()': it isn't
initialised within the call to 'r->closeLoop()' if 'callDepth != 0' is true.

I don't know whether r->closeLoop() should always initialise 'demote' in the
'callDepth != 0' case, or if 'demote' should have an initial value.  In
either case, I also don't know what value 'demote' should be given.


Reproducible: Always

Comment 1

[Andreas Gal :gal]

Harmless, fixing.

Comment 2

[Andreas Gal :gal]

(harmless in the sense that this is a perf regression worst case, still fixing and block final)

Comment 3

[Andreas Gal :gal]

Attachment: patch

Nick, could you test this?

Comment 4

[Nicholas Nethercote [inactive]]

Looks good -- no complaints from Valgrind now.

Comment 5

[Andreas Gal :gal]

Landed on TM:

http://hg.mozilla.org/tracemonkey/rev/70138fc7d671

Comment 6

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d9e2b8d54641

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/70138fc7d671

Comment 8

[Aakash Desai [:aakashd]]

Seeing as there hasn't been any discussions about this bug for 1 1/2 months and it's been in mochitest, I'm assuming there aren't any residual issues. I'm moving this to verified as a result. If anyone has any qualms, feel free to bring them up.

Comment 9

[Bob Clary [:bc] (inactive)]

(In reply to comment #8)
> Seeing as there hasn't been any discussions about this bug for 1 1/2 months and
> it's been in mochitest, I'm assuming there aren't any residual issues. I'm
> moving this to verified as a result. If anyone has any qualms, feel free to
> bring them up.

don't verify valgrind bugs without running valgrind on the test.

Comment 10

[Jesse Ruderman]

and don't bother verifying bugs that are in-testsuite+, imo ;)

Comment 11

[Bob Clary [:bc] (inactive)]

v 1.9.1