Closed Bug 947188 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

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.
Assignee: nobody → hv1989
Attachment #8343712 - Flags: review?(kvijayan)
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 ?
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);

...
<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 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
Attachment #8343712 - Flags: review?(kvijayan) → review+
Note: I had to disable support for AsmJS, since ImmPtr is disabled there.

https://hg.mozilla.org/integration/mozilla-inbound/rev/162e0b8608a1
Blocks: 947601
Drive-by style nit: assume_unreachable -> assumeUnreachable?
Flags: needinfo?(hv1989)
https://hg.mozilla.org/mozilla-central/rev/162e0b8608a1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(In reply to Jan de Mooij [:jandem] from comment #7)
> Drive-by style nit: assume_unreachable -> assumeUnreachable?

Good point. Created follow-up to fix ;).
Blocks: 947765
Flags: needinfo?(hv1989)
Blocks: 899998
Duplicate of this bug: 899998
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.