906128 - Add missing post barriers in createCallObject

Status: RESOLVED DUPLICATE of bug 906241

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: add_missing_barriers_in_createCallObject-v0.diff

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.

Comment 1

[Terrence Cole [:terrence]]

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.

Comment 2

[Terrence Cole [:terrence]]

Attachment: add_missing_barriers_in_createCallObject-v1.diff

This is substantially more code, but doesn't regress.

Comment 3

[Brian Hackett [Laid off!]]

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.

Comment 4

[Terrence Cole [:terrence]]

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.

Comment 5

[Terrence Cole [:terrence]]

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.