Closed
Bug 1077031
Opened 10 years ago
Closed 10 years ago
OdinMonkey: Valgrind detects Conditional jump or move depends on uninitialised value(s) involving IsFloatLiteral
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: gkw, Assigned: bbouvier)
Details
(Keywords: testcase, valgrind)
Attachments
(1 file, 2 obsolete files)
(function() { "use asm"; var f = f; })() valgrind --smc-check=all-non-file --vex-iropt-register-updates=allregs-at-mem-access --track-origins=yes ./js-dbgDisabled-opt-64-dm-vg-nsprBuild-darwin-6a63bcb6e0d3 --no-threads --ion-eager testcase.js ==954== Conditional jump or move depends on uninitialised value(s) ==954== at 0x1000832FC: IsFloatLiteral((anonymous namespace)::ModuleCompiler&, js::frontend::ParseNode*) (AsmJSValidate.cpp:1956) ==954== by 0x101EE5FFF: ??? ==954== by 0x1000832B2: IsNumericNonFloatLiteral(js::frontend::ParseNode*) (AsmJSValidate.cpp:1902) ==954== by 0x101EE5FFF: ??? ==954== by 0x100082B33: IsNumericLiteral((anonymous namespace)::ModuleCompiler&, js::frontend::ParseNode*) (AsmJSValidate.cpp:2030) ==954== by 0x101EE5FC7: ??? ==954== by 0x7FFF5FBFC68F: ??? ==954== Uninitialised value was created by a stack allocation ==954== at 0x1000832E0: IsFloatLiteral((anonymous namespace)::ModuleCompiler&, js::frontend::ParseNode*) (AsmJSValidate.cpp:1953) My configure flags are: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/fuzz3/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --disable-debug --enable-optimize=-O1 --enable-nspr-build --enable-more-deterministic --enable-valgrind --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests Diagnosis from Julian: ====================== js/src/asmjs/AsmJSValidate.cpp:1952 static bool IsFloatLiteral(ModuleCompiler &m, ParseNode *pn) { ParseNode *coercedExpr; AsmJSCoercion coercion; if (!IsCoercionCall(m, pn, &coercion, &coercedExpr) || coercion != AsmJS_FRound) return false; return IsNumericNonFloatLiteral(coercedExpr); } Initially "AsmJSCoercion coercion;" means |coercion| is uninitialised. IsCoercionCall(m, pn, &coercion, &coercedExpr) returns |true| but it doesn't write anything to &coercion. Hence the second part of the ||, coercion != AsmJS_FRound is done on the uninitialised, stack-allocated value. Hence (IMO, #include <disclaimer.h>, etc) the bug is that IsCoercionCall returns |true| but doesn't write anything to its 3rd parameter. ====================== Luke, any idea how to move this forward? (Locking s-s in case this involves anything dangerous)
Flags: needinfo?(luke)
Comment 1•10 years ago
|
||
I can't reproduce any valgrind report locally (on 64-bit linux with all the configure and valgrind flags specified). The diagnosis is helpful, but it's pretty easy to see that, reading IsCoercionCall, all places where we return true have just set *coercion to a constant. Anyhow, in this tiny testcase, IsCoercionCall returns 'false' immediately. I'd try on OSX, but currently have to wait some hours for Xcode to install after Mavericks upgrade. Julian, is there a chance that the undefined warning is coming from somewhere else? The code pointed to seems pretty benign.
Flags: needinfo?(luke)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(jseward)
Comment 2•10 years ago
|
||
I also cannot repro on OSX 10.9.5.
Assignee | ||
Comment 3•10 years ago
|
||
Confirming what luke says here: all paths that set the |coercion| out-parameter are paths that return true. Curiously, this is also something that GCC sometimes cannot determine: in opt builds, a lot of the JS warnings about "maybe uninitialized variables" are variables that are given as out-params in functions wrapped in JS_ALWAYS_TRUE (which reduces to |MOZ_ASSERT(...)| in debug builds, and to nothing more than the args in opt builds).
Comment 4•10 years ago
|
||
This is a false error, caused by Memcheck being confused by a Clang optimisation. In this case, Clang evaluates !IsCoercionCall(m, pn, &coercion, &coercedExpr) || coercion != AsmJS_FRound right-to-left, even when |coercion| is uninitialised. As Nick points out, this is -- in this case -- harmless, and is documented in http://llvm.org/bugs/show_bug.cgi?id=12319 I am not sure what can be done in Memcheck to fix this. I am not aware of any easy fix, since this interacts somewhat complicatedly with Memcheck's rules on undefinedness propagation vs reporting. Although not the ideal solution, a small modification in the source would make Memcheck happy: force Clang to evaluate the condition left-to-right, by splitting it: - if (!IsCoercionCall(m, pn, &coercion, &coercedExpr) || coercion != AsmJS_FRound) + if (!IsCoercionCall(m, pn, &coercion, &coercedExpr)) + return false; + if (coercion != AsmJS_FRound) return false; I was going to say that a possibly more elegant change would be like this .. // These EcmaScript-defined coercions form the basis of the asm.js type system. enum AsmJSCoercion { AsmJS_ToInt32, AsmJS_ToNumber, AsmJS_FRound, AsmJS_ToInt32x4, - AsmJS_ToFloat32x4 + AsmJS_ToFloat32x4, + AsmJS_INVALID }; ... - AsmJSCoercion coercion; + AsmJSCoercion coercion = AsmJS_INVALID; // Keep Valgrind happy. See bug 1077031. .. but that turns out to produce a whole bunch of compiler complaining about the new value AsmJS_INVALID not being handled in various switches. So it would be considerably more verbose. Would either such change be considered acceptable?
Flags: needinfo?(jseward)
Assignee | ||
Comment 5•10 years ago
|
||
Yeah, that's an easy fix. Thanks for investigating the cause.
Attachment #8504693 -
Flags: review?(luke)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → benj
Status: NEW → ASSIGNED
Comment 6•10 years ago
|
||
Maybe worth adding a very brief comment saying why this is now written without an || ? So as to stop people from transforming it back to the || version in the future.
Assignee | ||
Comment 7•10 years ago
|
||
Indeed, plus it's better with a test.
Attachment #8504703 -
Flags: review?(luke)
Assignee | ||
Updated•10 years ago
|
Attachment #8504693 -
Attachment is obsolete: true
Attachment #8504693 -
Flags: review?(luke)
Assignee | ||
Comment 8•10 years ago
|
||
Sorry for the mess here, i just updated the comment to be sure after irc quick chat with sewardj.
Attachment #8504719 -
Flags: review?(luke)
Assignee | ||
Updated•10 years ago
|
Attachment #8504703 -
Attachment is obsolete: true
Attachment #8504703 -
Flags: review?(luke)
Comment 9•10 years ago
|
||
Comment on attachment 8504719 [details] [diff] [review] Move dependent condition out of an if statement to work around a clang codegen bug Review of attachment 8504719 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for digging into this Julian, and thanks for fixing Benjamin. ::: js/src/asmjs/AsmJSValidate.cpp @@ +2013,5 @@ > + return false; > + // Note that while this test could be combined with the one above, clang > + // optimizes it such that valgrind's memcheck thinks there is a test > + // depending on an uninitialized value. See also bug 1077031. > + if (coercion != AsmJS_FRound) A comment is good, but I could do with a much shorter one: // Don't fold into || to avoid clang/memcheck bug (bug 1077031)
Attachment #8504719 -
Flags: review?(luke) → review+
Reporter | ||
Comment 10•10 years ago
|
||
(In reply to Julian Seward [:jseward] from comment #4) > This is a false error, caused by Memcheck being confused by a Clang > optimisation. Opening up then.
Group: core-security
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/db0912d0106d
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/db0912d0106d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•