Closed Bug 1036781 Opened 10 years ago Closed 10 years ago

Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH in js/src/jit

Categories

(Core :: JavaScript Engine: JIT, defect)

Product:
Core
Component:
JavaScript Engine: JIT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35
Tracking Flags:
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(1 file)

replace-js-jit-MOZ_ASSUME_UNREACHABLE.patch
301.05 KB, patch
jandem
: review+
Details | Diff | Splinter Review 
Replace calls to the deprecated MOZ_ASSUME_UNREACHABLE macro with MOZ_CRASH.

Calls to MOZ_NOT_REACHED were replaced with MOZ_ASSUME_UNREACHABLE which, instead of crashing unconditionally, invokes undefined behavior in the name of dubious and dangerous compiler optimizations. See bug 990764 for details. This patch reverts to the original crashing behavior.

(MOZ_ASSUME_UNREACHABLE will eventually be removed or renamed in bug 990764.)
Attachment #8453545 - Flags: review?(jdemooij)
Comment on attachment 8453545 [details] [diff] [review]
replace-js-jit-MOZ_ASSUME_UNREACHABLE.patch

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

Looks good, thanks!
Attachment #8453545 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/805ac89b5924
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
This would appear to the most likely bug in the range that caused a 52% regression on awfy on asmjs-ubench-skinning on 32-bit mac both asm.js and no-asm.js.
(In reply to Luke Wagner [:luke] from comment #4)
> This would appear to the most likely bug in the range that caused a 52%
> regression on awfy on asmjs-ubench-skinning on 32-bit mac both asm.js and
> no-asm.js.

I'm not familiar with the benchmark, but "ubench" in the name suggests we don't spend a lot of time compiling, right? Maybe we call some VM function that's now slower?
I can back out this change. Later I will submit a new patch for review that renames these MOZ_ASSUME_UNREACHABLE calls to MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE (with equivalent behavior with a new name) instead of MOZ_CRASH.
Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67c13008762f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Oops, I missed these replies.

(In reply to Jan de Mooij [:jandem] from comment #5)
> I'm not familiar with the benchmark, but "ubench" in the name suggests we
> don't spend a lot of time compiling, right? Maybe we call some VM function
> that's now slower?

Yes, I expect you're right; compilation time should be inconsequential for skinning.  A VM path was affected.  A little profiling would hopefully point to the offender.
Chris let me know if you want help tracking this down.
Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH and MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE in js/src/asmjs:

https://hg.mozilla.org/integration/mozilla-inbound/rev/264ff666617e
status-firefox33: fixed → wontfix
status-firefox34: --- → affected
Keywords: leave-open
For what it's worth, AWFY isn't updating at the moment, but local testing says there's no slowdown for asm.js micro benchmarks on a x86 build.
Part 2: js/src/jit/x86 with only 1.8% regression on asmjs-ubench-skinning

But I backed out and relanded to fix the incorrect bug number in the commit message. :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e3534959ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/33373d724284
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2ceee36816
Part 3: Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH in js/src/jit for x86 and x64:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff9ac3b549cc
Part 4: Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH in js/src/jit for Baseline code.

https://hg.mozilla.org/integration/mozilla-inbound/rev/21bc84614aa2
Part 5: Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH in js/src/jit for Ion.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdc4a4fe462f
Part 6: Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH in js/src/jit/arm/
Part 7: Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH in js/src/jit/mips/

https://hg.mozilla.org/integration/mozilla-inbound/rev/d89484b7c0fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b719ddeea01
I backed out the ARM changes to see if it burned Android 2.3 Opt, whose tests started timing out with this changeset.

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7b719ddeea01

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeee10f1b325
I just thought of something: MOZ_CRASH("why") will actually print "why" in a release build (to stderr), right?  Has anyone measured the binary size increase from all these otherwise-unused string literals?  It seems pretty easy to measure (just change MOZ_CRASH not to embed the string and measure binary before/after).
(In reply to Luke Wagner [:luke] from comment #23)
> I just thought of something: MOZ_CRASH("why") will actually print "why" in a
> release build (to stderr), right?  Has anyone measured the binary size
> increase from all these otherwise-unused string literals?  It seems pretty
> easy to measure (just change MOZ_CRASH not to embed the string and measure
> binary before/after).

Only in release builds:

 * If we're a DEBUG build and we crash at a MOZ_CRASH which provides an
 * explanation-string, we print the string to stderr.  Otherwise, we don't
 * print anything; this is because we want MOZ_CRASH to be 100% safe in release
 * builds, and it's hard to print to stderr safely when memory might have been
 * corrupted.

("Don't increase the binary size" could probably also be mentioned there...)
(In reply to Jan de Mooij [:jandem] from comment #24)
> Only in release builds:

Oops, s/release/debug/ obviously.
Oops, thanks, I misread the code.
Part 8: Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH in js/src/jit/*Allocator.* code

https://hg.mozilla.org/integration/mozilla-inbound/rev/66e350a413a6
Part 9: Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH in js/src/jit/arm/

https://hg.mozilla.org/integration/mozilla-inbound/rev/499e35dfdace
Part 10: Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH in js/src/jit MIR code.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8106c19ed3b0
Part 12: Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH in js/src/jit:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb651803afe9
status-firefox34: affected → wontfix
status-firefox35: --- → fixed
Keywords: leave-open
With this final patch, there was no asmjs-ubench-skinning regression:

http://arewefastyet.com/#machine=11&view=single&suite=asmjs-ubench&subtest=skinning

But there was minor regression on Octane DeltaBlue, Splay, and SplayLatency but it looks like a temporary blip:

http://arewefastyet.com/#machine=11&view=breakdown&suite=octane
https://hg.mozilla.org/mozilla-central/rev/fb651803afe9
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: mozilla33 → mozilla35
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: