feat: (CM64) Make realloc signature match the memory's address type#2501
feat: (CM64) Make realloc signature match the memory's address type#2501alexcrichton merged 4 commits intobytecodealliance:mainfrom
realloc signature match the memory's address type#2501Conversation
| let memory_idx = memory.ok_or(BinaryReaderError::new( | ||
| "canonical option `realloc` requires the `memory` option", | ||
| offset, | ||
| ))?; |
There was a problem hiding this comment.
This'll want to defer validation of the signature until after all canonical options have been processed because the realloc option isn't required to show up after the memory option (there's no implicit orderings). What this can do, however, is to require, at the end, that if both realloc and memory are present then there's the correct signature.
To preserve the preexisting behavior of specifying realloc and no memory I think we'll have to continue to validate that it has the i32-based signature, but we should probably make it a first-class spec-level error to specify realloc with memory or basically consider this situation in isolation.
There was a problem hiding this comment.
Paragraph 1: Will do.
To preserve the preexisting behavior of specifying realloc and no memory
Do I understand you correctly that for the present, we must remain backwards compatible, but that you are suggesting to change the spec and move forward at some point? If so, we should gate this new bullet point behind the elephant emoji:
- if
reallocis present thenmemorymust be present
Or we remove it altogether for the present.
There was a problem hiding this comment.
Or, if you can point me in the right direction, I can try to
make it a first-class spec-level error to specify realloc with[out] memory
There was a problem hiding this comment.
Hm ok yeah good point. That bullet was actually added by me in WebAssembly/component-model#467 as well, but I botched the implementation in wasm-tools. The implementation in wasm-tools is such that if realloc is required, then memory is also required. The implement needs to be if realloc is specified, then memory is required, however.
Could you file an issue for this and tag this handle-memory-is-None with a fixme pointing to that issue? This PR preserves the preexisting behavior, which is good, but I'll need to sit down and review to see if this is something we can "just change" or whether it's a problem in practice and needs to change more.
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
In the meantime this all looks good so I'll merge
2 participants
This is part of a series of PRs to enable verification of 64bit component model modules. The effort is tracked in #2500.
This PR validates that the
realloccanonopt signature matches thememorycanonopt address type.