Bug 181177 – CheckStructure can be incorrectly subsumed by CheckStructureOrEmpty

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

The semantics of CheckStructure are such that it does not allow the empty value to flow through it. However, we may eliminate a CheckStructure if it's preceded by a CheckStructureOrEmpty. This doesn't have semantic consequences when validation is turned off. However,  with validation on, this trips up our OSR exit machinery that says when an exit could happen.

An IR example is as such:

a: GetClosureVar // Or any other node that produces BytecodeTop
b: CheckStructure(Cell:@a, {s1})
...
c: CheckStructure(Cell:@a, {s2})
d: PutByOffset(KnownCell:@a, KnownCell:@a, @value)

However, in the TypeCheckHoistingPhase, we may insert CheckStructureOrEmptys like this:
a: GetClosureVar
e: CheckStructureOrEmpty(@a, {s1})
b: CheckStructure(Cell:@a, {s1})
...
f: CheckStructureOrEmpty(@a, {s2})
c: CheckStructure(Cell:@a, {s2})
d: PutByOffset(KnownCell:@a, KnownCell:@a, @value)

This will cause constant folding to change the IR to:
a: GetClosureVar
e: CheckStructureOrEmpty(@a, {s1})
...
f: CheckStructureOrEmpty(@a, {s2})
d: PutByOffset(KnownCell:@a, KnownCell:@a, @value)

The only value that could theoretically flow through the two CheckStructureOrEmtpy is the empty value. However, when we filter values based on edge types, KnownCell only allows SpecCell to flow through, not SpecCellCheck. Given the original IR, this is a correct assumption. KnownCell is correct because CheckStructure wouldn't allow the empty value to flow through (it would crash). (Note: when the above IR actually runs, it will OSR exit.)

I think a way to solve this IR wart is to introduce an AssertNotEmpty node. There are probably other ways, but this seems the cleanest to me.

Note: in this IR example, the GetClosureVar would not actually produce a TDZ value. If it did happen to produce the TDZ value, based on the assumption that we emit correct bytecode, before the PutByOffset, the bytecode would have a TDZ check node. Therefore, the DFG IR would have also had a CheckNotEmpty node prior to the PutByOffset.

[Saam Barati]

<rdar://problem/36205704>

[Filip Pizlo]

I think that I agree, AssertNotEmpty is the way to go.

[Saam Barati]

Created attachment 330225 [details]
patch

[Saam Barati]

(In reply to Filip Pizlo from comment #2)
> I think that I agree, AssertNotEmpty is the way to go.

Some other ideas I had just for posterity:

- KnownCell filters SpecCellCheck instead of SpecCell. I don't like this because it weakens what we mean by KnownCell
- Weaken what we do when validation is enabled in the DFG. Right now, if mayExit tells us we won't exit, we make sure that's true. This seems like a nice property to ensure.

[Saam Barati]

ping review

[Yusuke Suzuki]

Comment on attachment 330225 [details]
patch

Looks nice to me.

[WebKit Commit Bot]

Comment on attachment 330225 [details]
patch

Clearing flags on attachment: 330225

Committed r226907: <https://trac.webkit.org/changeset/226907>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.