Fix identifying user instructions in LowerAllocObject

neildhar · facebook-github-bot · commit ffcaf3cd42ea · 2021-06-11T14:50:02.000-07:00
Summary:
`LowerAllocObject` performs a step where it identifies all of the
user instructions of a given `allocInst_` that are `StoreOwnProperty`
instructions, with the intention of merging some of them into a
serialised buffer.

However, this selection is not sufficiently targeted. Specifically,
an allocated object may be used by a `StoreOwnProperty` instruction
that is storing to a different object. Consider:

```
var x = {a: 5, b: 6}
var y = {e:5, f: x, g:6}
```

Here, we generate 3 `StoreOwnProperty` instructions that use `x`, 2
of which set the properties, and the third one assigns `x` to `y.f`.
That last instruction should never be considered for serialisation
into the buffer, since it is completely unrelated to initialising
`x`.

Our current code doesn't check for this, and does shortlist that last
instruction for serialisation. The only reason it doesn't end up
actually serialising it, is because of how it ends up interacting
with `LowerAllocObject::estimateBestNumElemsToSerialize`.
Specifically, because the `StoredValue` of the last instruction is an
object, it is not optimal to include it.

If we just change the return value of
`estimateBestNumElemsToSerialize` to instead be `curSize`, then the
above code generates the following IR:

```
%0 = HBCLoadConstInst 6 : number
%1 = HBCAllocObjectFromBufferInst 3 : number, "e" : string, 5 : number
%2 = HBCAllocObjectFromBufferInst 2 : number, "a" : string, 5 : number, "b" : string, 6 : number, "f" : string, null : null
%3 = StorePropertyInst %2 : object, %1 : object, "f" : string
%4 = StoreNewOwnPropertyInst %0 : number, %1 : object, "g" : string, true : boolean
```

In that, we can see that `x` now incorrectly has an `f` property.
This also breaks populating the `f` property on `y`, because it
does not emit the placeholder for `f`, which means that we are
not guaranteed that `f` is stored as an own property of `y`.

Less importantly this also blocks `y` from being optimised into a
serialised buffer, because the lowering of `x` changed the
`StoreOwnPropertyInst` into a `StorePropertyInst`.

Theoretically, there could be well formed IR that can cause this to
generate much more incorrect code. However, since we only generate
`StoreOwnProperty` from object and array literals, I couldn't think
of a way to generate an example to demonstrate this.

Reviewed By: tmikov

Differential Revision: D29002692

fbshipit-source-id: dd18b98a35fe527aa3470c0f950852abf29ebc81
diff --git a/lib/BCGen/Lowering.cpp b/lib/BCGen/Lowering.cpp
@@ -250,9 +250,9 @@ LowerAllocObjectFuncContext::collectInstructions() const {
         continue;
       }
       auto *SI = llvh::dyn_cast<StoreOwnPropertyInst>(&I);
-      if (!SI) {
-        // A user that's not a StoreOwnPropertyInst. We have to
-        // stop processing here.
+      if (!SI || SI->getObject() != allocInst_) {
+        // A user that's not a StoreOwnPropertyInst storing into the object
+        // created by allocInst_. We have to stop processing here.
         terminate = true;
         break;
       }

Original file line number	Diff line number	Diff line change
`@@ -250,9 +250,9 @@ LowerAllocObjectFuncContext::collectInstructions() const {`
`250`	`250`	`continue;`
`251`	`251`	`}`
`252`	`252`	`auto *SI = llvh::dyn_cast<StoreOwnPropertyInst>(&I);`
`253`		`- if (!SI) {`
`254`		`- // A user that's not a StoreOwnPropertyInst. We have to`
`255`		`- // stop processing here.`
	`253`	`+ if (!SI \|\| SI->getObject() != allocInst_) {`
	`254`	`+ // A user that's not a StoreOwnPropertyInst storing into the object`
	`255`	`+ // created by allocInst_. We have to stop processing here.`
`256`	`256`	`terminate = true;`
`257`	`257`	`break;`
`258`	`258`	`}`