[GC] Fix TypeRefining on StructGets without content but with a reachable ref #7138
Conversation
|
(diff is smaller without whitespace) |
test/lit/passes/type-refining.wast
Outdated
| ;; A struct.new, so that we have a field to refine. | ||
| (drop | ||
| (struct.new $optimizable | ||
| (ref.null func) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
How does this affect the rest of the test given that $optimizable and $never are not related types? Is it because the pass returns early if there are no fields to refine, so we wouldn't hit the bug without this? If so, it would be good to mention that explicitly in the comment.
test/lit/passes/type-refining.wast
Outdated
| (func $export (export "export") (result (ref $never)) | ||
| ;; Make the type $never public (if it were private, we'd optimize in a | ||
| ;; different way that avoids the bug that this tests for). |
There was a problem hiding this comment.
It's usually simpler to export a global with the public type.
| // so we should refinalize. However, we will be refinalizing later | ||
| // anyhow in updateTypes, so there is no need. | ||
| if (curr->ref->type.isNull() || newFieldType == Type::unreachable || | ||
| !Type::isSubType(newFieldType, curr->type)) { |
There was a problem hiding this comment.
Do we have a test that tickles this last !Type::isSubType(...) condition? It is surprising that that could ever happen.
There was a problem hiding this comment.
Here is the relevant test:
binaryen/test/lit/passes/type-refining.wast
Line 802 in 729ea41
The test hits an internal assertion without this condition.
|
@tlively PTAL at the last commit: that fixes a bug the fuzzer found. We were not actually noting content for |
2 participants
If we see a StructGet with no content (the type it reads from has no writes)
then we can make it unreachable. The old code literally just changed the type
to unreachable, which would later work out with refinalization - but only if
the StructGet's ref was unreachable. But it is possible for this situation to
occur without that, and if so, this hit the validation error "can't have an
unreachable node without an unreachable child".
To fix this, merge all code paths that handle "impossible" situations, which
simplifies things, and add this situation.