Skip to content
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

Emitting unreachable for unemittable String instructions can fail due to blocks #6443

Open
kripken opened this issue Mar 26, 2024 · 2 comments

Comments

@kripken
Copy link
Member

kripken commented Mar 26, 2024

#6415 solves most of the problem (see details there), but an exception is this:

(string.new_wtf16_array
 (ref.as_non_null
  (block (result (ref null $array))
   (ref.as_non_null
    (ref.null none)
   )
  )
 )
 (i32.const 0)
 (i32.const 0)
)

The block upcasts the type from a bottom type to some specific array type. However, in our binary emitter we do not emit blocks without names (since they have no branches to them), and without that block, we have a bottom type - but the logic in #6415 does not see that, as it scans the IR and not what we emit.

We could perhaps fix this by emitting blocks without names if they upcast types. That should not affect optimized builds, but it could affect unoptimized ones. Alternatively, we could scan what is actually emitted rather than the IR, but that would take some significant refactoring to track what we emit. We could also just do getFallthrough to look through such blocks, at the cost of extra work. However, if the stringref spec is changed/fixed then we don't need any of that.

@kripken
Copy link
Member Author

kripken commented Mar 26, 2024

Another option for now might be to ignore the V8 error, which looks like this:

string.new_wtf16_array[0] expected array of i16, found ref.as_non_null of type (ref none)

but that might be risky to do.

@tlively
Copy link
Member

tlively commented Mar 26, 2024

Another option would be to unconditionally emit a cast to the expected type before string.new_wtf16_array, essentially adding our own type annotation. That adds code size, of course, but stringref is EOL so that seems fine to me.

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

No branches or pull requests

2 participants