Closed
Bug 947601
Opened 10 years ago
Closed 10 years ago
OdinMonkey: Add support for assume_unreachable
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: h4writer, Assigned: romain.perier)
References
Details
Attachments
(2 files, 5 obsolete files)
10 years ago
(deleted),
patch
|
Details | Diff | Splinter Review | |
3.74 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → romain.perier
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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...
Assignee | ||
Updated•10 years ago
|
Attachment #8388372 -
Flags: review?(hv1989)
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8388372 [details] [diff] [review] OdinMonkey: Add support for assume_unreachable. One of you :)
Attachment #8388372 -
Flags: review?(luke)
Reporter | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8388691 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8388372 -
Attachment is obsolete: true
Attachment #8388372 -
Flags: review?(luke)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8388702 -
Flags: review?(luke)
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8388702 -
Attachment is obsolete: true
Attachment #8388702 -
Flags: review?(luke)
Assignee | ||
Comment 11•10 years ago
|
||
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)
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8388714 -
Attachment is obsolete: true
Attachment #8388714 -
Flags: review?(luke)
Assignee | ||
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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.
Assignee | ||
Comment 15•10 years ago
|
||
(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...)
Assignee | ||
Comment 16•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8389030 -
Attachment is obsolete: true
Attachment #8389030 -
Flags: review?(luke)
Assignee | ||
Updated•10 years ago
|
Attachment #8389203 -
Flags: review?(luke)
Comment 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
Yes, I did. I never upload a patch which breaks tests. I run them all the time ;)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/77b2ebd5ffff
Keywords: checkin-needed
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/77b2ebd5ffff
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•