1030460 - Conditional jump or move depends on uninitialised value(s) and Uninitialised value was created by a stack allocation [@ js::jit::TryAttachNativeGetPropStub]

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

Attachment: stack

function exploreDeeper(a, an) {
    var hns = Object.getOwnPropertyNames(a);
    var propertyNames = [];
    for (var j = 0; j < hns.length; ++j) {
        var hn = hns[j];
    }
}
var gns = Object.getOwnPropertyNames(this);
for (var i = 0; i < gns.length; ++i) {
    var gn = gns[i]; {
        var g = this[gn];
        if (g.toString().indexOf("[native code]") != -1) {
            exploreDeeper(g.prototype, gn + ".prototype");
        }
    }
}

$ valgrind --track-origins=yes --dsymutil=yes --smc-check=all-non-file --vex-iropt-register-updates=allregs-at-mem-access --leak-check=full --errors-for-leak-kinds=definite --show-leak-kinds=definite ./js-opt-64-dm-vg-ts-darwin-da1dbcff9493 testcase.js

Tested on opt 64-bit deterministic threadsafe js shell on m-c rev da1dbcff9493, on Mac 10.9.3.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --disable-debug --enable-optimize=-O1 --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --enable-valgrind --with-ccache --enable-threadsafe <other NSPR options>

(one should compile valgrind from SVN to get Mac support)

Locking this s-s since I'm not entirely sure what this is about.

Comment 1

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/58605e9a6ea1
user:        Kannan Vijayan
date:        Tue Nov 12 14:20:34 2013 -0500
summary:     Bug 912303 - Added noSuchMethod support to baseline CALLPROP/CALLELEM stubs. r=efaust

Kannan, is bug 912303 a likely regressor?

Comment 2

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

This blocks fuzzing js shell on Mac using Valgrind.

Comment 3

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

Not sure who develops JIT on the Mac - Jan, do you know who might be a good person?

Comment 4

[Jan de Mooij [:jandem]]

Line 6261 is this:

  if (isDOMProxy && domProxyShadowsResult == Shadows) {

But this line was there before bug 912303. Bug 912303 changed some code in this function so that might have triggered some compiler optimization. AFAICS, isDOMProxy should always be initialized by EffectlesslyLookupProperty, and domProxyShadowsResult should be initialized if isDOMProxy is true.

Gary, in EffectlesslyLookupProperty in BaselineIC.cpp, if you change:

    if (checkDOMProxy)
        *checkDOMProxy = false;

to:

    if (checkDOMProxy) {
        *checkDOMProxy = false;
        *shadowsResult = ShadowCheckFailed;
    }

Does that change anything?

Comment 5

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

Attachment: latest Valgrind stack with V rev r14145

This error seems to have gone away with Valgrind rev r14145 and m-c rev 8bfe3372f848, however there are still quite a few V leaks. Julian, are they still JS issues, or are they Valgrind-related?

Comment 6

[Julian Seward [:jseward]]

I think the leaks that Valgrind reports are not of concern.  It looks
to me as if they are either genuine leaks in the MacOS system libraries,
or artefacts of Valgrind's not-very-good handling of MacOS's malloc_zone
family of functions, or some combination of the two.  I'll see if I can
reduce the noise level by fixing up V on MacOS a bit more.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Gary can you set status flags for this bug?

Comment 8

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

It seems that this issue might have gone away (as per comment 6) and may not be one concerning Gecko. I'll leave the needinfo and check this out when I'm back at the office.

Comment 9

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

So, I rechecked with current versions: Valgrind rev r14269 and m-c rev 8068e74957f2.

For some reason, I saw the errors again without the suggested patch in comment 2.
I applied the suggested patch and the errors went away.

Jan, I think you're right in comment 2 - let's have the patch landed.

Comment 10

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

> For some reason, I saw the errors again without the suggested patch in
> comment 2.

> Jan, I think you're right in comment 2 - let's have the patch landed.

Oops, I meant comment 4 in both cases.

Comment 11

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

(In reply to David Bolter [:davidb] from comment #7)
> Gary can you set status flags for this bug?

If it's indeed a JS issue, Jan is probably the better person to decide which branches are affected.

Comment 12

[Jan de Mooij [:jandem]]

I looked at the code Clang (64-bit opt build) emits for:

  if (isDOMProxy && domProxyShadowsResult == Shadows) {

It's indeed doing something clever by combining the two checks and having only a single branch. Although this can cause us to read an uninitialized domProxyShadowsResult value if isDOMProxy == false, it doesn't matter because the branch is only taken if isDOMProxy == true.

So it's not a real bug, just a compiler quirk that makes Valgrind unhappy. I'll post a patch.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Patch

(See comment 4 and comment 12.)

Comment 14

[Jan de Mooij [:jandem]]

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

Gary, I was reading more about this and apparently false positives with Valgrind + opt builds is a known issue..

http://stackoverflow.com/questions/20809080/how-does-optimization-trip-valgrind-false-positives

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Is this worth backporting anywhere? Also, should this be sec-other instead?

Comment 16

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

(In reply to Jan de Mooij [:jandem] from comment #14)
> Gary, I was reading more about this and apparently false positives with
> Valgrind + opt builds is a known issue..

Yes, that's why I try to loop Julian Seward in as fast as possible to any Valgrind issue.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #15)
> Is this worth backporting anywhere? Also, should this be sec-other instead?

Not a real bug but rather a fix to make Valgrind happy. Opening up.