Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 9 years ago

function f() {};
print(f());
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
function g() {
    return Math.imul(524288, 524288) == f();
};
g();
f = function() {
    return 0;
};
print(g());

$ ./js-dbg-64-dm-darwin-388bdc46ba51 --fuzzing-safe --no-threads --baseline-eager testcase.js
undefined
true

$ ./js-dbg-64-dm-darwin-388bdc46ba51 --fuzzing-safe --no-threads --ion-eager testcase.js
undefined
false

Tested this on m-c rev 388bdc46ba51.

My configure flags are:

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

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 388bdc46ba51

autoBisect is running.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 1 • 9 years ago

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/95e326d4fd7d
parent:      214107:e066f112521a
user:        Hannes Verschore
date:        Wed Nov 05 16:46:21 2014 +0100
summary:     Bug 914255 - Reduce the number of objects tracked in a TypeSet, r=bhackett

Hannes, is bug 914255 a likely regressor?

	Hannes Verschore [:h4writer]
	Comment 2 • 9 years ago

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #1)
> Hannes, is bug 914255 a likely regressor?

Not likely. Bug 914255 only reduced the amount before we use AnyObject, instead of the particular Objects in a typeset.

The following testcase reproduces it before this revision:
function f() {};
print(f());
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
f = function() {};
function g() {
    return Math.imul(524288, 524288) == f();
};
g();
f = function() {
    return 0;
};
print(g());


@gkw: can you run a (auto?) bisect on that testcase?

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 3 • 9 years ago

Hannes, I'm afraid I can't help you more for now. It seems to go back further than m-c rev 54be5416ae5d (around Oct/Nov 2014).

	Hannes Verschore [:h4writer]
	Comment 4 • 9 years ago

The first bad revision is:
changeset:   187408:9e074affd0a3
user:        Sankha Narayan Guria <sankha93@gmail.com>
date:        Fri Jun 06 17:29:45 2014 +0200
summary:     Bug 1011540 - Implement Mul Recover Instruction. r=nbp

Could this be related to the Mul recover instruction?

	Nicolas B. Pierron [:nbp] Assignee
	Comment 5 • 9 years ago

(In reply to Hannes Verschore [:h4writer] from comment #4)
> summary:     Bug 1011540 - Implement Mul Recover Instruction. r=nbp
> 
> Could this be related to the Mul recover instruction?

Strangely, but yes.
Looking at the code, it seems that the MMul::mode is not encoded by the writeRecoverData function[1].

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/Recover.cpp#404

	Hannes Verschore [:h4writer]
	Comment 6 • 9 years ago

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> (In reply to Hannes Verschore [:h4writer] from comment #4)
> > summary:     Bug 1011540 - Implement Mul Recover Instruction. r=nbp
> > 
> > Could this be related to the Mul recover instruction?
> 
> Strangely, but yes.
> Looking at the code, it seems that the MMul::mode is not encoded by the
> writeRecoverData function[1].
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/Recover.cpp#404

The MMul::mode is actually never used. It sets everything on the MMul correctly: canBeNegativeZero and canOverflow. I assume we need to encode one of those. I assume "canOverflow". (Which in the recover state will have to truncate when canOverflow is false).

	Hannes Verschore [:h4writer]
	Comment 7 • 9 years ago

(In reply to Hannes Verschore [:h4writer] from comment #6)
> (In reply to Nicolas B. Pierron [:nbp] from comment #5)
> > (In reply to Hannes Verschore [:h4writer] from comment #4)
> > > summary:     Bug 1011540 - Implement Mul Recover Instruction. r=nbp
> > > 
> > > Could this be related to the Mul recover instruction?
> > 
> > Strangely, but yes.
> > Looking at the code, it seems that the MMul::mode is not encoded by the
> > writeRecoverData function[1].
> > 
> > [1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/Recover.cpp#404
> 
> The MMul::mode is actually never used. It sets everything on the MMul
> correctly: canBeNegativeZero and canOverflow. I assume we need to encode one
> of those. I assume "canOverflow". (Which in the recover state will have to
> truncate when canOverflow is false).

I mean truncate_. But like suggested, just encoding mode_ would fix this too... (Changed my mind here)

	Nicolas B. Pierron [:nbp] Assignee
	Comment 8 • 9 years ago

Note truncate_ might be set by range analysis after optimizations such as UCE (within GVN now), thus we might set the truncate_ flag, which does not mirror the state of the original statement and thus could cause other differential behaviour.

	Nicolas B. Pierron [:nbp] Assignee
	Comment 9 • 9 years ago

This patch fix the issue, and the added test cases highlight the failure of
properly recovering Math.imul as an Int32 operation instead of a Number
operation.

	Hannes Verschore [:h4writer]
	Comment 10 • 9 years ago

Comment on attachment 8704228 [details] [diff] [review]
IonMonkey: Recover Math.imul as an int32 operation.

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

Nice catch.

	Pulsebot
	Comment 11 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 12 • 9 years ago
bugherder

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

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 13 • 8 years ago

Nicolas, do you mind nominating this for approval‑mozilla‑esr45 as well? It would aid fuzzing on that branch.

	Nicolas B. Pierron [:nbp] Assignee
	Comment 14 • 8 years ago

Comment on attachment 8704228 [details] [diff] [review]
IonMonkey: Recover Math.imul as an int32 operation.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: differential testing fuzzing. (comment 13)

User impact if declined: low
Fix Landed on Version: 46
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

	Sylvestre Ledru [:Sylvestre]
	Comment 15 • 8 years ago

Comment on attachment 8704228 [details] [diff] [review]
IonMonkey: Recover Math.imul as an int32 operation.

Improve the worker of the fuzzer, taking it.
Should be in 45.1.0

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-esr45/rev/dbb29f84cc6a