947188 - Jit: Add a way to bake MOZ_ASSUME_UNREACHABLE in generated code.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Currently we use masm.breakpoint to indicate we are in an invalid state/place. This isn't really helpful, since it is hard to distinguish all masm.breakpoints. Therefore I want to introduce masm.assume_unreachable(explanation);

In release builds this will just do masm.breakpoint again, but in debug files it will assert with the given explanation. Making it easier to categorize those failures or to pinpoint the possible location/reason for this error.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: bug947188-unreachable

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8343712 [details] [diff] [review]
bug947188-unreachable

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

::: js/src/jit/IonMacroAssembler.cpp
@@ +1205,5 @@
>          sps_->reenter(*this, InvalidReg);
>  }
>  
> +static void assume_unreachable_(const char *output) {
> +    MOZ_ReportAssertionFailure(output, __FILE__, __LINE__);

Would we have a way to carry the C++ / JSScript location of the caller of masm.assumeUnreachable ?

Comment 3

[Hannes Verschore [:h4writer]]

This patch doesn't give the right __FILE__ and __LINENO__. It is possible to also get that correct. Now I think we only need to agree what the best syntax would be:

1) Nothing fancy, just write the following everywhere:
masm.assume_unreachable("reason", __FILE__, __LINENO__);

2) Macro 1:
MASM_ASSUME_UNREACHABLE(masm, "reason")
(converts to masm.assume_unreachable("reason", __FILE__, __LINENO__))

3) Macro 2:
masm.ASSUME_UNREACHABLE("reason")
(converts to masm.assume_unreachable("reason", __FILE__, __LINENO__))

4) Macro 3:
masm.assume_unreachable("reason", SRC_LOCATION);

...

Comment 4

[Hannes Verschore [:h4writer]]

<h4writer> nbp, thinking about it, would that actually make sense? I mean, the given filename/lineno is from where we baked that "assume unreached", not from when we hit that condition
<h4writer> nbp, it would be strange to have a "backtrace" where the filename/lineno doesn't match the printed place
<h4writer> nbp, and it is quick easy to match it with the place where we baked it in. Just do a quick grep ...

Comment 5

[Kannan Vijayan [:djvj]]

Comment on attachment 8343712 [details] [diff] [review]
bug947188-unreachable

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

::: js/src/jit/IonMacroAssembler.cpp
@@ +1205,5 @@
>          sps_->reenter(*this, InvalidReg);
>  }
>  
> +static void assume_unreachable_(const char *output) {
> +    MOZ_ReportAssertionFailure(output, __FILE__, __LINE__);

I'd just omit the __FILE__, __LINE__ for now.  Just the message is way better than a breakpoint which we have now.

As Nicolas noted it's pretty useless in its current form since it'll always be IonMacroAssembler.cpp:NNNN

Comment 6

[Hannes Verschore [:h4writer]]

Note: I had to disable support for AsmJS, since ImmPtr is disabled there.

https://hg.mozilla.org/integration/mozilla-inbound/rev/162e0b8608a1

Comment 7

[Jan de Mooij [:jandem]]

Drive-by style nit: assume_unreachable -> assumeUnreachable?

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/162e0b8608a1

Comment 9

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> Drive-by style nit: assume_unreachable -> assumeUnreachable?

Good point. Created follow-up to fix ;).