1138740 - Differential Testing: Different output message involving array buffers

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

x = Int8Array(1)
function f(y) {
    x[0] = y
}
f()
f(3)
f(7)
try {
    x.buffer(f())
} catch (e) {}
print(x[0])

$ ./js-dbg-64-dm-nsprBuild-darwin-0b3c520002ad --fuzzing-safe --no-threads --ion-eager testcase.js
7

$ ./js-dbg-64-dm-nsprBuild-darwin-0b3c520002ad --fuzzing-safe --no-threads --baseline-eager testcase.js
0

Tested this on m-c rev 0b3c520002ad.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh ~/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build -R ~/trees/mozilla-central" -r 0b3c520002ad

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/d5b0e9e6a849
user:        Brian Hackett
date:        Mon Apr 07 13:04:37 2014 -0700
summary:     Bug 987508 - Create array buffers lazily for small typed arrays, r=sfink.

Setting s-s because typed arrays are involved.

Brian, is bug 987508 a likely regressor?

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

Ion needs to be notified when the base pointer of a possibly-singleton-typed buffer view changes.

Comment 2

[Brian Hackett [Laid off!]]

The only problem here is that Ion might continue using the original inline data for the typed array's contents.  That memory will always be there, so this will only lead to correctness problems.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8573363 [details] [diff] [review]
patch

Review of attachment 8573363 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good. It seems like there ought to be a simpler test case for this, but I was unable to come up with one. I couldn't get the compiler to emit the hardcoded data pointer. It created an MConstantElements MIR node with the pointer value, and lowered that to an LDefinition::SLOTS, but it seems like it never visited that node. I don't know anything about the compiler, so I don't know how to track it further. At any rate, it seemed like the generated code was loading it out of the private pointer.

The original test case does not reproduce for me.

Oh well. I'll remove my nose from the jit now. May as well just land as is.

Comment 4

[Brian Hackett [Laid off!]]

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

Comment 5

[Phil Ringnalda (:philor)]

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

Comment 6

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

Can we please backport this to 38? It is the next ESR and this will help fuzzing on that ESR branch if need be.

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 8573363 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: Bug 987508
[User impact if declined]: Possible incorrect behavior.  See comment 6.
[Describe test coverage new/current, TreeHerder]: Fuzz test.
[Risks and why]: Minimal.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8573363 [details] [diff] [review]
patch

[Triage Comment]
Sure, should be in 38 beta 8
39 is already fixed

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/e1fb2a5ab48d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e1fb2a5ab48d