947601 - OdinMonkey: Add support for assume_unreachable

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

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.

Comment 1

[Romain Perier]

Attachment: OdinMonkey: Add support for assume_unreachable.

Comment 2

[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.

Comment 3

[Romain Perier]

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...

Comment 4

[Romain Perier]

Comment on attachment 8388372 [details] [diff] [review]
OdinMonkey: Add support for assume_unreachable.

One of you :)

Comment 5

[Hannes Verschore [:h4writer]]

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 ;) .

Comment 6

[Luke Wagner [:luke]]

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.

Comment 7

[Romain Perier]

(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.

Comment 8

[Romain Perier]

Attachment: OdinMonkey: Add support for assume_unreachable.

Comment 9

[Romain Perier]

Attachment: patch version 2

Comment 10

[Romain Perier]

Attachment: OdinMonkey: Add support for assume_unreachable.

Comment 11

[Romain Perier]

Comment on attachment 8388714 [details] [diff] [review]
OdinMonkey: Add support for assume_unreachable.

I forgot the comment. It's okay now ;)

Comment 12

[Romain Perier]

Attachment: OdinMonkey: Add support for assume_unreachable.

Comment 13

[Romain Perier]

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.

Comment 14

[Luke Wagner [: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.

Comment 15

[Romain Perier]

(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...)

Comment 16

[Romain Perier]

Attachment: OdinMonkey: Add support for assume_unreachable.

Comment 17

[Luke Wagner [: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?)

Comment 18

[Romain Perier]

Yes, I did. I never upload a patch which breaks tests. I run them all the time ;)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77b2ebd5ffff

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77b2ebd5ffff