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)

x86_64
macOS
defect
Not set
critical

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)
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)
Flags: needinfo?(jseward)
I also cannot repro on OSX 10.9.5.
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).
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)
Yeah, that's an easy fix. Thanks for investigating the cause.
Attachment #8504693 - Flags: review?(luke)
Assignee: nobody → benj
Status: NEW → ASSIGNED
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.
Indeed, plus it's better with a test.
Attachment #8504703 - Flags: review?(luke)
Attachment #8504693 - Attachment is obsolete: true
Attachment #8504693 - Flags: review?(luke)
Sorry for the mess here, i just updated the comment to be sure after irc quick
chat with sewardj.
Attachment #8504719 - Flags: review?(luke)
Attachment #8504703 - Attachment is obsolete: true
Attachment #8504703 - Flags: review?(luke)
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+
(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
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.