Closed Bug 947601 Opened 6 years ago Closed 6 years ago

OdinMonkey: Add support for assume_unreachable

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: h4writer, Assigned: romain.perier)

References

Details

Attachments

(2 files, 5 obsolete files)

Follow-up for bug 947188. Currently assume_unreachable has the old behaviour for AsmJS. Shouldn't be to hard to let it work. It is just because AsmJS forbids ImmPtr very strictly. Should be possible to bypass easily.
Depends on: 947188
Assignee: nobody → romain.perier
This is a first version. I am not sure at all that passing the argument in this way is correct. I mean, If I pass it throught a movePtr(ImmPtr(...)  , it is not patched ... however... If I use AsmJSImmPtr... there are no ways to pass the pointer to "output" as argument... I am confused.
Tests pass, assumeUnreachable works from asm.js but I am not convinced... I mean, in this case there are no real differences between ImmPtr and ImmWord...
Attachment #8388372 - Flags: review?(hv1989)
Comment on attachment 8388372 [details] [diff] [review]
OdinMonkey: Add support for assume_unreachable.

One of you :)
Attachment #8388372 - Flags: review?(luke)
Comment on attachment 8388372 [details] [diff] [review]
OdinMonkey: Add support for assume_unreachable.

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

I'll let luke review. I think I would have done something similar. So I guess it is correct. But luke will know for sure ;) .
Attachment #8388372 - Flags: review?(hv1989)
Comment on attachment 8388372 [details] [diff] [review]
OdinMonkey: Add support for assume_unreachable.

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

Thanks for looking at this!  One comment below.  Let me know what you think and upload a new patch if you agree.

::: js/src/jit/IonMacroAssembler.cpp
@@ +1287,3 @@
>  
> +    if (IsCompilingAsmJS()) {
> +        movePtr(ImmWord(uintptr_t(output)), temp);

From IRC: with ASLR, we can't rely on 'output' to point to the same char array after serialization/deserialization.  That is why ImmWord is only supposed to be used for values that are word-sized but not dependent on the address space layout in any way.  My recommendation is to just ignore the 'output' arg (and put a nice comment why).  That is less useful for debugging, but at least it preserves the assertion.
(In reply to Luke Wagner [:luke] from comment #6)
> Comment on attachment 8388372 [details] [diff] [review]
> OdinMonkey: Add support for assume_unreachable.
> 
> Review of attachment 8388372 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for looking at this!  One comment below.  Let me know what you think
> and upload a new patch if you agree.
> 
> ::: js/src/jit/IonMacroAssembler.cpp
> @@ +1287,3 @@
> >  
> > +    if (IsCompilingAsmJS()) {
> > +        movePtr(ImmWord(uintptr_t(output)), temp);
> 
> From IRC: with ASLR, we can't rely on 'output' to point to the same char
> array after serialization/deserialization.  That is why ImmWord is only
> supposed to be used for values that are word-sized but not dependent on the
> address space layout in any way.  My recommendation is to just ignore the
> 'output' arg (and put a nice comment why).  That is less useful for
> debugging, but at least it preserves the assertion.
Mhhh, I understand better now why it does not make sense to pass an address in this way.
Making this "address passing" possible would be a big change for no big gain, I agree with you.
As discussed on IRC, call MOZ_CRASH(<generic error>) from a wrapped function looks a good idea.
Attachment #8388691 - Attachment is obsolete: true
Attachment #8388372 - Attachment is obsolete: true
Attachment #8388372 - Flags: review?(luke)
Attached patch patch version 2 (obsolete) — Splinter Review
Attachment #8388702 - Flags: review?(luke)
Attachment #8388702 - Attachment is obsolete: true
Attachment #8388702 - Flags: review?(luke)
Comment on attachment 8388714 [details] [diff] [review]
OdinMonkey: Add support for assume_unreachable.

I forgot the comment. It's okay now ;)
Attachment #8388714 - Flags: review?(luke)
Attachment #8388714 - Attachment is obsolete: true
Attachment #8388714 - Flags: review?(luke)
Comment on attachment 8389030 [details] [diff] [review]
OdinMonkey: Add support for assume_unreachable.

I updated how we use setupABICall before calling the function, as the one called from asm.js no longer requires arguments.
Attachment #8389030 - Flags: review?(luke)
Comment on attachment 8389030 [details] [diff] [review]
OdinMonkey: Add support for assume_unreachable.

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

::: js/src/jit/AsmJSModule.cpp
@@ +211,5 @@
>  }
>  
> +#ifdef DEBUG
> +static void
> +AssumeUnreachable_() {

For non-member functions, the SM style is to put the { on a new line.  Also, can you take off the trailing underscore (I'm not sure why they added it in the other place)?

::: js/src/jit/IonMacroAssembler.cpp
@@ +1278,5 @@
>  void
>  MacroAssembler::assumeUnreachable(const char *output)
>  {
>  #ifdef DEBUG
> +    // with ASLR, we can't rely on 'output' to point to the

Can you capitalize "With"?

@@ +1283,5 @@
> +    // same char array after serialization/deserialization.
> +    // It is not possible until we modify AsmJsImmPtr and
> +    // the underlying "patching" mechanism.
> +    if (IsCompilingAsmJS()) {
> +        setupAlignedABICall(0);

Even though you aren't passing an argument, since assumeUnreachable can be called at any stack depth, I think we can't trust that the stack pointer is aligned (according to the ABI) for a call.  Thus, I think you need to call setupUnalignedABICall() just like the other branch.
(In reply to Luke Wagner [:luke] from comment #14)
> Comment on attachment 8389030 [details] [diff] [review]
> OdinMonkey: Add support for assume_unreachable.
> 
> Review of attachment 8389030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/AsmJSModule.cpp
> @@ +211,5 @@
> >  }
> >  
> > +#ifdef DEBUG
> > +static void
> > +AssumeUnreachable_() {
> 
> For non-member functions, the SM style is to put the { on a new line.  Also,
> can you take off the trailing underscore (I'm not sure why they added it in
> the other place)?
sure no problem.
> 
> ::: js/src/jit/IonMacroAssembler.cpp
> @@ +1278,5 @@
> >  void
> >  MacroAssembler::assumeUnreachable(const char *output)
> >  {
> >  #ifdef DEBUG
> > +    // with ASLR, we can't rely on 'output' to point to the
> 
> Can you capitalize "With"?
ack
> 
> @@ +1283,5 @@
> > +    // same char array after serialization/deserialization.
> > +    // It is not possible until we modify AsmJsImmPtr and
> > +    // the underlying "patching" mechanism.
> > +    if (IsCompilingAsmJS()) {
> > +        setupAlignedABICall(0);
> 
> Even though you aren't passing an argument, since assumeUnreachable can be
> called at any stack depth, I think we can't trust that the stack pointer is
> aligned (according to the ABI) for a call.  Thus, I think you need to call
> setupUnalignedABICall() just like the other branch.
good catch, I am wondering why test suites did not catch this case... (well, perhaps it is not completly deterministic...)
Attachment #8389030 - Attachment is obsolete: true
Attachment #8389030 - Flags: review?(luke)
Attachment #8389203 - Flags: review?(luke)
Comment on attachment 8389203 [details] [diff] [review]
OdinMonkey: Add support for assume_unreachable.

Perfect!  (I assume from your previous comment that you've run js/src/jit-tests/jit_tests.py?)
Attachment #8389203 - Flags: review?(luke) → review+
Yes, I did. I never upload a patch which breaks tests. I run them all the time ;)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/77b2ebd5ffff
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.