Closed
Bug 1240880
Opened 9 years ago
Closed 9 years ago
Assertion failure: consumer->isConsistentFloat32Use(use.use()), at js/src/jit/IonAnalysis.cpp:1791
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla46
| Tracking | Status | |
|---|---|---|
| firefox45 | --- | wontfix |
| firefox46 | --- | verified |
| firefox-esr38 | --- | unaffected |
| firefox-esr45 | 48+ | fixed |
People
(Reporter: gkw, Assigned: h4writer)
References
Details
(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update][adv-main46+][adv-esr45.3+])
Attachments
(1 file, 1 obsolete file)
|
842 bytes,
patch
|
h4writer
:
review+
Sylvestre
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision b67316254602 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):
function f(x) {
"use strict";
Math.fround(this === x && x);
}
f();
f();
(function() {
f(Math.fround(0));
})();
Backtrace:
0 js-dbg-64-dm-darwin-b67316254602 0x0000000100232e64 js::jit::ApplyTypeInformation(js::jit::MIRGenerator*, js::jit::MIRGraph&) + 6068 (IonAnalysis.cpp:1791)
1 js-dbg-64-dm-darwin-b67316254602 0x000000010022a50b js::jit::OptimizeMIR(js::jit::MIRGenerator*) + 1835 (Ion.cpp:1634)
2 js-dbg-64-dm-darwin-b67316254602 0x0000000100235f53 js::jit::CompileBackEnd(js::jit::MIRGenerator*) + 67 (Ion.cpp:2011)
3 js-dbg-64-dm-darwin-b67316254602 0x000000010023804b js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*, bool, bool) + 4587 (Ion.cpp:2282)
4 js-dbg-64-dm-darwin-b67316254602 0x00000001002389b2 js::jit::CanEnter(JSContext*, js::RunState&) + 370 (Ion.cpp:2614)
5 js-dbg-64-dm-darwin-b67316254602 0x0000000100703f61 js::RunScript(JSContext*, js::RunState&) + 289 (Interpreter.cpp:405)
6 js-dbg-64-dm-darwin-b67316254602 0x00000001006f21b9 js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 841 (Interpreter.cpp:499)
7 js-dbg-64-dm-darwin-b67316254602 0x000000010071b4db js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) + 555 (Interpreter.cpp:533)
8 js-dbg-64-dm-darwin-b67316254602 0x0000000100197ddf js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 2655 (BaselineIC.cpp:6186)
9 ??? 0x0000000102a213ab 0 + 4339143595
10 ??? 0x0000000103e54d58 0 + 4360326488
Setting s-s as a start as this seems to involve MIR.
| 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/2db399cd414f
parent: 257780:0760af2a400f
user: Hannes Verschore
date: Fri Aug 14 11:46:28 2015 +0200
summary: Bug 1193112: IonMonkey - Let the float32 optimization work with Float32, r=bbouvier
Hannes, is bug 1193112 a likely regressor?
Blocks: 1193112
Flags: needinfo?(hv1989)
| Reporter | ||
Updated•9 years ago
|
OS: Mac OS X → All
| Assignee | ||
Comment 2•9 years ago
|
||
Only do these conversions to float32, if the input is typed MIRType_Double; Else this can be problematic like in this example. We decide the MFilterTypeSet should return a "Float32", while it typeset is empty and should be MIRType_Value.
Comment 3•9 years ago
|
||
Comment on attachment 8710362 [details] [diff] [review]
Patch
Review of attachment 8710362 [details] [diff] [review]:
-----------------------------------------------------------------
If the checks in canProduceFlaot32/canConsumeFloat32 are necessary, please let me know. That seems overly conservative at first glance, but I might be proven wrong.
Also, test case please? :-)
::: js/src/jit/MIR.cpp
@@ +2443,5 @@
>
> void
> MFilterTypeSet::trySpecializeFloat32(TempAllocator& alloc)
> {
> + if (!IsFloatingPointType(type()))
This shouldn't be needed: trySpecializeFloat32 is called only if isFloatCommutative returns true. So there's no way we can end up being here if !IsFloatingPointType(type()) as isFloatCommutative would have returned false.
@@ +2456,5 @@
>
> bool
> MFilterTypeSet::canProduceFloat32() const
> {
> + if (!IsFloatingPointType(type()))
I don't think this is necessary.
@@ +2469,5 @@
>
> bool
> MFilterTypeSet::canConsumeFloat32(MUse* operand) const
> {
> + if (!IsFloatingPointType(type()))
And this isn't as well.
::: js/src/jit/MIR.h
@@ +12597,5 @@
>
> + bool isFloat32Commutative() const override {
> + if (!IsFloatingPointType(type()))
> + return false;
> + return true;
Can simply be:
return IsFloatingPointType(type());
Attachment #8710362 -
Flags: review?(bbouvier) → review+
| Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Comment on attachment 8710362 [details] [diff] [review]
> Patch
>
> Review of attachment 8710362 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> If the checks in canProduceFlaot32/canConsumeFloat32 are necessary, please
> let me know. That seems overly conservative at first glance, but I might be
> proven wrong.
>
> Also, test case please? :-)
needinfo myself to upload testcase later.
Flags: needinfo?(hv1989)
Comment 5•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
Comment 6•9 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•9 years ago
|
Group: javascript-core-security → core-security-release
Comment 7•9 years ago
|
||
Why was this checked in without sec-approval?
From the date in comment 1, this probably affects ESR45 (and affected Firefox 45) and should have gone through getting a security rating and sec-approval before checkin.
Can you please set the sec-approval? flag and answer the template questions?
status-firefox45:
--- → wontfix
status-firefox-esr38:
--- → unaffected
status-firefox-esr45:
--- → affected
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main45+]
Updated•9 years ago
|
Whiteboard: [jsbugmon:update][adv-main45+] → [jsbugmon:update][adv-main46+]
Comment 8•9 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #4)
> needinfo myself to upload testcase later.
You can use the "in-testsuite?" flag and then have a query/whine that shows bugs assigned to you in the in-testsuite? state.
(In reply to Phil Ringnalda (:philor) from comment #5)
> https://hg.mozilla.org/mozilla-central/rev/de993f2b8952
This seems to be only part of the patch attached to this bug -- what happened to the MIR.cpp bits?
Updated•9 years ago
|
Flags: needinfo?(bbouvier)
Comment 9•9 years ago
|
||
No idea, the full extent of my involvement with it was typing "hg pull m-i && hg merge && hg commit -m "Merge m-i to m-c, a=merge""
Flags: needinfo?(philringnalda)
Comment 10•9 years ago
|
||
Not sure why I have been needinfo'd, did you have any specific question in mind that I could help answering?
For what it's worth, the review in comment 3 seems to indicate that the MIR.cpp bits were not needed after all. I'd try pinging h4writer on irc about this bug, as I can see he still has a needinfo on this particular bug.
Flags: needinfo?(bbouvier)
Comment 11•9 years ago
|
||
Any update here? Does this need to be on ESR? And is there something still missing?
Eric, can you help resolve this? We need sec-approval request and also if this is needed in ESR, an uplift request.
Flags: needinfo?(efaustbmo)
Comment 12•9 years ago
|
||
This will have to wait for ESR 45.3.0 since we are going to build on ESR 45.2.0 tonight.
Comment 13•9 years ago
|
||
Looks like Hannes is already ni? here. I will ask him to nominate for sec-approval and uplifts all around. Leaving the ni? for reminder.
| Assignee | ||
Comment 14•9 years ago
|
||
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined:
Wrong JS values, maybe crashes and I wouldn't rule out exploitable (but I have no POC and haven't tried to come up with one) with easy fix.
Fix Landed on Version:
FF46
Risk to taking this patch (and alternatives if risky):
Very low. Patch is very small and has been working in FF46. No additional fixes were needed on top of this bug.
String or UUID changes made by this patch:
/
See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8710362 -
Attachment is obsolete: true
Flags: needinfo?(efaustbmo)
Attachment #8759612 -
Flags: review+
Attachment #8759612 -
Flags: approval-mozilla-esr45?
Comment 15•9 years ago
|
||
Comment on attachment 8759612 [details] [diff] [review]
Updated patch (works also on ESR45)
let's take it for esr 45.3.0
Attachment #8759612 -
Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Comment 16•9 years ago
|
||
| Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(hv1989)
Updated•9 years ago
|
Whiteboard: [jsbugmon:update][adv-main46+] → [jsbugmon:update][adv-main46+][adv-esr45.3+]
Updated•9 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•