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

c#: Handle Cabi realloc post return #1145

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jsturtevant
Copy link
Collaborator

fixes #1116

This pull request includes several changes to the FunctionBindgen implementation in the crates/csharp/src/function.rs file, primarily focusing on memory management and cleanup handling. Additionally, there are modifications in the InterfaceGenerator implementation in the crates/csharp/src/interface.rs file to support post-return handling for guest exports.

The makes sure memory that was allocated for export functions is removed in the ABI's post return. Prior to this change exports where pinning memory but not releasing the pins in most cases. It also fixes a case in the List import where the list was being lowered and clean up code was executing but since memory holding it was pinned the memory was still there. This makes sure that the memory is exported for long enough time and is cleaned up in later calls.

@jsturtevant jsturtevant requested review from yowl, silesmo and dicej January 28, 2025 22:14
@@ -868,6 +870,10 @@ impl Bindgen for FunctionBindgen<'_, '_> {
}}
"
);
if realloc.is_none() {
uwrite!(self.src,
"cleanups.Add(()=> NativeMemory.AlignedFree({address}));")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naive question, why do we keep cleanups list instead of generating this line after the call to the host ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of a lower during an import, we would still need to clean up the lowered list. We could re-write this to use an array/stackalloc and fixed in the case of an import.

Copy link
Collaborator

@pavelsavara pavelsavara Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest that we try to kill as many by using fixed first and then see what scenarios still need another approach.

I think the nested arrays in public static unsafe void ListParam4(List<List<string>> a) is the nasty example.
I'm confused why the buffers are not freed by the receiving end ? Could you please explain (or link the spec).

It seems to me that if we really need cleanup with dynamic number of elements, perhaps all those elements are just buffers. At lest current code looks like it.
And so cleanups collection could be just list of pointers instead of more expensive delegates.

var cleanups = new List<IntPtr>();

var address3 = NativeMemory.AlignedAlloc(10,4);
cleanups.Add((IntPtr)address3);

/// more code here ...

foreach (var cleanup in cleanups)
{
    Marshal.FreeHGlobal(cleanup);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And so cleanups collection could be just list of pointers instead of more expensive delegates.

This one is on me, We had two different clean up code paths a few pr's back and I consolidated both of them and used a delegate because we had two different types and it was simple way to handle it. I hadn't really considered the allocations from the closure. I've got #1143 to track cleaning it up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused why the buffers are not freed by the receiving end ? Could you please explain (or link the spec).

/// Lowers a list where the element's layout in the native language is
/// expected to match the canonical ABI definition of interface types.
///
/// Pops a list value from the stack and pushes the pointer/length onto
/// the stack. If `realloc` is set to `Some` then this is expected to
/// *consume* the list which means that the data needs to be copied. An
/// allocation/copy is expected when:
///
/// * A host is calling a wasm export with a list (it needs to copy the
/// list in to the callee's module, allocating space with `realloc`)
/// * A wasm export is returning a list (it's expected to use `realloc`
/// to give ownership of the list to the caller.
/// * A host is returning a list in a import definition, meaning that
/// space needs to be allocated in the caller with `realloc`).
///
/// A copy does not happen (e.g. `realloc` is `None`) when:
///
/// * A wasm module calls an import with the list. In this situation
/// it's expected the caller will know how to access this module's
/// memory (e.g. the host has raw access or wasm-to-wasm communication
/// would copy the list).
///
/// If `realloc` is `Some` then the adapter is not responsible for
/// cleaning up this list because the other end is receiving the
/// allocation. If `realloc` is `None` then the adapter is responsible
/// for cleaning up any temporary allocation it created, if any.

There are couple scenarios describe in that doc how to handle lowering but this particular code change has two scenarios:

  1. first one is when its an import (we are calling the host/another component). We can lower our type and then clean it up after the call to the external component is complete. - i.e realloc.is_none()
  2. We are in an export (meaning the host or another component is calling us). In this case, we need to allocate and lower but that lowered value needs to live past our function call and so the ABI will call us with cabi_post to clean it up. i.e realloc.is_some()

This section has some notes too but I find it harder to parse: https://github.com/WebAssembly/component-model/blob/main/design/mvp/Explainer.md#canonical-abi

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest that we try to kill as many by using fixed first and then see what scenarios still need another approach.

Agreed, there was a mix different ways this was done originally... slowly trying to consolidate them all as we learn. I've opened #1150 to track that work

Signed-off-by: James Sturtevant <[email protected]>
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

Successfully merging this pull request may close these issues.

c#: handle cabi_post properly
2 participants