feat: (CM64) context.{set, get} accept i64 immediate

[michael-weigelt]

This is part of a series of PRs to enable verification of 64bit components.. The effort is tracked here.

This PR allows context.{set, get} to accept i64 as well as i32.

[michael-weigelt]

Moving the discussion from there to here:

[michael-weigelt]

adambratschikaye
I think these ones should be malformed instead of invalid because the spec doesn't even permit the construction with other values. I guess this means you need to have a check in the parser itself as well.

michael-weigelt
Ah I wrongly assumed that this is extensible, coming from the resource rep type. Then maybe I should not use ValType anywhere at all. (?)

[alexcrichton]

This is a good question! By the letter of the spec here @adambratschikaye is correct where these tests should be assert_malformed rather than assert_invalid. Additionally following the letter of the spec all usage of ValType would want to get replaced with something else, yeah. (in Wasmtime we have an IndexType for memories/tables which may be another possible name).

cc @lukewagner on this specific point though, how do you feel about this? Should the binary format for context.{get,set} allow any core wasm value type in the binary encoding (and, thus, the text encoding too)? Or should the binary/text formats only allow i32 and i64? I could see us having a desire in the future to allow something like anyref here too for GC-using programs, in which case it'd be easiest to relax and allow any core value type here with validation restricting it to i32 and i64.

[lukewagner]

In the binary format, we use the <valtype> opcodes for i32 and i64 (0x7f and 0x7e) directly in the decoding rule instead of decoding a generic <valtype> and then using validation rules to rule out everything except i32/i64 (just like we do for the (rep i32)/(rep i64) of a resource type). But in the AST, we have (canon context.{get,set} <valtype> ...) and verbiage about validation rejecting everything but i32/i64, so I suppose it's ambiguous. Probably we should align Binary.md with Explainer.md and go the decode+validate route, hence assert_invalid?

Practically speaking, I hope we don't do anyref since it'll raise gross questions of what to do if you, e.g., context.set anyref 0 and then context.get i32 0. With integers we say that they alias like 64-bit registers do on x64, but less clear what the right thing to do with anyref that avoids overhead. My thinking was that, if you wanted to store a reference, you'd store it in a table and then store the i32 index in TLS.

[alexcrichton]

I hadn't thought about the view-typing of slot 0 and slot 1 as different integer widths and that's a good point... Do you think it's feasible that we could guarantee that each slot is viewed as exactly one type in validation? For example within a component context.get 0 could only have one type?

Otherwise though I agree that syncing the text format and Binary.md makes sense, namely by changing Binary.md to use a valtype, which @michael-weigelt would keep this as assert_invalid yeah.

[lukewagner]

Do you think it's feasible that we could guarantee that each slot is viewed as exactly one type in validation?

That's a good point; I suppose we could add a whole-component validation rule to that effect. It'd be the only validation rule like that and usually a rule like that has composability problems (compiling unrelated bits of code that disagree), but given that the use of context.{get,set} requires all the code in the same component to agree on the ABI anyways, I think it'd be be fine in this case. I think I'll work on a CM PR for this and the Binary.md fix.

[alexcrichton]

Makes sense, and sounds good! In that case @michael-weigelt let's keep all the ValType bits as-is in this PR

[alexcrichton]

This is a good question! By the letter of the spec here @adambratschikaye is correct where these tests should be assert_malformed rather than assert_invalid. Additionally following the letter of the spec all usage of ValType would want to get replaced with something else, yeah. (in Wasmtime we have an IndexType for memories/tables which may be another possible name).

cc @lukewagner on this specific point though, how do you feel about this? Should the binary format for context.{get,set} allow any core wasm value type in the binary encoding (and, thus, the text encoding too)? Or should the binary/text formats only allow i32 and i64? I could see us having a desire in the future to allow something like anyref here too for GC-using programs, in which case it'd be easiest to relax and allow any core value type here with validation restricting it to i32 and i64.