Stream: git-wasmtime

Topic: wasmtime / PR #7974 winch: Overhaul the internal ABI

Wasmtime GitHub notifications bot (Feb 21 2024 at 16:04):

saulecabrera opened PR #7974 from saulecabrera:winch-overhaul-abi to bytecodealliance:main:

This change overhauls Winch's ABI. This means that as part of this change, the default ABI now closely resembles Cranelift's ABI, particularly on the treatment of the VMContext. This change also fixes many wrong assumptions about trampolines, which are tied to how the previous ABI operated.

The main motivation behind this change is:

• To make it easier to integrate Winch-generated functions with Wasmtime
• Fix fuzz bugs related to imports
• Solidify the implementation regarding the usage of a pinned register to hold the VMContext value throughout the lifetime of a function.

The previous implementation had the following characteristics, and wrong assumptions):

• Assumed that internal functions don't receive a caller or callee VMContexts as parameters.

• Worked correctly in the following scenarios:
  * Wasm -> Native: since we can explicitly load the caller and callee VMContext, because we're calling a native import.
  * (Native, Array) -> Wasm: because the native signatures define a tuple of VMContext as arguments.

• It didn't work in the following scenario:

  • Wasm->Wasm: When calling imports from another WebAssembly instance (via direct call or call_indirect. The previous implementation wrongly assumes that there should be a trampoline in this case, but there isn't. The code was generated by the same compiler, so the same ABI should be used in both functions, but it doesn't.

This change introduces the following changes, which fix the previous assumptions and bugs:

• All internal functions declare a two extra pointer-sized parameters, which will hold the callee and caller VMContexts
• Use a pinned register that will be considered live through the lifetime of the function instead of pinning it at the trampoline level. The pinning explicitlly happens when entering the function body and no other assumptions are made from there on.
• Introduce the concept of special ContextArgs for function calls. This enum holds metadata about which context arguments are needed depending on the callee. The previous implementation of introducing register values at arbitrary locations in the value stack conflicts with the stack ordering principle which states that older values must always precede newer values. So we can't insert a register, because if a spill happens the order of the values will be wrong.

Finally, given that this change also enables the imports.wast test suite, it also includes a fix to global.{get, set} instructions which didn't account entirely for imported globals.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Feb 21 2024 at 16:04):

saulecabrera requested wasmtime-default-reviewers for a review on PR #7974.

Wasmtime GitHub notifications bot (Feb 21 2024 at 16:04):

saulecabrera requested cfallin for a review on PR #7974.

Wasmtime GitHub notifications bot (Feb 21 2024 at 16:04):

saulecabrera requested wasmtime-compiler-reviewers for a review on PR #7974.

Wasmtime GitHub notifications bot (Feb 21 2024 at 16:07):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 21 2024 at 16:07):

saulecabrera created PR review comment:

This change is fairly large already, so I opted to just introduce this macro and use it in a couple of places that are updated by this change. My plan is to follow up with a general replace after landing this change (but I can do so here too if preferred).

Wasmtime GitHub notifications bot (Feb 21 2024 at 16:09):

saulecabrera edited PR #7974:

This change overhauls Winch's ABI. This means that as part of this change, the default ABI now closely resembles Cranelift's ABI, particularly on the treatment of the VMContext. This change also fixes many wrong assumptions about trampolines, which are tied to how the previous ABI operated.

The main motivation behind this change is:

• To make it easier to integrate Winch-generated functions with Wasmtime
• Fix fuzz bugs related to imports
• Solidify the implementation regarding the usage of a pinned register to hold the VMContext value throughout the lifetime of a function.

The previous implementation had the following characteristics, and wrong assumptions):

• Assumed that internal functions don't receive a caller or callee VMContexts as parameters.

• Worked correctly in the following scenarios:
  * Wasm -> Native: since we can explicitly load the caller and callee VMContext, because we're calling a native import.
  * (Native, Array) -> Wasm: because the native signatures define a tuple of VMContext as arguments.

• It didn't work in the following scenario:

  • Wasm->Wasm: When calling imports from another WebAssembly instance (via direct call or call_indirect. The previous implementation wrongly assumed that there should be a trampoline in this case, but there isn't. The code was generated by the same compiler, so the same ABI should be used in both functions.

This change introduces the following changes, which fix the previous assumptions and bugs:

• All internal functions declare a two extra pointer-sized parameters, which will hold the callee and caller VMContexts
• Use a pinned register that will be considered live through the lifetime of the function instead of pinning it at the trampoline level. The pinning explicitlly happens when entering the function body and no other assumptions are made from there on.
• Introduce the concept of special ContextArgs for function calls. This enum holds metadata about which context arguments are needed depending on the callee. The previous implementation of introducing register values at arbitrary locations in the value stack conflicts with the stack ordering principle which states that older values must always precede newer values. So we can't insert a register, because if a spill happens the order of the values will be wrong.

Finally, given that this change also enables the imports.wast test suite, it also includes a fix to global.{get, set} instructions which didn't account entirely for imported globals.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Feb 21 2024 at 16:25):

saulecabrera edited PR #7974:

This change overhauls Winch's ABI. This means that as part of this change, the default ABI now closely resembles Cranelift's ABI, particularly on the treatment of the VMContext. This change also fixes many wrong assumptions about trampolines, which are tied to how the previous ABI operated.

The main motivation behind this change is:

• To make it easier to integrate Winch-generated functions with Wasmtime
• Fix fuzz bugs related to imports
• Solidify the implementation regarding the usage of a pinned register to hold the VMContext value throughout the lifetime of a function.

The previous implementation had the following characteristics, and wrong assumptions):

• Assumed that internal functions don't receive a caller or callee VMContexts as parameters.
• Worked correctly in the following scenarios:
  * Wasm -> Native: since we can explicitly load the caller and callee VMContext, because we're calling a native import.
  * (Native, Array) -> Wasm: because the native signatures define a tuple of VMContext as arguments.
• It didn't work in the following scenario:
  • Wasm->Wasm: When calling imports from another WebAssembly instance (via direct call or call_indirect. The previous implementation wrongly assumed that there should be a trampoline in this case, but there isn't. The code was generated by the same compiler, so the same ABI should be used in both functions.

This change introduces the following changes, which fix the previous assumptions and bugs:

• All internal functions declare a two extra pointer-sized parameters, which will hold the callee and caller VMContexts
• Use a pinned register that will be considered live through the lifetime of the function instead of pinning it at the trampoline level. The pinning explicitlly happens when entering the function body and no other assumptions are made from there on.
• Introduce the concept of special ContextArgs for function calls. This enum holds metadata about which context arguments are needed depending on the callee. The previous implementation of introducing register values at arbitrary locations in the value stack conflicts with the stack ordering principle which states that older values must always precede newer values. So we can't insert a register, because if a spill happens the order of the values will be wrong.

Finally, given that this change also enables the imports.wast test suite, it also includes a fix to global.{get, set} instructions which didn't account entirely for imported globals.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Feb 21 2024 at 16:25):

saulecabrera edited PR #7974:

This change overhauls Winch's ABI. This means that as part of this change, the default ABI now closely resembles Cranelift's ABI, particularly on the treatment of the VMContext. This change also fixes many wrong assumptions about trampolines, which are tied to how the previous ABI operated.

The main motivation behind this change is:

• To make it easier to integrate Winch-generated functions with Wasmtime
• Fix fuzz bugs related to imports
• Solidify the implementation regarding the usage of a pinned register to hold the VMContext value throughout the lifetime of a function.

The previous implementation had the following characteristics, and wrong assumptions:

• Assumed that internal functions don't receive a caller or callee VMContexts as parameters.
• Worked correctly in the following scenarios:
  * Wasm -> Native: since we can explicitly load the caller and callee VMContext, because we're calling a native import.
  * (Native, Array) -> Wasm: because the native signatures define a tuple of VMContext as arguments.
• It didn't work in the following scenario:
  • Wasm->Wasm: When calling imports from another WebAssembly instance (via direct call or call_indirect. The previous implementation wrongly assumed that there should be a trampoline in this case, but there isn't. The code was generated by the same compiler, so the same ABI should be used in both functions.

This change introduces the following changes, which fix the previous assumptions and bugs:

• All internal functions declare a two extra pointer-sized parameters, which will hold the callee and caller VMContexts
• Use a pinned register that will be considered live through the lifetime of the function instead of pinning it at the trampoline level. The pinning explicitlly happens when entering the function body and no other assumptions are made from there on.
• Introduce the concept of special ContextArgs for function calls. This enum holds metadata about which context arguments are needed depending on the callee. The previous implementation of introducing register values at arbitrary locations in the value stack conflicts with the stack ordering principle which states that older values must always precede newer values. So we can't insert a register, because if a spill happens the order of the values will be wrong.

Finally, given that this change also enables the imports.wast test suite, it also includes a fix to global.{get, set} instructions which didn't account entirely for imported globals.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Feb 21 2024 at 16:26):

saulecabrera edited PR #7974:

This change overhauls Winch's ABI. This means that as part of this change, the default ABI now closely resembles Cranelift's ABI, particularly on the treatment of the VMContext. This change also fixes many wrong assumptions about trampolines, which are tied to how the previous ABI operated.

The main motivation behind this change is:

• To make it easier to integrate Winch-generated functions with Wasmtime
• Fix fuzz bugs related to imports
• Solidify the implementation regarding the usage of a pinned register to hold the VMContext value throughout the lifetime of a function.

The previous implementation had the following characteristics, and wrong assumptions:

• Assumed that internal functions don't receive a caller or callee VMContexts as parameters.
• Worked correctly in the following scenarios:
  * Wasm -> Native: since we can explicitly load the caller and callee VMContext, because we're calling a native import.
  * (Native, Array) -> Wasm: because the native signatures define a tuple of VMContext as arguments.
• It didn't work in the following scenario:
  • Wasm -> Wasm: When calling imports from another WebAssembly instance (via direct call or call_indirect. The previous implementation wrongly assumed that there should be a trampoline in this case, but there isn't. The code was generated by the same compiler, so the same ABI should be used in both functions.

This change introduces the following changes, which fix the previous assumptions and bugs:

• All internal functions declare a two extra pointer-sized parameters, which will hold the callee and caller VMContexts
• Use a pinned register that will be considered live through the lifetime of the function instead of pinning it at the trampoline level. The pinning explicitlly happens when entering the function body and no other assumptions are made from there on.
• Introduce the concept of special ContextArgs for function calls. This enum holds metadata about which context arguments are needed depending on the callee. The previous implementation of introducing register values at arbitrary locations in the value stack conflicts with the stack ordering principle which states that older values must always precede newer values. So we can't insert a register, because if a spill happens the order of the values will be wrong.

Finally, given that this change also enables the imports.wast test suite, it also includes a fix to global.{get, set} instructions which didn't account entirely for imported globals.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Feb 21 2024 at 17:47):

cfallin submitted PR review:

Overall looks great, thanks! Just a few nits below.

Wasmtime GitHub notifications bot (Feb 21 2024 at 17:47):

cfallin submitted PR review:

Overall looks great, thanks! Just a few nits below.

Wasmtime GitHub notifications bot (Feb 21 2024 at 17:47):

cfallin created PR review comment:

s/the the/the/

Wasmtime GitHub notifications bot (Feb 21 2024 at 17:47):

cfallin created PR review comment:

s/paramters/parameters/

Wasmtime GitHub notifications bot (Feb 21 2024 at 17:47):

cfallin created PR review comment:

Rather than unchecked, which to me implies lack of a bounds check or similar, can we name this in a way that indicates the index-space is shifted relative to above? E.g. perhaps the above get_local becomes get_wasm_local and get_local_unchecked becomes get_raw_local?

Alternately, avoid exposing get_local_unchecked at all and have two separate methods get_callee_local() / get_caller_local()?

Wasmtime GitHub notifications bot (Feb 21 2024 at 17:47):

cfallin created PR review comment:

here and perhaps elsewhere, can we have a constant (maybe an associated constant of ContextArgs) indicating how many args we expect? Basically it'd be nice to tie this to other definitions so that we don't miss it if we ever add, e.g., a third context arg.

Wasmtime GitHub notifications bot (Feb 21 2024 at 20:45):

github-actions[bot] commented on PR #7974:

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "winch"

Thus the following users have been cc'd because of the following labels:

• saulecabrera: winch

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:15):

saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #7974.

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:15):

saulecabrera requested wasmtime-core-reviewers for a review on PR #7974.

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:15):

saulecabrera requested fitzgen for a review on PR #7974.

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:15):

saulecabrera updated PR #7974.

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:16):

saulecabrera commented on PR #7974:

Oh bad rebase -- sorry about that. Fixing it now.

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:20):

saulecabrera updated PR #7974.

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:54):

saulecabrera updated PR #7974.

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:54):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:54):

saulecabrera created PR review comment:

Fixed, thanks!

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:54):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:54):

saulecabrera created PR review comment:

Fixed!

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:55):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:55):

saulecabrera created PR review comment:

Thanks for the suggestion -- I ended up with get_frame_local and get_wasm_local to be able to distinguish one from another.

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:56):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 21 2024 at 21:56):

saulecabrera created PR review comment:

Good catch -- I introduced MAX_CONTEXT_ARGS near the ContextArgs definition, which is used in the trampoline. In the non-trampoline case (normal function calls), the expected number of context arguments is already parametrized through ContextArgs::len

Wasmtime GitHub notifications bot (Feb 21 2024 at 23:15):

saulecabrera merged PR #7974.

Last updated: Dec 23 2024 at 12:05 UTC