SignaturePruning: Properly handle public types #6630
Conversation
| ;; CHECK: (rec | ||
| ;; CHECK-NEXT: (type $none (func)) | ||
| (type $none (func)) | ||
| ;; CHECK: (type $much (func (param i32))) |
There was a problem hiding this comment.
It looks like the type name is repeated in the output, which we shouldn't allow.
There was a problem hiding this comment.
Yeah, that's a quirk we have atm. It's actually useful in cases where the old and new types remain in use (like here), but we should probably ensure a new unique name. I can look into that separately.
There was a problem hiding this comment.
I would expect us to have problems with text round trip fuzzing before this is fixed.
There was a problem hiding this comment.
I don't see issues in fuzzing so far, but this is a pre-existing issue, so that isn't surprising. It must take specific fuzzer luck to get it.
There was a problem hiding this comment.
Experimenting with a patch that deduplicates the names, the problem is that it will generate a large diff on existing tests (before this PR; unrelated) regardless of whether we deduplicate the old or the new name. We seem to just have enough cases that both remain in use. The diff one way is 66 K and the other is 276 K, so at least one is clearly less annoying, but it will still be a quite large change unfortunately.
Co-authored-by: Thomas Lively <[email protected]>
…6642) We need StringLowering to modify even public types, as it must replace every single stringref with externref, even if that modifies the ABI. To achieve that we told it that all string-using types were private, which let TypeUpdater update them, but the problem is that it moves all private types to a new single rec group, which meant public and private types ended up in the same group. As a result, a single public type would make it all public, preventing optimizations and breaking things as in #6630 #6640. Ideally TypeUpdater would modify public types while keeping them in the same rec groups, but this may be a very specific issue for StringLowering, and that might be a lot of work. Instead, just make StringLowering handle public types of functions in a manual way, which is simple and should handle all cases that matter in practice, at least in J2Wasm.
The SignaturePruning pass optimizes away parameters that it proves are safe to
remove. It turns out that that does not always match the definition of private
types, which is more restrictive. Specifically, if say all the types are in one big
rec group and one of them is used on an exported function then all of them are
considered public (as the rec group is). However, in closed world, it would be ok
to leave that rec group unchanged but to create a pruned version of that type
and use it, in cases where we see it is safe to remove a parameter. (See the
testcase in the PR for a concrete example.)
To put it another way, SignaturePruning already proves that a parameter is
safe to remove in all the ways that matter. Before this PR, however, the testcase
in this PR would error - so this PR is not an optimization but a bugfix, really -
because SignaturePruning would see that a parameter is safe to remove but
then TypeUpdating would see the type is public and so it would leave it alone,
leading to a broken module.
This situation is in fact not that rare, and happens on real-world Java code.
The reason we did not notice it before is that typically there are no remaining
SignaturePruning opportunities late in the process (when other closed world
optimizations have typically led to a single big rec group).
The concrete fix here is to add
additionalPrivateTypesto a few more placesin TypeUpdating. We already supported that for cases where a pass knew
better than the general logic what can be modified, and this adds that
ability to the signature-rewriting logic there. Then SignaturePruning can
send in all the types it has proven are safe to modify.
additionalPrivateTypesif the typeis not already in our list (or we'd end up with duplicates in the final rec
group).
newSignaturesin SignaturePruning out of the top level, whichwas confusing (the pass has multiple iterations, and we want each to have
a fresh instance).