Closed Bug 1399471 Opened 7 years ago Closed 7 years ago

Check callWithABI invariants in debug builds

Categories

(Core :: JavaScript Engine: JIT, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(2 files)

Attached patch PatchSplinter 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)
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/21edf7e4f6ad
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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
Flags: needinfo?(jdemooij)
Resolution: FIXED → ---
Target Milestone: mozilla57 → ---
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.
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)
Attachment #8909297 - Flags: review?(nicolas.b.pierron) → review+
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
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
OK let's try this again. Seems green on Try now on top of part 1 I pushed yesterday...
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/b481e36a9958
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.