fix(nim-bindings): bridge Nim/C ABI mismatch via C shim layer#64
fix(nim-bindings): bridge Nim/C ABI mismatch via C shim layer#64osmaczko wants to merge 2 commits intochore/remove-dynamic-loadingfrom
Conversation
Nim's code generator transforms function signatures involving large structs
in two ways that conflict with the standard C ABI:
- Return values of large structs (> register size): Nim emits a void
function with an explicit out-pointer appended as the *last* argument.
The standard x86-64 SysV ABI passes the hidden return pointer in RDI
(before the real arguments); ARM64 aapcs64 uses X8. Calling Rust
directly from Nim therefore puts the pointer in the wrong register /
stack slot on both architectures, causing crashes.
- Large struct parameters (> ~24 bytes): Nim passes a pointer rather
than copying bytes on the stack / into registers as the C ABI expects.
This commit introduces a thin C shim (nim_shims.c) that acts as a
translation layer:
- Each nim_* wrapper is declared with a Nim-compatible signature, so
Nim calls it correctly by its own rules.
- Inside the wrapper the C compiler calls the real Rust-exported
function using the standard C ABI, inserting the correct hidden-
pointer placement and stack-copy behaviour for the current platform.
As a result:
- The Rust API stays standard C ABI (return T by value; destroy takes
*mut T, which is pointer-sized and matches Nim's large-param transform).
- Other language bindings (C, Swift, Go, …) call Rust directly without
any shim — the standard ABI is preserved for them.
- The fix is correct on both x86-64 and ARM64 without any
architecture-specific code in Nim or Rust.
Changes:
- nim-bindings/src/nim_shims.c: C bridge with nim_* wrappers for all
create/handle/installation_name and destroy functions
- nim-bindings/src/bindings.nim: {.compile: "nim_shims.c"}, proc
signatures use natural return-by-value form, importc names point to
the nim_* shims
- nim-bindings/src/libchat.nim: call sites use natural let binding form;
destroy calls pass addr res (ptr T)
- conversations/src/api.rs: destroy functions take *mut T so Nim's
large-param-to-pointer transform is satisfied without a stack copy
There was a problem hiding this comment.
Pull request overview
This PR fixes critical ABI mismatches between Nim's calling convention and the standard C ABI when calling Rust FFI functions. Nim transforms functions returning large structs (>16 bytes) into void functions with an out-pointer as the last argument, and transforms large struct parameters into pointers. These transformations conflict with the x86-64 SysV and ARM64 aapcs64 ABIs, which handle hidden return pointers differently, causing crashes.
The solution introduces a thin C shim layer (nim_shims.c) that bridges between Nim's conventions and standard C ABI:
- Each
nim_*wrapper function accepts Nim's calling convention - The C compiler correctly handles the standard C ABI when calling Rust functions
- Rust destroy functions are changed to take
*mut Tto match Nim's pointer-passing for large structs
Changes:
- Added C shim layer to translate between Nim and Rust calling conventions
- Updated Rust destroy functions to accept pointers instead of values
- Modified Nim bindings to call through the shim and pass pointers to destroy functions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| nim-bindings/src/nim_shims.c | New C shim layer with wrapper functions that bridge Nim's calling convention to standard C ABI for all create/handle/installation_name and destroy functions |
| nim-bindings/src/bindings.nim | Updated to compile nim_shims.c and route function calls through nim_* shims; destroy functions now take ptr types |
| nim-bindings/src/libchat.nim | Updated to use var instead of let for results and pass addr to destroy functions |
| conversations/src/api.rs | Changed destroy functions to take *mut T pointers and use drop_in_place for proper cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let name = installation_name(ctx.handle) | ||
| result = $name |
There was a problem hiding this comment.
Memory leak: the ReprCString returned from installation_name contains heap-allocated data that must be freed. After copying the string on line 28, you need to free the original ReprCString's heap memory. However, there's no destroy function provided for ReprCString. You should either add a destroy function for ReprCString or change the approach to avoid the leak.
jazzz
left a comment
jazzz
left a comment
There was a problem hiding this comment.
Oof. I don't like this or #62.
Originally caller provided buffer approach was abandoned, in favor of safer_ffi's better flexibility and type safety.
This issue with nim abi is diminishing the benefit of safer_ffi.
The Split-Ownership model, where the Caller owns the outer struct and rust owns the inner members is going to cause issues. Returning results by value and then freeing by reference lacks symmetry which makes it harder to reason about.
I don't have any strict blockers for this approach (assuming comments are addressed) however I'm starting to wonder if a Box allocated model would be cleaner. Heap-Allocate every call via -> Box<T>. Memory is completely managed by rust, and since the return type is a boxed pointer all issues with the ABI are sidestepped.
| proc installation_name*(ctx: ContextHandle): ReprCString {.importc.} | ||
| proc installation_name*(ctx: ContextHandle): ReprCString {.importc: "nim_installation_name".} |
There was a problem hiding this comment.
I like the nim prefix for avoiding name conflicts
| destroy_intro_result(addr res) | ||
| return err("Failed to create intro bundle: " & $res.error_code) |
There was a problem hiding this comment.
[Mountain] Is this not undefined behavior? res is destroyed, and then its member 'error_code` is accessed
| result = err("Failed to create private convo: " & $res.error_code) | ||
| destroy_intro_result(res) | ||
| return | ||
| destroy_intro_result(addr res) | ||
| return err("Failed to create intro bundle: " & $res.error_code) |
There was a problem hiding this comment.
[Mountain] This looks like undefined behavior. res is freed and then its member error_code is accessed.
There was a problem hiding this comment.
[Pebble] This file ought to generated by safer_ffi::generate_headers to ensure that updates are appropriately handled.
| if !result.is_null() { | ||
| unsafe { std::ptr::drop_in_place(result) } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't love this. Though I see its need.
It's not because the unsafe code - thats appropriate here. But because the memory memory model is quietly altered. Previously objects were allocated in rust , ownership was passed to nim, and then returned when no longer needed for deletion/cleanup.
This model is now changed, and will need some documentation. Especially since the generative api calls have not changed.
|
tagging @kaichaosun for observability |
|
Arguments given in #62 (comment) convinced me. We can get back to this solution once it is needed (i.e. more bindings). |
Nim's code generator transforms function signatures involving large structs in two ways that conflict with the standard C ABI:
Return values of large structs (> register size): Nim emits a void function with an explicit out-pointer appended as the last argument. The standard x86-64 SysV ABI passes the hidden return pointer in RDI (before the real arguments); ARM64 aapcs64 uses X8. Calling Rust directly from Nim therefore puts the pointer in the wrong register / stack slot on both architectures, causing crashes.
Large struct parameters (> ~24 bytes): Nim passes a pointer rather than copying bytes on the stack / into registers as the C ABI expects.
This commit introduces a thin C shim (nim_shims.c) that acts as a translation layer:
As a result:
Changes: