[Wasm GC] Add support for non-nullable locals in binary reading #3955
Conversation
|
@kripken, what would you think about adding a pass in the binary parser to seek out and parse the target features section before parsing anything else? We're already loading full binaries into memory before parsing them, so I don't think this would have much of a performance impact. Somewhat separate from this particular PR but on the topic of applying features: The current model is that the command line feature flags, if they're supplied, must explicitly enable a superset of the features enabled by the target features section. This is annoying because enabling one additional feature on the command line requires either providing all the flags for the features in the target features section as well or providing the strange I would be open to simplifying this to a model where the command line feature flags are applied on top of and in addition to the target feature section, rather than having to explicitly be a superset. We could emit a warning whenever the flags would try to disable a feature present in the target features section. |
|
Pre-parsing the features section might not be that bad. Are you saying that if we pre-parse it, then we can set the features once in that pre-parsing, and not once before and once after? Another option here is for The simplification you suggest in feature handling sgtm. Would that help here? |
Hmm, I was thinking that if we pre-parse the features section, then in this PR you wouldn't need to apply the command line features before the parse to have the correct features enabled. But that won't be sufficient if the target features section is absent (for example when parsing a text module) or if it is present but does not contain the relevant feature needed for the parse.
Actually, something slightly different would help here. We could apply the features from the command line just once before parsing the module and let the target features section add additional features on top of that (probably before parsing the rest of the module if the enabled features can affect parsing). Then we would emit a warning whenever the target features section disables a feature that was enabled by the command line flags (which is the opposite of what I suggested in my previous comment). |
|
Interesting idea @tlively , that does sound like it could be nice and simple. I'll work on a patch to see. |
As suggested in #3955 (comment) This applies commandline features first. If the features section is present, and disallows some of them, then we warn. Otherwise, the features can combine (for example, a wasm may enable feature X because it has to use it, and a user can simply add the flag for feature Y if they want the optimizer to try to use it; both flags will then be enabled). This is important because in some cases we need to know the features before parsing the wasm, in the case that the wasm does not use the features section. In particular, non-nullable GC locals have an effect during parsing. (Typed function references also does, but we found a way to apply its effect all the time, that is, always use the refined type, and that happened to not break the case where the feature is disabled - but such a workaround is not possible with non-nullable locals.) To make this less error-prone, add a FeatureSet input as a parameter to WasmBinaryBuilder. That is, when building a module, we must give it the features to use while doing so. This will unblock #3955 . That PR will also add a test for the actual usage of a feature during loading (the test can only be added there, after that PR unbreaks things).
|
This PR has been rebased on top of #3960 and is now very simple! |
We only tested that feature on the text format. For binary support, the reader needs
to know that the feature is enabled.
@tlively This sadly gets back to the issue of when to apply the features. This applies
them before and after, which works, but I think we agreed before is not great. With
typed-function-references we found a workaround (always use the refined type), but
I can't think of such a thing here that is not annoying. Thoughts?
Fixes #3953
@askeksa-google This PR may be useful while you wait.