Closed
Bug 906128
Opened 11 years ago
Closed 11 years ago
Add missing post barriers in createCallObject
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
DUPLICATE
of bug 906241
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review |
When messing with what gets pre-tenured in bug 889129, we changed what gets put into the store buffer slightly and this popped up immediately on PdfJS.
Attachment #791392 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 1•11 years ago
|
||
Comment on attachment 791392 [details] [diff] [review]
add_missing_barriers_in_createCallObject-v0.diff
Uhg, we need the args too, so this should probably be unconditional. I'll add a new M/LPostWriteBarrier type for this.
Attachment #791392 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 2•11 years ago
|
||
This is substantially more code, but doesn't regress.
Attachment #791392 -
Attachment is obsolete: true
Attachment #791484 -
Flags: review?(bhackett1024)
Comment 3•11 years ago
|
||
Comment on attachment 791484 [details] [diff] [review]
add_missing_barriers_in_createCallObject-v1.diff
Review of attachment 791484 [details] [diff] [review]:
-----------------------------------------------------------------
I don't think this is the right approach. We test whether every created call object is in the nursery, even though in almost all cases we executed inline code which definitely allocated the call object from the nursery. So these tests in general aren't necessary, and doing things this way sets a precedent that would be inconsistent with the ideal of removing post barriers on writes to newly allocated objects when possible.
Currently it looks like we will only allocate from the tenured heap when an OOL call is made to NewCallObject(). Could that function just add the object to the store buffer if it is in the tenured heap? Then it should be correct to do all the initial StoreSlots and StoreFixedSlots to fill in the call object infallibly, as since there has been no intervening allocation the call object is guaranteed to be either in the store buffer or in the nursery.
::: js/src/jit/MIR.h
@@ +7743,5 @@
> }
> };
>
> +// Given a value being written to another object, update the generational store
> +// buffer if the value is in the nursery and object is in the tenured heap.
This comment is wrong.
Attachment #791484 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 4•11 years ago
|
||
Good point! I found this is in the context of bug 889129, where we are allocating into tenured in more cases than just the fallback path. I think in that case, we can just add a test for isLongLived in createCallObject and only fire the barrier if we generate a tenured allocation.
For the other bug here where we bail, I'll go ahead and morph this into the solution you suggested.
Assignee | ||
Comment 5•11 years ago
|
||
And it looks like Gary found this exact issue with fuzzing, so I'm going to dup to the bug with the nice test case.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•