1487097 - Audit uses of MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE in js/src

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jason Orendorff [:jorendorff]]

See bug 990764 (5 years ago).

MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE() could be called MOZ_UNDEFINED_BEHAVIOR(). The purpose is to tell the compiler "we're pretty sure this can't happen; anyway if it does happen we're ok with just executing any of the other branches, or even jumping to a random address," the idea being that the C++ compiler might then eliminate a branch that "can't" be taken, or a bounds check that "can't" fail. Compilers don't necessarily do so, even in opt builds. See <https://raw.githubusercontent.com/bjacob/builtin-unreachable-study/master/notes>.

Every use of this macro is a security risk. We don't audit them. It should be used only in cases where (a) we are *very* sure it really is unreachable; AND (b) we actually get enough of a speed boost that we care about it.

There are 11 current uses in js/src. If I had to guess how many qualify, I'd guess none; but the number might be as high as 4.

Comment 1

[Nicolas B. Pierron [:nbp]]

I will note that many of these uses have 2 valid branches, and having a MOZ_CRASH instead of a MOZ_*_UNREACHABLE will add an extra branch for one of the cases, but I do not think any of our current use cases might appear significantly in profiles.

However, we have more than 11 uses of this macro, as it is aliased in the VIXL backend [1]. So we should probably ask Lars to double check the performance of Rabaldr after replacing VIXL_UNREACHABLE by a MOZ_CRASH.

[1] https://searchfox.org/mozilla-central/search?q=VIXL_UNREACHABLE&redirect=false

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1487097 - Remove all uses of MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE in js/src. r=tcampbell

Comment 3

[Jason Orendorff [:jorendorff]]

Two and a half years later, it's up from 11 to 15. ಠ_ಠ

I am fairly sure every single one of these is unwarranted, so we might as well get clear of it.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1fd50f2d1a4e
Remove all uses of MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE in js/src. r=nbp

Comment 5

[Lars T Hansen [:lth]]

Most uses in wasm are there to placate the C++ compiler because they are preceded by switches that are exhaustive but the compiler refuses to acknowledge that. Basically, this change adds a redundant default case to those switches. Looking over the patch, the only change that might even be plausibly perf-sensitive is the one in ToMIRType, but even there I wouldn't be too concerned.

Comment 6

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/1fd50f2d1a4e