Skip to content

[Strings] Make string a subtype of ext, not any #7373

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 31 commits into from
Mar 20, 2025

Conversation

kripken
Copy link
Member

@kripken kripken commented Mar 13, 2025

StringLowering converts strings to externs, which makes sense as we lower
stringrefs to imported JS strings. For the reverse transform it is convenient
to just have strings be subtypes of ext, see #7370 - that makes it simple to
switch stringref to externref and vice versa.

@kripken kripken requested a review from tlively March 13, 2025 23:51
Comment on lines 449 to 451
// Bottom types already handled (including string as the switch value,
// which implies the other value is bottom, which again would have been
// handled).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the addition to this comment. string isn't a bottom type, so why is it included in the bottom types already having been handled?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 399-400 swap to get the lower type as a. If a is string, then b is higher than a, which means (since string is the last of the non-bottom types in the list) that b is a bottom type. But we already handled bottom types before on 392-396.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but I don't get that from reading the comment. Is there something special about string here that doesn't also apply to other types?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just that strings are at the end (right before all the bottom types). I can add that to the comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment improved.

@@ -949,8 +956,9 @@ std::optional<HeapType> HeapType::getSuperType() const {
case none:
case exn:
case noexn:
case string:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes! This doesn't look like it was correct before, since it should have returned any. Do you know how this wasn't caught by tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test was just wrong, but it agreed with the code 😄

See line 1683 in the gtest modified in this PR.

Comment on lines -491 to -493
(any.convert_extern
(extern.convert_any
(string.const "string")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might still be useful to test any.convert_extern of a string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow - convert it from what to what?

In the new model a string is an externref like a JS object is an externref: it doesn't have a wasm representation under any.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any.convert_extern and extern.convert_any are infallible; all any and extern values, including strings, can be converted to the other representation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what is the "other representation" of string? I.e. what are you saying that any.convert_extern of a string should return - what actual type?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an anyref. There's no more specific type and trying to cast it to anything else will fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I guess we can leave it for later. I need to update the interpretation of casts to handle exact types at some point anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fuzzer hits this, so we do need to resolve the matter in this PR somehow. I'm not sure it is easy to work around it in the fuzzer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already support Literals with type externref that can be unwrapped to access the underlying any value. This is the same thing in reverse.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do/should exact casts to externref work on such values?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but if they do for now, it might be ok since there are no real engines to compare such executions to.

Comment on lines +239 to +240
(ref.null noextern) ;; The change from stringref to externref does not
;; cause problems here, nothing needs to change.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this unlock any potential code simplifications in StringLowering.cpp?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered this too, but the answer is actually no. While we are doing something much simpler, logically - swapping a type for a supertype, rather than an entirely unrelated type - the actual mechanical change is identical. We just swap all appearances of type X for Y, and don't need to handle any subtyping.

@kripken
Copy link
Member Author

kripken commented Mar 18, 2025

We can no longer fuzz strings in v8, as we represent them in a nonstandard way, so I disabled that.

@kripken
Copy link
Member Author

kripken commented Mar 18, 2025

I added a readme mention with references to the relevant proposals in the last commit.

@kripken
Copy link
Member Author

kripken commented Mar 18, 2025

@tlively I am going to fix this fuzztest error:

https://github.com/WebAssembly/binaryen/actions/runs/13933139917/job/38994921125?pr=7373

but only because luckily it seems obvious what the issue is. In general, how are people intended to debug errors like that?

@tlively
Copy link
Member

tlively commented Mar 19, 2025

FUZZTEST_PRNG_SEED=(whatever was printed in the failing log) ./bin/binaryen-unittest --gtest_filter=TypeFuzzTest.TestHeapTypeRelationsFuzz should deterministically reproduce the failure.

@tlively
Copy link
Member

tlively commented Mar 19, 2025

The fact that this shows up in CI at all is a nice property of FuzzTest over our other fuzzer patterns. (Although of course we could run the other fuzzers for limited time on CI as well.)

@kripken
Copy link
Member Author

kripken commented Mar 19, 2025

Thanks, now I see it... it was separated from the error itself, for some reason:

FUZZTEST_PRNG_SEED=8gPJDq0F9ObaC0X1WRajSUh_YPiNJkLQ3mvZBc8xll8
[..50 lines of stuff..]
5: Heap type has an invalid supertype
binaryen-unittests: /home/runner/work/binaryen/binaryen/test/gtest/type-domains.cpp:1048: std::vector<HeapType> wasm::(anonymous namespace)::BuildHeapTypes(TypeBuilderPlan): Assertion `built' failed.

=================================================================
=== SETUP FAILURE!

/home/runner/work/binaryen/binaryen/test/gtest/type-builder.cpp:1170: There was a problem with TypeFuzzTest.TestHeapTypeRelationsFuzz.
=================================================================

@kripken
Copy link
Member Author

kripken commented Mar 19, 2025

I added commits for fuzztest, and the last commit adds hacks to allow converting string to anyref and back.

@kripken
Copy link
Member Author

kripken commented Mar 20, 2025

Fuzzer looks happy now.

@kripken kripken merged commit 087c613 into WebAssembly:main Mar 20, 2025
15 checks passed
@kripken kripken deleted the string.is.sub.of.extern branch March 20, 2025 16:49
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