From 947a922be2e1353e18c4caf129af6ffcf7ba7027 Mon Sep 17 00:00:00 2001 From: Gabriel Cruz Date: Thu, 16 Jul 2026 10:53:47 -0300 Subject: [PATCH] fix: pr comments --- README.md | 34 ++++++------------ examples/echo/c_abi_bindings/echo.h | 7 ++-- examples/echo/c_bindings/nim_ffi_cbor.h | 4 +-- examples/timer/c_bindings/nim_ffi_cbor.h | 4 +-- ffi/codegen/c.nim | 40 +++++++-------------- ffi/codegen/meta.nim | 12 ++----- ffi/codegen/templates/c/cbor_helpers.h.tpl | 4 +-- ffi/internal/ffi_macro.nim | 30 ++++++++-------- ffi/internal/ffi_scalar.nim | 8 ++--- tests/e2e/c_abi/test_echo_c_abi.c | 15 ++++---- tests/unit/fixtures/scalar_skip_fixture.nim | 11 +++--- tests/unit/test_c_abi_codegen.nim | 3 -- tests/unit/test_scalar_skip_gen.nim | 11 ++---- 13 files changed, 66 insertions(+), 117 deletions(-) diff --git a/README.md b/README.md index 27e9ec7..00637cd 100644 --- a/README.md +++ b/README.md @@ -160,29 +160,17 @@ header shape from the library's ABI format. It carries two honest limits today: - **Events are CBOR-only.** Applying `abi = c` to an `{.ffiEvent.}` proc is a hard compile error; declare events with `abi = cbor` (they ride CBOR internally regardless of the library default). -- **All-scalar `abi = c` procs are dropped from the foreign bindings.** A - `{.ffi: "abi = c".}` method whose every param and return is a plain scalar - takes the CBOR-free scalar fast path at runtime, but the foreign codegen for - that inline-args shape is a follow-up (tracked in #120) — such procs are - omitted from the generated `.h`. Give a proc at least one non-scalar - (struct / `seq` / `Option`) param or return, or use `abi = cbor`, if you need - it in the bindings. - -An `abi = c` proc whose whole signature is scalar — fixed-width integer, float, -or bool params (a `string` return is fine, a `string` param is not) and no -structs, handles, or pointers — dispatches through a CBOR-free scalar fast path. -The `abi = c` C header (an `abi = c` library generated with `-d:targetLang=c`) -emits real bindings for that shape: the wrapper passes the scalar args inline -(no request struct) and adapts the raw-bytes reply into the same typed callback -surface the flat-struct methods use. The CBOR-speaking outputs — the CBOR C -header and the `cpp`, `rust`, `cddl` targets — have no scalar codegen, so under -`-d:ffiGenBindings` they would omit such a proc from the generated bindings — -and `genBindings()` fails with an error naming the affected procs. Resolve it by -making the whole library `abi = c` and generating C bindings, switching the proc -to `abi = cbor`, adding a non-scalar param so it takes the CBOR wire shape, or -passing `-d:ffiAllowScalarSkip` to accept the omission (the proc still works -over the scalar fast path; it's just absent from the generated foreign -bindings). +- **All-scalar `abi = c` procs bind only in the `abi = c` C header.** A + `{.ffi: "abi = c".}` method whose params and return are all scalars — ints, + floats, bools; a `string` return is fine, a `string` param is not — takes a + CBOR-free fast path, and its C wrapper passes the args inline instead of + packing a request struct. Only the `abi = c` C header emits that shape. The + CBOR C header and the `cpp`, `rust` and `cddl` targets have no scalar codegen + and would silently omit the proc, so `genBindings()` fails and names it. Fix + it by generating C bindings from an `abi = c` library, switching the proc to + `abi = cbor`, giving it a non-scalar param, or passing + `-d:ffiAllowScalarSkip` to accept the omission — the proc still works over the + fast path, it's just absent from the bindings. ## Placement of `genBindings()` diff --git a/examples/echo/c_abi_bindings/echo.h b/examples/echo/c_abi_bindings/echo.h index a7d7961..def7d56 100644 --- a/examples/echo/c_abi_bindings/echo.h +++ b/examples/echo/c_abi_bindings/echo.h @@ -37,14 +37,13 @@ typedef void (*EchoShoutReplyFn)(int err_code, const ShoutResponse* reply, const typedef void (*EchoVersionReplyFn)(int err_code, const char* reply, const char* err_msg, void* user_data); typedef void (*EchoCreateRawFn)(int err_code, const char* ctx_addr, const char* err_msg, void* user_data); -/* Raw reply of a scalar-fast-path export: `msg`/`len` are raw bytes (a - string return's UTF-8, or the 8-byte native-endian scalar image), not +/* Raw reply of a scalar-fast-path export: `msg`/`len` are bytes (a string + return's UTF-8, or the 8-byte native-endian scalar image), not NUL-terminated and valid only for the duration of the call. */ typedef void (*EchoScalarRawFn)(int caller_ret, char* msg, size_t len, void* user_data); #ifndef NIMFFI_ABI_DUP_CSTR_N #define NIMFFI_ABI_DUP_CSTR_N -/* NUL-terminated copy of a length-delimited (not NUL-terminated) byte run; - NULL on allocation failure or a length that would overflow `n + 1`. */ +/* NUL-terminated copy of a length-delimited byte run; NULL if it can't. */ static inline char* nimffi_abi_dup_cstr_n(const char* s, size_t n) { if (n == SIZE_MAX) return NULL; char* p = (char*)malloc(n + 1); diff --git a/examples/echo/c_bindings/nim_ffi_cbor.h b/examples/echo/c_bindings/nim_ffi_cbor.h index 63244aa..059f7a3 100644 --- a/examples/echo/c_bindings/nim_ffi_cbor.h +++ b/examples/echo/c_bindings/nim_ffi_cbor.h @@ -271,8 +271,8 @@ static inline char* nimffi_dup_cstr(const char* s) { } /* NUL-terminated copy of a length-delimited (not NUL-terminated) byte run, - * for turning the FFICallback's raw error `msg`/`len` into a C string. Returns - * NULL on allocation failure or a length that would overflow `n + 1`. */ + * for turning the FFICallback's raw error `msg`/`len` into a C string; NULL if + * it can't. */ static inline char* nimffi_dup_cstr_n(const char* s, size_t n) { if (n == SIZE_MAX) { return NULL; diff --git a/examples/timer/c_bindings/nim_ffi_cbor.h b/examples/timer/c_bindings/nim_ffi_cbor.h index 63244aa..059f7a3 100644 --- a/examples/timer/c_bindings/nim_ffi_cbor.h +++ b/examples/timer/c_bindings/nim_ffi_cbor.h @@ -271,8 +271,8 @@ static inline char* nimffi_dup_cstr(const char* s) { } /* NUL-terminated copy of a length-delimited (not NUL-terminated) byte run, - * for turning the FFICallback's raw error `msg`/`len` into a C string. Returns - * NULL on allocation failure or a length that would overflow `n + 1`. */ + * for turning the FFICallback's raw error `msg`/`len` into a C string; NULL if + * it can't. */ static inline char* nimffi_dup_cstr_n(const char* s, size_t n) { if (n == SIZE_MAX) { return NULL; diff --git a/ffi/codegen/c.nim b/ffi/codegen/c.nim index 1e3ca40..b248ad5 100644 --- a/ffi/codegen/c.nim +++ b/ffi/codegen/c.nim @@ -946,8 +946,6 @@ proc newAbiReg(types: seq[FFITypeMeta], procs: seq[FFIProcMeta]): AbiReg = for t in types: reg.typeTable[t.name] = t for p in procs: - # A scalar-fast-path proc has no Req envelope: its export takes the scalar - # args inline (see emitAbiScalarMethod). if p.kind != FFIKind.DTOR and not p.scalarFastPath: let rt = reqTypeMeta(p) reg.typeTable[rt.name] = rt @@ -1013,24 +1011,18 @@ proc emitAbiReplyTypedefs( ) func abiScalarRawFnName(libType: string): string = - ## The `FFICallBack`-shaped raw callback typedef a scalar-fast-path export - ## takes: the dylib replies with raw bytes (no CBOR, no flat struct) that the - ## per-method trampoline converts into the typed reply. + ## Raw-bytes callback typedef a scalar-fast-path export takes. return libType & "ScalarRawFn" const abiScalarDupCStr = "nimffi_abi_dup_cstr_n" func abiScalarDupHelper(): seq[string] = - ## CBOR-free twin of `nimffi_dup_cstr_n` for the scalar trampolines: a - ## NUL-terminated copy of a length-delimited (not NUL-terminated) byte run, - ## returning NULL on allocation failure or a length that would overflow the - ## `n + 1` size passed to `malloc`. - # Guarded so co-including two `abi = c` headers in one TU doesn't redefine it. + ## CBOR-free twin of `nimffi_dup_cstr_n`, include-guarded so two `abi = c` + ## headers can co-exist in one TU. return @[ "#ifndef NIMFFI_ABI_DUP_CSTR_N", "#define NIMFFI_ABI_DUP_CSTR_N", - "/* NUL-terminated copy of a length-delimited (not NUL-terminated) byte run;", - " NULL on allocation failure or a length that would overflow `n + 1`. */", + "/* NUL-terminated copy of a length-delimited byte run; NULL if it can't. */", "static inline char* " & abiScalarDupCStr & "(const char* s, size_t n) {", " if (n == SIZE_MAX) return NULL;", " char* p = (char*)malloc(n + 1);", @@ -1044,8 +1036,6 @@ func abiScalarDupHelper(): seq[string] = ] func abiScalarArgParams(m: FFIProcMeta): seq[string] = - ## C parameters for a scalar method's args — passed inline by value, in both - ## the raw export and the high-level wrapper (no Req struct). var params: seq[string] = @[] for ep in m.extraParams: params.add(abiLeafCType(ep.typeName.strip()).cType & " " & ep.name) @@ -1070,10 +1060,10 @@ proc emitAbiExternDecls( ")(int err_code, const char* ctx_addr, const char* err_msg, void* user_data);" ) if haveScalar: - lines.add("/* Raw reply of a scalar-fast-path export: `msg`/`len` are raw bytes (a") lines.add( - " string return's UTF-8, or the 8-byte native-endian scalar image), not" + "/* Raw reply of a scalar-fast-path export: `msg`/`len` are bytes (a string" ) + lines.add(" return's UTF-8, or the 8-byte native-endian scalar image), not") lines.add(" NUL-terminated and valid only for the duration of the call. */") lines.add( "typedef void (*" & abiScalarRawFnName(libType) & @@ -1240,11 +1230,9 @@ proc emitAbiMethod( lines.add("") func abiScalarOkLines(m: FFIProcMeta, fnType: string): seq[string] = - ## Trampoline RET_OK branch: convert the raw reply bytes into the typed - ## reply. A string return rides as its own UTF-8 bytes (copied and - ## NUL-terminated here); every other scalar is the 8-byte native-endian image - ## of the Nim-side pack (signed ints sign-extended to 64 bits, floats widened - ## to double, bool as 0/1 — see `ffiScalarRetBytes`). + ## Trampoline RET_OK branch. A string return rides as its own UTF-8; every + ## other scalar is the 8-byte image `ffiScalarRetBytes` packs (ints + ## sign-extended, floats widened to double, bool as 0/1). let rt = m.returnTypeName.strip() if rt == "string" or rt == "cstring": return @[ @@ -1294,13 +1282,9 @@ proc emitAbiScalarMethod( ctxType, libName, libType: string, m: FFIProcMeta, ) = - ## A scalar-fast-path method: no Req struct crosses — the wrapper hands the - ## scalar args straight to the raw export and a per-method trampoline adapts - ## the raw-bytes reply into the same typed `ReplyFn` surface the flat-struct - ## methods use. The heap box carrying the caller's callback is freed by the - ## trampoline, which the dylib invokes exactly once on every path (the ctx - ## guard and enqueue failures reply synchronously; success replies from the - ## FFI thread). + ## Args go inline to the raw export and a trampoline adapts the raw-bytes + ## reply into the typed `ReplyFn` surface. The trampoline frees the callback + ## box, relying on the dylib invoking it exactly once on every path. let stripped = stripLibPrefix(m.procName, m.libName) let pascal = snakeToPascalCase(stripped) let info = abiMethodReplyInfo(reg, libType, m) diff --git a/ffi/codegen/meta.nim b/ffi/codegen/meta.nim index b493611..1aa7314 100644 --- a/ffi/codegen/meta.nim +++ b/ffi/codegen/meta.nim @@ -32,11 +32,8 @@ type returnIsHandle*: bool abiFormat*: ABIFormat scalarFastPath*: bool - ## True for an `abi = c` proc whose whole signature is scalar (see - ## `isScalarOnly`): dispatches through the CBOR-free scalar fast path. - ## Only the `abi = c` C header emits a foreign binding for it (inline - ## scalar args + raw-bytes reply trampoline); every other target — and - ## the CBOR C header — drops it (see `bindableProcs`). + ## `abi = c` proc with an all-scalar signature: uses the CBOR-free fast + ## path, and binds only in the `abi = c` C header (see `bindableProcs`). FFIFieldMeta* = object name*: string @@ -138,8 +135,5 @@ const ffiOutputDir* {.strdefine.} = "" # Nim src path override relative to outputDir (-d:ffiSrcPath); empty derives it. const ffiSrcPath* {.strdefine.} = "" -# When set, scalar-only `abi = c` procs are silently omitted from bindings -# generated by targets without scalar codegen (every target except the -# `abi = c` C header) instead of failing the build. Off by default so the drop -# is loud; see genBindings(). +# When true, targets without scalar codegen silently omit scalar-only `abi = c` procs rather than failing the build. Off by default so the drop is loud; see genBindings(). const ffiAllowScalarSkip* {.booldefine.} = false diff --git a/ffi/codegen/templates/c/cbor_helpers.h.tpl b/ffi/codegen/templates/c/cbor_helpers.h.tpl index 8afdba7..3056aab 100644 --- a/ffi/codegen/templates/c/cbor_helpers.h.tpl +++ b/ffi/codegen/templates/c/cbor_helpers.h.tpl @@ -271,8 +271,8 @@ static inline char* nimffi_dup_cstr(const char* s) { } /* NUL-terminated copy of a length-delimited (not NUL-terminated) byte run, - * for turning the FFICallback's raw error `msg`/`len` into a C string. Returns - * NULL on allocation failure or a length that would overflow `n + 1`. */ + * for turning the FFICallback's raw error `msg`/`len` into a C string; NULL if + * it can't. */ static inline char* nimffi_dup_cstr_n(const char* s, size_t n) { if (n == SIZE_MAX) { return NULL; diff --git a/ffi/internal/ffi_macro.nim b/ffi/internal/ffi_macro.nim index 973c7fa..5911ea0 100644 --- a/ffi/internal/ffi_macro.nim +++ b/ffi/internal/ffi_macro.nim @@ -1519,10 +1519,8 @@ macro ffiEvent*(args: varargs[untyped]): untyped = return generated proc reportScalarFastPathDrops(procs: seq[FFIProcMeta]) {.compileTime.} = - ## Only the `abi = c` C header emits foreign bindings for scalar-fast-path - ## procs; every other target (and the CBOR C header) drops them. Fail loudly, - ## naming them, unless `-d:ffiAllowScalarSkip` opts into the silent omission - ## (then just hint). + ## Fail loudly on scalar-fast-path procs a target can't bind, unless + ## `-d:ffiAllowScalarSkip` downgrades it to a hint. var skipped: seq[string] = @[] for p in procs: if p.scalarFastPath: @@ -1537,15 +1535,17 @@ proc reportScalarFastPathDrops(procs: seq[FFIProcMeta]) {.compileTime.} = ) return error( - "genBindings: this target has no foreign-binding codegen for " & - "scalar-fast-path `abi = c` procs, so these would be silently omitted " & - "from the generated bindings: " & skipped.join(", ") & - ".\nThey are emitted only into the `abi = c` C header (an `abi = c` " & - "library generated with -d:targetLang=c).\n" & "Fix by one of:\n" & - " - make the library `abi = c` (declareLibrary(..., \"c\")) and generate " & - "C bindings, or\n" & " - switch the proc to `abi = cbor`, or\n" & - " - add a non-scalar param (e.g. a struct or handle) so it takes the " & - "CBOR wire shape, or\n" & " - pass -d:ffiAllowScalarSkip to accept the omission." + """genBindings: this target has no foreign-binding codegen for scalar-fast-path +`abi = c` procs, so these would be silently omitted from the generated bindings: +$1 +They are emitted only into the `abi = c` C header (an `abi = c` library generated +with -d:targetLang=c). +Fix by one of: + - make the library `abi = c` (declareLibrary(..., "c")) and generate C bindings, or + - switch the proc to `abi = cbor`, or + - add a non-scalar param (e.g. a struct or handle) so it takes the CBOR wire shape, or + - pass -d:ffiAllowScalarSkip to accept the omission.""" % + [skipped.join(", ")] ) proc bindingsOutputDir(lang, explicit: string): string {.compileTime.} = @@ -1604,9 +1604,7 @@ macro genBindings*( let lang = string_helpers.toLower(rawLang.strip()) if lang.len == 0: continue - # The `abi = c` C header is the one output with scalar-fast-path codegen, - # so it binds the full registry. Every other target — and the CBOR C - # header — drops scalar procs (loudly, unless skipped). + # The `abi = c` C header is the only output with scalar-fast-path codegen. let emitsScalars = lang == "c" and currentDefaultABIFormat == ABIFormat.C let genProcs = if emitsScalars: diff --git a/ffi/internal/ffi_scalar.nim b/ffi/internal/ffi_scalar.nim index 414a69a..3fdde24 100644 --- a/ffi/internal/ffi_scalar.nim +++ b/ffi/internal/ffi_scalar.nim @@ -32,11 +32,9 @@ func isScalarOnly*(p: FFIProcMeta): bool = true func bindableProcs*(procs: seq[FFIProcMeta]): seq[FFIProcMeta] = - ## The procs the CBOR-speaking foreign-binding generators emit for. - ## Scalar-fast-path procs are dropped: their C export takes inline scalar - ## args, not the CBOR `(reqCbor, reqCborLen)` shape those backends assume, so - ## emitting a CBOR caller for them would be wrong. Only the `abi = c` C header - ## has scalar codegen and binds the full registry (see genBindings()). + ## Procs the CBOR-speaking generators emit for; scalar-fast-path procs are + ## dropped (their inline-scalar export doesn't match the CBOR codegen shape). + ## The `abi = c` C header binds the full registry instead. var kept: seq[FFIProcMeta] = @[] for p in procs: if not p.scalarFastPath: diff --git a/tests/e2e/c_abi/test_echo_c_abi.c b/tests/e2e/c_abi/test_echo_c_abi.c index 5846221..893ff43 100644 --- a/tests/e2e/c_abi/test_echo_c_abi.c +++ b/tests/e2e/c_abi/test_echo_c_abi.c @@ -1,12 +1,9 @@ -/* End-to-end test for the CBOR-free `abi = c` echo bindings. Unlike the CBOR C - * backend, this header links no TinyCBOR: the flat structs in echo.h are the C - * ABI, strings are plain borrowed `const char*`. The test drives the same - * async, callback-per-call surface — constructor, an object-returning method, - * a scalar-fast-path method and teardown — copying out what each callback - * delivers (owned by the binding, valid only for the call) and polling a - * `done` flag to sequence the async calls. echoVersion rides the CBOR-free - * scalar fast path: its binding passes no struct and adapts the raw-bytes - * reply into the same typed callback shape as the flat `_CWire` methods. */ +/* End-to-end test for the CBOR-free `abi = c` echo bindings: the `_CWire` + * structs in echo.h are the C ABI, strings are borrowed `const char*`, no + * TinyCBOR. Drives the async callback-per-call surface (ctor, object-returning + * method, teardown). echoVersion rides the scalar fast path, whose binding + * passes no struct and adapts the raw-bytes reply into the same typed callback + * shape as the flat `_CWire` methods. */ #include "echo.h" #include #include diff --git a/tests/unit/fixtures/scalar_skip_fixture.nim b/tests/unit/fixtures/scalar_skip_fixture.nim index 508c02c..8322981 100644 --- a/tests/unit/fixtures/scalar_skip_fixture.nim +++ b/tests/unit/fixtures/scalar_skip_fixture.nim @@ -1,8 +1,7 @@ ## Compile fixture for the scalar-fast-path drop error (see -## tests/unit/test_scalar_skip_gen.nim). This is a CBOR-default library with one -## stray scalar `abi = c` proc, so no target can emit a binding for it: under -## `-d:ffiGenBindings` genBindings() must fail — unless `-d:ffiAllowScalarSkip` -## is passed, which downgrades the drop to a hint. +## tests/unit/test_scalar_skip_gen.nim): a CBOR-default library, so no target +## binds its scalar `abi = c` proc and genBindings() fails unless +## -d:ffiAllowScalarSkip. import ffi, chronos @@ -20,8 +19,8 @@ proc scalarskip_create*(cfg: SkipConfig): Future[Result[SkipLib, string]] {.ffiC proc scalarskip_add*( lib: SkipLib, a: int, b: int ): Future[Result[int, string]] {.ffi: "abi = c".} = - ## All-scalar signature: dispatches through the CBOR-free fast path. In this - ## CBOR-default library no target can emit a foreign binding for it. + ## All-scalar signature: CBOR-free fast path, unbindable outside an `abi = c` + ## library. return ok(lib.base + a + b) genBindings() diff --git a/tests/unit/test_c_abi_codegen.nim b/tests/unit/test_c_abi_codegen.nim index c8a5896..3383ed7 100644 --- a/tests/unit/test_c_abi_codegen.nim +++ b/tests/unit/test_c_abi_codegen.nim @@ -161,11 +161,8 @@ suite "generateCAbiLibHeader": check "return timer_add(ctx->ptr, timer_add_scalar_reply, box, a, b);" in header test "scalar returns unpack the 8-byte image; strings copy through the dup helper": - # int return: the slot is the sign-extended int64 image. check "memcpy(&reply, &slot, sizeof(reply));" in header - # float32 return: packed as a widened double, narrowed back here. check "float reply = (float)wide;" in header - # string return: copied via the overflow-guarded, NUL-terminating dup helper. check "char* reply = nimffi_abi_dup_cstr_n(msg ? msg : \"\", msg ? len : 0);" in header check "if (n == SIZE_MAX) return NULL;" in header diff --git a/tests/unit/test_scalar_skip_gen.nim b/tests/unit/test_scalar_skip_gen.nim index 5e6164e..8b853e8 100644 --- a/tests/unit/test_scalar_skip_gen.nim +++ b/tests/unit/test_scalar_skip_gen.nim @@ -1,12 +1,7 @@ -## Drives the scalar-fast-path drop error end to end: compiles -## `fixtures/scalar_skip_fixture.nim` (a CBOR-default library with a stray -## all-scalar `abi = c` proc) with `-d:ffiGenBindings` and asserts genBindings() -## fails loudly, and that `-d:ffiAllowScalarSkip` downgrades the drop to a clean -## build. The positive path — the `abi = c` C header emitting a real scalar -## binding — is covered by test_c_abi_codegen and the echo c_abi e2e. +## Asserts genBindings() fails loudly on a scalar `abi = c` proc no target can +## bind, and that -d:ffiAllowScalarSkip downgrades it to a clean build. ## -## The fixture is compiled in a child `nim check` (search paths and compiler -## captured at compile time) so its expected failure is observed as a test +## The fixture compiles in a child `nim check` so its expected failure is a test ## assertion, not this file's own compile error. import std/[os, osproc, strutils, compilesettings]