Skip to content

Conversation

@kripken
Copy link
Member

@kripken kripken commented Mar 22, 2024

The only StringEncode we support is the one that writes into an array, so it
has the same effects as ArrayCopy. Precompute needs to be made aware of
such side effects in a manual manner (as we already do for ArrayCopy etc.):
it simply tries to execute code in the interpreter, and if it succeeds it replaces;
it does not check for side effects (checking for side effects would prevent
optimizing cases where the side effects do not happen, as we check them
statically, e.g. dividing by a non-zero constant does not trap but a division
would be seen as having a potential trap effect).

I believe @tlively was worried about this before 😄 I misremembered how
this pass works, so I did not realize that worry was well-founded. Apologies!

This is somewhat brittle and needs careful thought for the few operations like
ArrayCopy/StringEncode that have that form of effects, so perhaps it is
worth thinking about how to refactor the pass to avoid it, but I can't see an
easy way atm. (I did verify no other string operation is hit by this: all the others
emit or operate on immutable strings; it is just StringEncode that is basically
an Array operation that appears in the Strings proposal.)

@kripken kripken requested a review from tlively March 22, 2024 20:02
@tlively
Copy link
Member

tlively commented Mar 22, 2024

Was the fuzzer able to find this bug?

@kripken
Copy link
Member Author

kripken commented Mar 22, 2024

No, the fuzzer was not lucky enough to find it. Just bad luck I think as we do have testcases that are close enough, e.g.

(func $encode (export "encode")

But the encode operation's output is logged there... so the fuzzer would have had to drop it (so that it looks like it can be removed). I don't think we do that kind of operation, to replace the parent but keep the child. We should probably do it, at least for drops it is trivial, I'll look into it.

@kripken kripken enabled auto-merge (squash) March 22, 2024 20:20
@kripken kripken merged commit f40e90a into WebAssembly:main Mar 22, 2024
@kripken kripken deleted the precompute.encode branch March 22, 2024 20:46
@gkdn gkdn mentioned this pull request Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants