1077031 - OdinMonkey: Valgrind detects Conditional jump or move depends on uninitialised value(s) involving IsFloatLiteral

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 1

[Luke Wagner [: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.

Comment 2

[Luke Wagner [:luke]]

I also cannot repro on OSX 10.9.5.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

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

[Julian Seward [:jseward]]

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?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Move dependent condition out of an if statement to work around a clang codegen bug

Yeah, that's an easy fix. Thanks for investigating the cause.

Comment 6

[Julian Seward [:jseward]]

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.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Move dependent condition out of an if statement to work around a clang codegen bug

Indeed, plus it's better with a test.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Move dependent condition out of an if statement to work around a clang codegen bug

Sorry for the mess here, i just updated the comment to be sure after irc quick
chat with sewardj.

Comment 9

[Luke Wagner [: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)

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db0912d0106d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/db0912d0106d