Skip to content

feat(c): CBOR-free transport for non-scalar abi = c procs#137

Open
gmelodie wants to merge 1 commit into
masterfrom
feat/non-scalar-fast-path
Open

feat(c): CBOR-free transport for non-scalar abi = c procs#137
gmelodie wants to merge 1 commit into
masterfrom
feat/non-scalar-fast-path

Conversation

@gmelodie

@gmelodie gmelodie commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

Implements the second half of #131: "Additionally, implement CBOR-free path for non-scalar procs too".

Non-scalar abi = c procs already presented a CBOR-free foreign surface — the _CWire flat structs from #87 are the C ABI — but every call still paid a full CBOR round trip internally, on both legs of the FFI-thread hop:

cwireUnpack(req[])cborEncodeShared → hop → cborDecodePtr → handler → cborEncode → hop → cborDecodePtrcwirePack

Only the scalar fast path (#106/#110/#124) was genuinely CBOR-free. This PR carries the packed _CWire struct itself across the hop instead, so CBOR is gone from the abi = c path in both directions:

  • Request: the calling thread packs into a malloc'd owned copy (cwireOwnedCopy) which the FFI thread unpacks and frees. cwireUnpack deep-copies into Nim memory (strings via $), so freeing the wire right after the unpack is safe; the envelope buffer itself still goes with deleteRequest.
  • Reply: an object return rides back as the native image of its packed _CWire (cwireStructBytes) and a string return as raw UTF-8 (ffiRawRetBytes). The reply trampoline hands the struct's address straight to the caller, then cwireFrees the buffers it owns.

The foreign ABI is unchanged: check_bindings passes, so the generated headers and exported dylib symbols are byte-identical. This is purely an internal transport swap.

Emission order

The _CWire companions are emitted by genBindings() at the bottom of the user's module, but handlers expand at each {.ffi.} annotation above it — so a handler naming ShoutResponse_CWire would reference a type Nim has not seen yet (confirmed in the macro dump: the EchoShoutReq handler lands at line ~114, ShoutResponse_CWire at ~257). The Req type must also precede its own _CWire, so the dependency is genuinely circular in one file.

abi = c therefore splits emission: the Req envelope type stays at the annotation site, while processFFIRequest + the registry assignment are built there but stashed on CAbiSpec.handler and flushed by flushCAbiDispatch right after ensureCWireForFields. This is the same mechanism the abi = c ctor path already used, now shared by both. registerReqFFI keeps its CBOR behaviour untouched for its existing callers in tests/unit/test_gc_compat.nim, test_event_thread.nim, test_event_dispatch.nim and tests/bench/, and no new macros are added to the public API.

Affected areas

  • ffi/ffi_thread_request.nim: isScalarrawReply, generalised from "scalar fast path" to "CBOR-free request", since an abi = c string reply of "" is also a real empty value rather than CBOR's "no value" sentinel. initFromOwnedShared gains a rawReply parameter; ffiScalarRetBytesffiRawRetBytes (same behaviour, now shared with the abi = c string reply).
  • ffi/internal/ffi_macro.nim: reqDecodePreamble and replyEncode select the payload shape per ABI; buildProcessFFIRequestProc / addNewRequestToRegistry / buildCtorProcessFFIRequestProc / addCtorRequestToRegistry take an ABIFormat. abi = c ctors no longer emit the CBOR-only ffiNewReq.
  • ffi/internal/c_macro_helpers.nim: the exported method/ctor wrappers pack an owned wire instead of cborEncodeShared; the reply trampolines read the raw image instead of cborDecodePtr. cwireTypeName/isStringType exported for ffi_macro.
  • ffi/internal/c_wire.nim: adds cwireAllocBuf/cwireFreeBuf, cwireStructBytes, cwireOwnedCopy.
  • tests/unit/test_c_abi_dispatch.nim: new; drives an abi = c library through a real FFI thread.

Impact on library users

No API or ABI change — additive and internal. Generated headers, exported symbols and the reply/error ownership contract are identical (check_bindings is green). Non-scalar abi = c calls simply stop encoding and decoding CBOR on each leg of the hop.

The renames (isScalarrawReply, ffiScalarRetBytesffiRawRetBytes) touch symbols that are exported but internal to the request plumbing; no example or generated binding referenced them.

Risk assessment

The real risk is memory ownership, since request buffers are now allocated on the calling thread and freed on the FFI thread.

  • Payload validation: the request preamble rejects a payload whose dataLen is not sizeof(<Req>_CWire) before casting, mirroring the size check the reply trampoline applies. Without it a short payload would be an out-of-bounds read of every cstring field.
  • Rejected sends: sendRequestToFFIThread deleteRequests on the eventQueueStuck and reentrancy paths, and deleteRequest only frees the payload buffer itself — not the buffers a _CWire struct points at. The wrappers therefore cwireFree the wire they still alias on that path; on success the FFI thread's unpack owns that free instead. (The CBOR blob was a single allocation, so this hazard is new to this PR.)
  • Allocation failure: cwireOwnedCopy returns nil rather than copyMeming into a failed malloc, and the wrappers surface it as RET_ERR. Note the generated cwirePack sites still write through unchecked cwireAllocBuf/cwireAllocStr results — that is the pre-existing alloc.nim pattern repo-wide and is left for a separate change.
  • _CWire seq/Option payload buffers moved from allocShared to libc malloc (cwireAllocBuf), matching the rationale ffi/alloc.nim already documents for the request envelope: libc is process-global, so a foreign caller thread that exits before the FFI thread frees cannot dangle. To be precise about the evidence: reverting this to allocShared did not reproduce a failure in the new tests, so this is consistency with alloc.nim's stated rule rather than a fix for an observed crash — the tests keep the calling thread alive, so they do not exercise the exiting-producer case the comment warns about.
  • cwireOwnedCopy uses copyMem rather than assignment because the destination is raw malloc memory and an assignment would run ORC's copy hooks over uninitialised bytes.

Verification

  • nimble test (full unit suite), nimble test_c_e2e, nimble test_c_abi_e2e, nimble test_cpp_e2e (20/20), nimble check_bindings — all pass.
  • NIM_FFI_SAN=asan-ubsan nimble test_c_abi_e2e_sanitized — clean, no leaks or UB. Sanitizer presence confirmed in the linked binary (the task defaults to NIM_FFI_SAN=none, which silently proves nothing).
  • CBOR removal confirmed directly from -d:ffiDumpMacros output: zero cborEncode/cborDecode in the abi = c dispatch, replaced by cwireUnpack / cwireOwnedCopy / cwireStructBytes / ffiRawRetBytes.
  • New tests/unit/test_c_abi_dispatch.nim (4 tests) drives seq[string] + Option[string] request fields across the hop through a real FFI thread, covering the object reply, the raw-UTF-8 string reply, the empty-seq/none case, and the handler error path. The echo example only has string fields, so seq/Option payload buffers crossing threads had no end-to-end coverage before.

Known coverage gap: no test exercises the rejected-send path (eventQueueStuck / reentrancy), so the cwireFree-on-failure fix above is reasoned from the ownership contract rather than observed under ASAN.

References

The other half of #131 — supporting CBOR and abi = c simultaneously in one library (the libWireFormat mismatch guard in ffi/codegen/c.nim) — is not addressed here and needs a decision on the export-symbol collision, since both ABIs currently exportc the same name with incompatible signatures.

@gmelodie
gmelodie force-pushed the feat/non-scalar-fast-path branch 2 times, most recently from 2633076 to 73b9068 Compare July 17, 2026 12:02
@gmelodie
gmelodie force-pushed the feat/non-scalar-fast-path branch from 73b9068 to 842b836 Compare July 17, 2026 12:12
@gmelodie
gmelodie marked this pull request as ready for review July 17, 2026 12:12
@gmelodie
gmelodie requested a review from Ivansete-status July 17, 2026 17:11
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.

1 participant