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

Should IndexValueToU64 use ToBigInt64? #85

Open
backes opened this issue Sep 26, 2024 · 5 comments
Open

Should IndexValueToU64 use ToBigInt64? #85

backes opened this issue Sep 26, 2024 · 5 comments

Comments

@backes
Copy link
Member

backes commented Sep 26, 2024

When implementing #75 in V8 I was wondering that the IndexValueToU64 algorithm throws a TypeError if the value is not a BigInt, instead of using the ToBigInt64 algorithm that's also used in ToWebAssemblyValue.

Any reason for this restriction, or should we be consistent there and accept anything that converts to a BigInt64 according to that algorithm (in particular boolean values and string, but notably not Numbers).

One difficulty with switching to ToBigInt64 would be static typing on the IndexValue type. Instead of unsigned long or bigint this would need to be unsigned long or object I guess.

@bvisness
Copy link
Collaborator

bvisness commented Sep 26, 2024

The intent of IndexValueToU64 is to mirror the behavior of [EnforceRange] unsigned long, but for BigInt. Looking again, though, it does seem like we should probably be able to use strings and other values that ToBigInt would allow, which would be a good reason to use ToBigInt.

Putting my spec lawyer hat on, though...

I think the real question is how this interacts with WebIDL. The rules for conversion to WebIDL bigint are laid out here. While it is a bit confusing, I think the third algorithm applies, because [EnforceRange] unsigned long or bigint matches "an IDL numeric type T or bigint". (This is reinforced by point 16 here.)

In our case, the third algorithm will pass BigInt values through unchanged, but will convert all other values to [EnforceRange] unsigned long, before even getting into the body of IndexValueToU64. This would mean that, for example, a String value in u64 range would actually be converted to Number, and [EnforceRange]'d, rather than being passed to ToBigInt.

So that sucks. I think the above explanation is correct from a pedantic point of view, but generally we expect these IndexValue parameters to act like either [EnforceRange] unsigned long or bigint, since we're treating this like an overload.

If we want to stick very pedantically to all the specs, I think we have two options:

  1. Keep the WebIDL [EnforceRange] unsigned long or bigint, and continue explicitly rejecting non-BigInt values.
  2. Replace the WebIDL with any (or something similar) and rebuild the exact semantics we want in IndexValueToU64. (This would require us to recreate the parts of the WebIDL conversion that we want.)

@backes
Copy link
Member Author

backes commented Sep 27, 2024

Thanks for the explanations, that all makes sense. I was not aware of the conversions rules that WebIDL specifies for numeric type or bigint.
I agree with all you said. In particular, we don't really want the semantics of [EnforceRange] unsigned long or bigint, but depending on the index type we want either the conversion rules for [EnforceRange] unsigned long or bigint.

So if (2) is "allowed" to do, then I think this would be the right solution.

@bvisness
Copy link
Collaborator

I don't see why it wouldn't be allowed, but it will no doubt be clumsy to specify. I'll ask some Mozilla folks for their opinion and then try and clean this up next week.

@backes
Copy link
Member Author

backes commented Sep 30, 2024

A colleague had a nice idea for an elegant solution:

  • Use typedef any IndexValue; (or drop that typedef?)
  • Define IndexValueToU64(v, indextype) as ToWebAssemblyValue(v, ToValueType(indextype))

This would be similar to the Global constructor or the Global value setter.

@bvisness
Copy link
Collaborator

bvisness commented Sep 30, 2024

The thing that ToWebAssemblyValue does not capture is [EnforceRange]. ToWebAssemblyValue uses ToInt32 for i32 and ToBigInt64 for i64, both of which do a modulo for out-of-range values instead of throwing TypeError.

At the very least, we need to preserve [EnforceRange] for i32 memories since that is how it is specified today. My goal was to mirror that for i64 memories as well for semantic consistency.

To be very pedantic, there is also the matter of memory limits actually being u64, which is not a value type.

hubot pushed a commit to v8/v8 that referenced this issue Oct 1, 2024
The JS API for WebAssembly.Memory and WebAssembly.Table objects should
accept and return BigInt instead of Number for 64-bit memories and
tables.

It's still being discussed if we want to allow anything that can be
converted to BigInt, or just BigInt itself, see
WebAssembly/memory64#85.
For now, we only accept BigInt as currently spec'ed.

[email protected]

Bug: 358381633
Change-Id: I5741aab5feec2a047320a355e3a5c5a0d4ccf673
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5898572
Reviewed-by: Eva Herencsárová <[email protected]>
Commit-Queue: Clemens Backes <[email protected]>
Cr-Commit-Position: refs/heads/main@{#96362}
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