Handle sharedness in Heap2Local's Array2Struct

[tlively]

Heap2Local uses an internal utility called Array2Struct to turn non-escaping array allocations into struct allocations so they can subsequently be optimized by the Heap2Local struct optimizations. It
previously used a non-shared struct type, even for shared array types, but this could cause problems when the non-shared struct type became a non-shared null in places that required a shared type to
validate. This could specifically occur when the allocation flowed into a StructCmpxchg expected field that needed to be a subtype of shared eqref.

As a drive-by to make the regression test parse correctly, fix child-typer for StructCmpxchg to correctly handle sharedness in the expected field as well.

[aheejin]

I'm not familiar with this pass (#3866 said I reviewed it, which I don't have any memory of... 🫠)

This could specifically occur when the allocation flowed into a StructCmpxchg expected field that needed to be a subtype of shared eqref.

Why is the expected operand in StructCmpxchg special?

As a drive-by to make the regression test parse correctly, fix child-typer for StructCmpxchg to correctly handle sharedness in the expected field as well.

How is this fix related to the Heap2Local fix? What happens if this child-typer is not fixed here?

[aheejin]

This is preexisting, but why does this have to be Nullable?

[tlively]

When the type of the accessed struct field is a reference, the expected operand is allowed to be any subtype of (ref null eq) (i.e. eqref), or (ref null (shared eq)) if the field type is a shared reference. In either case, the expected operand can be nullable.

[aheejin]

I get that Heap2Local can extract fields like i32s out of a struct if that struct doesn't escape, but that would just extract the fields into locals and wouldn't create a random i32 values (like 0) to set them, right? Why can we replace (array.new_fixed $array 0) with (ref.null (shared none)) here? Where did this value none come from?

[tlively]

The way Heap2Local works in general is that it places struct fields in locals and also replaces the struct itself with a null value. This generally makes it easier to update the IR than it would be if we removed the value entirely. For example, in this test it would not be valid to leave the expected operand with type none, so we would have had to fix up the struct.atomic.rmw.cmpxchg to maintain validity even though it is unreachable. But making the expected operand null is still valid, so we can less work to fix up the IR.

[aheejin]

Why is it OK to create a value (ref.null (shared none) here) out of thin air? It wouldn't be OK to arbitrarly assign 0 for i32s, no? What if we get to use the value? In this case I guess that's not the case because the third operand is unreachable, but that's not always the case.

Also I thought this pass creates locals for each field. Why do we not create a local here?
Sorry, maybe I should read the pass..

[tlively]

It's ok to create the null out of thin air because we know it cannot escape the function (otherwise we would not be optimizing the allocation we are replacing with the null), and we know we will update all the expressions that the value flows to so that they no longer use it.

We don't create a local here because the allocated array has size zero, so there are no elements to put into locals.

[tlively]

This could specifically occur when the allocation flowed into a StructCmpxchg expected field that needed to be a subtype of shared eqref.

Why is the expected operand in StructCmpxchg special?

StructCmpxchg is special because it can take 3 reference operands (ref, expected, and replacement), which is more than most other instructions. That allows for more complex combinations of operands being optimized or not or being unreachable or not than are possible for other types. This particular bug depends on ref having a struct type but not being optimizable, expected having an array type and being optimizable, and replacement being unreachable so that the cmpxchg parent is unreachable and therefore not optimized by this pass.

As a drive-by to make the regression test parse correctly, fix child-typer for StructCmpxchg to correctly handle sharedness in the expected field as well.

How is this fix related to the Heap2Local fix? What happens if this child-typer is not fixed here?

The original fuzzer test case depending on running --gufa before --heap2local to set up the IR that triggers the bug. But for checked-in regression test, we want to be able to parse the Wasm text directly into the IR that triggers the bug. Without the fix to child-typer, IRBuilder sees the unreachable replacement and thinks that preceding expected value has an invalid type, so it parses the text into this IR instead:

(drop
 (local.get $shared-struct)
)
(drop
 (struct.new_default $shared-struct)
)
(struct.atomic.rmw.cmpxchg <unprintable type> 0
 (unreachable)
 (unreachable)
 (unreachable)
)

But this IR does not trigger the bug.

[aheejin]

StructCmpxchg is special because it can take 3 reference operands (ref, expected, and replacement), which is more than most other instructions. That allows for more complex combinations of operands being optimized or not or being unreachable or not than are possible for other types. This particular bug depends on ref having a struct type but not being optimizable, expected having an array type and being optimizable, and replacement being unreachable so that the cmpxchg parent is unreachable and therefore not optimized by this pass.

Why is this particular condition necessary?
What happens if this field in question of being shared or not is not expected but ref or replacement?

The original fuzzer test case depending on running --gufa before --heap2local to set up the IR that triggers the bug. But for checked-in regression test, we want to be able to parse the Wasm text directly into the IR that triggers the bug. Without the fix to child-typer, IRBuilder sees the unreachable replacement and thinks that preceding expected value has an invalid type, so it parses the text into this IR instead:

(drop
 (local.get $shared-struct)
)
(drop
 (struct.new_default $shared-struct)
)
(struct.atomic.rmw.cmpxchg <unprintable type> 0
 (unreachable)
 (unreachable)
 (unreachable)
)

But this IR does not trigger the bug.

We don't reject the invalid IR, and just make them unreachables? Why? Was the parser this way before, or is this a new behavior of the new parser?

[tlively]

StructCmpxchg is special because it can take 3 reference operands (ref, expected, and replacement), which is more than most other instructions. That allows for more complex combinations of operands being optimized or not or being unreachable or not than are possible for other types. This particular bug depends on ref having a struct type but not being optimizable, expected having an array type and being optimizable, and replacement being unreachable so that the cmpxchg parent is unreachable and therefore not optimized by this pass.

Why is this particular condition necessary? What happens if this field in question of being shared or not is not expected but ref or replacement?

replacement being unreachable is important because that makes the cmpxchg unreachable, which means that the pass returns early and does not try to optimize it, leaving the cmpxchg in the output. ref having a struct type (with a shared field) sets up the validation requirement that expected needs to be a subtype of (ref null (shared eq)). This is obviously true in the input, but it needs to be true in the output as well. expected being optimizable and having an array type in the input triggers the bug and means that it had an unshared reference type in the output. All these elements came together to create a validation failure in the pass output before the fix.

If the shared array reference flowed in to ref and became an unshared null reference, that would not cause a validation failure because then we would have no further constraints on the types of expected or replacement. If the shared array reference flowed into replacement, OTOH, we might be able to catch the same bug by making expected unreachable instead. But I don't think we need to test it both ways.

We don't reject the invalid IR, and just make them unreachables? Why? Was the parser this way before, or is this a new behavior of the new parser?

This is a new behavior of the new parser. The old parser would have parsed invalid IR, but the new parser does extra work to parse this into valid IR because it is valid WebAssembly. This is good in general, but means bugs in the parser can prevent us from parsing IR we need for specific tests involving unreachability.