Closed
Bug 1399471
Opened 7 years ago
Closed 7 years ago
Check callWithABI invariants in debug builds
Categories
(Core :: JavaScript Engine: JIT, enhancement, P2)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
Details
Attachments
(2 files)
87.66 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
5.03 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
masm.callWithABI can be used to call C++ functions directly. This is faster than callVM etc, but it's also more risky: if we don't push an exit frame, the callee is not allowed to GC, walk the stack, etc. To make it easier to find these bugs, I'm attaching a patch that instruments callWithABI to require (1) the callee to use AutoUnsafeCallWithABI (which contains AutoCheckCannotGC) or (2) an annotation passed to callWithABI to say "this call is okay". (2) can be used when we pushed an exit frame for instance or when we're calling a builtin function like malloc. There are some other things I want to do with this in follow-up bugs, but let's get this landed first.
Attachment #8907592 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 1•7 years ago
|
||
Comment on attachment 8907592 [details] [diff] [review] Patch Review of attachment 8907592 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MacroAssembler.cpp @@ +763,5 @@ > // initializing writes. > return IsNurseryAllocable(allocKind) && initialHeap != gc::TenuredHeap; > } > > +// Inline version of Nursery::allocateObject. If the object has dynamic slots,x Typo, removed locally.
Comment 2•7 years ago
|
||
Comment on attachment 8907592 [details] [diff] [review] Patch Review of attachment 8907592 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MacroAssembler.h @@ -565,5 @@ > inline void passABIArg(Register reg); > inline void passABIArg(FloatRegister reg, MoveOp::Type type); > > - template <typename T> > - inline void callWithABI(const T& fun, MoveOp::Type result = MoveOp::GENERAL); Thanks for removing the template from the MacroAssembler generic interface.
Attachment #8907592 -
Flags: review?(nicolas.b.pierron) → review+
Updated•7 years ago
|
status-firefox57:
--- → affected
Priority: -- → P2
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/21edf7e4f6ad Add a mechanism to check callWithABI invariants in debug builds. r=nbp
Comment 4•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/21edf7e4f6ad
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 5•7 years ago
|
||
Backed out for frequently failing wpt /IndexedDB/interleaved-cursors.html and browser-chrome tests, all on Linux32 debug: https://hg.mozilla.org/mozilla-central/rev/f954ddf67d55cb5b5cb623e7adc95f2637742a91 See pushes on Treeherder: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&tochange=21edf7e4f6adee7c85cef2e5827bc5431a62d7e6&fromchange=2769237547ba7e8d1c61bd95b2e65de8611016be&filter-searchStr=linux32%20debug Failure log indexeddb: https://treeherder.mozilla.org/logviewer.html#?job_id=131323349&repo=mozilla-inbound > TEST-UNEXPECTED-TIMEOUT | /IndexedDB/interleaved-cursors.html | 500 cursors - Test timed out
Status: RESOLVED → REOPENED
status-firefox57:
fixed → ---
Flags: needinfo?(jdemooij)
Resolution: FIXED → ---
Target Milestone: mozilla57 → ---
Assignee | ||
Comment 6•7 years ago
|
||
Hm this sucks. These tests timed out before and this patch probably just made them a little slower. I'll see what I can do.
Assignee | ||
Comment 7•7 years ago
|
||
I looked at the WPT timeout. We were spending a *lot* of time under AssertValidStringPtr, also without the patch. In bug 1331414 Hannes added a pref to disable some slow debug checks. I think we should enable this in browser builds. The patch changes this so we still emit the TI checks if the pref is disabled - these checks are much faster and still useful to have. This patch fixes the WPT timeouts at least.
Flags: needinfo?(jdemooij)
Attachment #8909297 -
Flags: review?(nicolas.b.pierron)
Updated•7 years ago
|
Attachment #8909297 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 8•7 years ago
|
||
Because pulsebot seems to be MIA: https://hg.mozilla.org/integration/mozilla-inbound/rev/e758012cf5b3eadc98f2c5428724e483338ec11b This will make our debug tests a bit faster in automation, FWIW.
Keywords: leave-open
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d51f41589301 part 1 - Disable the full_debug_checks jit pref by default in browser builds. r=nbp
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d51f41589301
Comment 11•7 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b481e36a9958 part 2 - Add a mechanism to check callWithABI invariants in debug builds. r=nbp
Assignee | ||
Comment 12•7 years ago
|
||
OK let's try this again. Seems green on Try now on top of part 1 I pushed yesterday...
Keywords: leave-open
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b481e36a9958
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•