diff --git a/ffi/codegen/meta.nim b/ffi/codegen/meta.nim index f20e298..8734442 100644 --- a/ffi/codegen/meta.nim +++ b/ffi/codegen/meta.nim @@ -33,9 +33,9 @@ type abiFormat*: ABIFormat # wire format for this interaction (default Cbor) scalarFastPath*: bool ## True for an `abi = c` proc whose whole signature is scalar (see - ## `isScalarOnly`): it dispatches through the CBOR-free scalar fast path - ## and is skipped by the foreign-binding generators (no dispatch codegen - ## yet — the request rides inline POD args, no `_CWire`, no CBOR). + ## `isScalarOnly`): dispatches through the CBOR-free scalar fast path and + ## is skipped by the foreign-binding generators (their codegen is a + ## follow-up). FFIFieldMeta* = object name*: string # e.g. "delayMs" diff --git a/ffi/ffi_thread_request.nim b/ffi/ffi_thread_request.nim index 9d0e863..bd7d3c6 100644 --- a/ffi/ffi_thread_request.nim +++ b/ffi/ffi_thread_request.nim @@ -30,11 +30,14 @@ type FFIThreadRequest* = object reqId*: cstring ## Per-proc Req type name used to look up the handler. data*: ptr UncheckedArray[byte] ## Owned CBOR-encoded request payload. dataLen*: int + isScalar*: bool + ## Set by `initScalar`: the payload rode inline in `scalarArgs` (no CBOR, + ## no `data` buffer). Lets `handleRes` tell a scalar 0-length return (a real + ## empty string) from a CBOR "no value". scalarArgs*: array[MaxScalarArgs, uint64] - ## Scalar-fast-path args inlined in the envelope (one `ffiPackScalar` value - ## per slot) so there's no per-call `c_malloc`. A plain array rather than a - ## `union` with `data`: costs a fixed 64 bytes per request but keeps the - ## `next` link and `deleteRequest` unaliased and branch-free. + ## Scalar-fast-path args inlined in the envelope so there's no per-call + ## `c_malloc`. A plain array rather than a `union` with `data`: costs a fixed + ## 64 bytes per request but keeps `deleteRequest` unaliased and branch-free. next*: ptr FFIThreadRequest ## Intrusive ingress-queue link (see `ffi_request_queue.nim`). Touched only ## under the queue's lock; the request doubles as its own node, so no @@ -79,6 +82,7 @@ proc allocBaseRequest( ret[].reqId = reqId.alloc() ret[].data = nil ret[].dataLen = 0 + ret[].isScalar = false ret[].next = nil ret[].responded = false return ret @@ -169,17 +173,17 @@ proc initScalar*( "initScalar: " & $args.len & " scalar args exceed MaxScalarArgs (" & $MaxScalarArgs & ")" var ret = allocBaseRequest(callback, userData, reqId) + ret[].isScalar = true for i in 0 ..< args.len: ret[].scalarArgs[i] = args[i] ret func ffiScalarRetBytes*[T](x: T): seq[byte] = - ## Serializes a scalar handler result into the raw response payload the - ## callback carries — no CBOR envelope. A `string`/`cstring` rides as its - ## own UTF-8 bytes (like the error path); every other scalar rides as the - ## 8-byte native-endian image of `ffiPackScalar(x)`. Note: an empty string - ## yields a 0-length payload, which `handleRes` sends as the CBOR-null - ## sentinel — the foreign scalar reader (a follow-up) must special-case it. + ## Serializes a scalar handler result into the raw response payload — no CBOR + ## envelope. A `string`/`cstring` rides as its own UTF-8 bytes (like the error + ## path); every other scalar rides as the 8-byte native-endian image of + ## `ffiPackScalar(x)`. An empty string yields a 0-length payload (see + ## `handleRes`, which delivers it as `""` rather than the CBOR-null sentinel). when T is string: var b = newSeq[byte](x.len) if x.len > 0: @@ -231,6 +235,15 @@ proc fireCallback*(res: Result[seq[byte], string], request: ptr FFIThreadRequest cast[csize_t](bytes.len), request[].userData, ) + elif request[].isScalar: + # `isScalar` marks a scalar-fast-path request (args rode inline in + # `scalarArgs`, no CBOR): its result bytes come from `ffiScalarRetBytes`, + # not a CBOR encoder. So a 0-byte return is a real empty string, not + # "no value" — hand back a genuine empty buffer, not the CBOR-null sentinel. + var empty: byte + request[].callback( + RET_OK, cast[ptr cchar](addr empty), 0.csize_t, request[].userData + ) else: # Always hand the callback a real buffer; CBOR null marks "no value". var sentinel = CborNullByte diff --git a/ffi/internal/ffi_macro.nim b/ffi/internal/ffi_macro.nim index 3644ce4..6b4b675 100644 --- a/ffi/internal/ffi_macro.nim +++ b/ffi/internal/ffi_macro.nim @@ -954,10 +954,9 @@ macro ffi*(args: varargs[untyped]): untyped = abiFormat: abiFormat, ) - # Does this proc qualify for the CBOR-free scalar fast path? Only `abi = c` - # opts in, and only when every wire param + the return is a plain scalar - # (see `isScalarOnly`) and the args fit the inline slots. An `abi = c` proc - # that isn't scalar-only stays gated — full C-wire dispatch is a follow-up. + # Qualifies for the CBOR-free fast path only if `abi = c`, every wire param + + # return is scalar (`isScalarOnly`), and the args fit the inline slots. A + # non-scalar `abi = c` proc stays gated — full C-wire dispatch is a follow-up. let scalarEligible = abiFormat == ABIFormat.C and isScalarOnly(procMeta) and extraParamNames.len <= MaxScalarArgs @@ -1821,6 +1820,12 @@ macro genBindings*( # export doesn't take the CBOR `(reqCbor, reqCborLen)` shape); drop them so # the generators don't emit a broken CBOR caller for them. let genProcs = bindableProcs(ffiProcRegistry) + for p in ffiProcRegistry: + if p.scalarFastPath: + hint( + "genBindings: skipping scalar-fast-path proc '" & p.procName & + "' (no foreign-binding codegen for the scalar shape yet)" + ) case lang of "rust": generateRustCrate( @@ -1832,8 +1837,7 @@ macro genBindings*( ) of "c": generateCBindings( - ffiProcRegistry, ffiTypeRegistry, libName, outputDir, nimSrcRelPath, - ffiEventRegistry, + genProcs, ffiTypeRegistry, libName, outputDir, nimSrcRelPath, ffiEventRegistry ) of "cddl": generateCddlBindings(genProcs, ffiTypeRegistry, libName, outputDir, nimSrcRelPath) diff --git a/ffi/internal/ffi_scalar.nim b/ffi/internal/ffi_scalar.nim index a5cded5..bc7f1e7 100644 --- a/ffi/internal/ffi_scalar.nim +++ b/ffi/internal/ffi_scalar.nim @@ -17,11 +17,10 @@ const scalarPodTypeNames = [ "int", "int8", "int16", "int32", "int64", "uint", "uint8", "uint16", "uint32", "uint64", "byte", "float", "float32", "float64", "bool", ] - ## Fixed-width POD scalars that fit a single `uint64` slot and survive the - ## async hop by value — the payload the scalar fast path inlines into the - ## request (no heap copy). `cstring`/`string` are intentionally absent as - ## *params*: they are pointers to caller memory the FFI thread reads later, - ## so they'd need a copy, defeating the zero-alloc promise. + ## Fixed-width POD scalars that fit one `uint64` slot and survive the async + ## hop by value. `cstring`/`string` are intentionally absent as *params*: + ## they point to caller memory the FFI thread reads after the call returns, + ## so passing them inline by value would be unsafe. func isScalarParamTypeName*(name: string): bool = ## A param type eligible for the CBOR-free scalar fast path. @@ -37,7 +36,7 @@ func isScalarOnly*(p: FFIProcMeta): bool = ## True iff `p` is a plain `{.ffi.}` method whose every wire param and return ## is scalar — the whole signature crosses without CBOR or `_CWire`. Handles ## and raw pointers are excluded (a handle needs a ctx-registry round-trip; - ## a pointer never crosses). Pure over the compile-time metadata. + ## a pointer never crosses). if p.kind != FFIKind.FFI: return false if p.returnIsPtr or p.returnIsHandle: @@ -99,7 +98,7 @@ proc buildScalarPath*( let retValIdent = genSym(nskLet, "retVal") handlerBody.add quote do: let `retValIdent` = (await `helperCall`).valueOr: - return err($error) + return err(error) return ok(ffiScalarRetBytes(`retValIdent`)) let seqByteResult = nnkBracketExpr.newTree( diff --git a/tests/unit/test_c_codegen.nim b/tests/unit/test_c_codegen.nim index f77917d..f6bdeed 100644 --- a/tests/unit/test_c_codegen.nim +++ b/tests/unit/test_c_codegen.nim @@ -3,9 +3,10 @@ ## pipeline, no files written) and asserts on the emitted text — the same ## approach as test_cddl_codegen. -import std/strutils +import std/[strutils, sequtils] import unittest2 import ffi/codegen/[meta, c] +import ffi/internal/ffi_scalar proc field(n, t: string): FFIFieldMeta = FFIFieldMeta(name: n, typeName: t) @@ -210,6 +211,60 @@ suite "generateCLibHeader: no-event libraries stay lean": check "listeners_len" notin header check "_add_event_listener" in header # raw ABI symbol is always declared +suite "generateCLibHeader: scalar-fast-path procs are excluded": + setup: + let procs = @[ + FFIProcMeta( + procName: "calc_create", + libName: "calc", + kind: FFIKind.CTOR, + libTypeName: "Calc", + returnTypeName: "Calc", + ), + FFIProcMeta( + procName: "calc_echo", + libName: "calc", + kind: FFIKind.FFI, + libTypeName: "Calc", + extraParams: @[param("req", "EchoRequest")], + returnTypeName: "EchoResponse", + ), + FFIProcMeta( + procName: "calc_add", + libName: "calc", + kind: FFIKind.FFI, + libTypeName: "Calc", + extraParams: @[param("a", "int"), param("b", "int")], + returnTypeName: "int", + abiFormat: ABIFormat.C, + scalarFastPath: true, + ), + ] + let types = @[ + FFITypeMeta(name: "EchoRequest", fields: @[field("m", "string")]), + FFITypeMeta(name: "EchoResponse", fields: @[field("echoed", "string")]), + ] + + test "bindableProcs keeps the CBOR procs and drops the scalar one": + let kept = bindableProcs(procs) + check kept.anyIt(it.procName == "calc_create") + check kept.anyIt(it.procName == "calc_echo") + check not kept.anyIt(it.procName == "calc_add") + + test "the C header emitted from the bindable set carries no scalar symbol": + let header = generateCLibHeader(bindableProcs(procs), types, "calc") + check "int calc_echo(void* ctx, FFICallback callback" in header + check "int calc_add(" notin header # note: calc_add_event_listener is unrelated + + test "unfiltered, the generator would emit a wrong-ABI CBOR caller for it": + # Regression guard for the bug bindableProcs prevents: handed the raw + # registry, the C generator declares calc_add with the CBOR + # (req_cbor, req_cbor_len) prototype, which mismatches its real inline-arg + # export. genBindings must feed the filtered set (see the `of "c":` branch). + let header = generateCLibHeader(procs, types, "calc") + check "int calc_add(void* ctx, FFICallback callback, void* user_data, " & + "const uint8_t* req_cbor, size_t req_cbor_len);" in header + suite "shared headers: prelude and cbor split": test "the prelude owns the leaf types and libc/TinyCBOR includes": let prelude = generateCPreludeHeader() diff --git a/tests/unit/test_scalar_fastpath.nim b/tests/unit/test_scalar_fastpath.nim index 750054b..8a4a47c 100644 --- a/tests/unit/test_scalar_fastpath.nim +++ b/tests/unit/test_scalar_fastpath.nim @@ -61,7 +61,14 @@ proc scalarfast_checked*( return err("negative not allowed") return ok(n * 2) -## --- C-shape callback harness (mirrors test_ffi_context.nim) ---------------- +proc scalarfast_blank*( + lib: ScalarLib +): Future[Result[string, string]] {.ffi: "abi = c".} = + ## Empty-string return — must ride back as a genuine 0-length RET_OK payload, + ## not the CBOR-null sentinel. + return ok("") + +## C-shape callback harness (mirrors test_ffi_context.nim). type CallbackData = object lock: Lock @@ -122,10 +129,7 @@ proc scalarStr(d: CallbackData): string = s proc callbackErr(d: CallbackData): string = - var msg = newString(d.msgLen) - if d.msgLen > 0: - copyMem(addr msg[0], unsafeAddr d.msg[0], d.msgLen) - msg + scalarStr(d) proc encodedPtr(bytes: var seq[byte]): ptr byte = if bytes.len == 0: @@ -183,6 +187,22 @@ suite "scalar fast path — C export shape": check d.retCode == RET_OK check scalarStr(d) == "scalarfast v1" + test "empty string return rides back as a real 0-length RET_OK payload": + let ctx = makeCtx(0) + defer: + check ScalarLibFFIPool.destroyFFIContext(ctx).isOk() + + var d: CallbackData + initCallbackData(d) + defer: + deinitCallbackData(d) + + check scalarfast_blank(ctx, testCallback, addr d) == RET_OK + waitCallback(d) + check d.retCode == RET_OK + check d.msgLen == 0 # NOT 1 (would be the 0xf6 sentinel) + check scalarStr(d) == "" + test "float param round-trips through the uint64 slot": let ctx = makeCtx(0) defer: @@ -250,13 +270,17 @@ suite "scalar fast path — Nim-native shape": check bad.isErr() check bad.error == "negative not allowed" + let blank = waitFor scalarfast_blank(ScalarLib(base: 0)) + check blank.isOk() + check blank.value == "" + # `ffiProcRegistry` is a compile-time var, so its assertions run in a static # block (mirrors test_abi_format.nim). A scalar-only `abi = c` proc must be # flagged, recognised by `isScalarOnly`, and dropped from `bindableProcs`. static: const scalarNames = [ "scalarfast_add", "scalarfast_version", "scalarfast_scale", "scalarfast_positive", - "scalarfast_checked", + "scalarfast_checked", "scalarfast_blank", ] var seen = 0 for p in ffiProcRegistry: