From 22e69a2d32280c36fa664a6a4081a4d26599e002 Mon Sep 17 00:00:00 2001 From: Gabriel Cruz Date: Mon, 6 Jul 2026 11:11:07 -0300 Subject: [PATCH] fix: pr comments --- examples/echo/c_abi_bindings/echo.h | 14 +------ ffi/codegen/c_abi.nim | 57 +++++++++++++---------------- ffi/internal/c_macro_helpers.nim | 45 +++++++++++++---------- ffi/internal/ffi_macro.nim | 3 +- tests/e2e/c_abi/test_echo_c_abi.c | 29 ++++----------- 5 files changed, 60 insertions(+), 88 deletions(-) diff --git a/examples/echo/c_abi_bindings/echo.h b/examples/echo/c_abi_bindings/echo.h index bd33544..8b9bfdd 100644 --- a/examples/echo/c_abi_bindings/echo.h +++ b/examples/echo/c_abi_bindings/echo.h @@ -6,11 +6,9 @@ #include #include -#ifndef NIMFFI_RET_OK #define NIMFFI_RET_OK 0 #define NIMFFI_RET_ERR 1 #define NIMFFI_RET_MISSING_CALLBACK 2 -#endif /* Flat wire structs — the C ABI. Strings are borrowed, NUL-terminated `const char*` valid only for the duration of the call they cross. */ @@ -30,12 +28,8 @@ typedef struct { typedef struct { ShoutRequest req; } EchoShoutReq; -typedef struct { - uint8_t _placeholder; /* C forbids empty structs */ -} EchoVersionReq; typedef void (*EchoShoutReplyFn)(int err_code, const ShoutResponse* reply, const char* err_msg, void* user_data); -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); #ifdef __cplusplus @@ -44,7 +38,6 @@ extern "C" { void* echo_create(const EchoCreateCtorReq* req, EchoCreateRawFn on_created, void* user_data); int echo_shout(void* ctx, EchoShoutReplyFn on_reply, void* user_data, const EchoShoutReq* req); -int echo_version(void* ctx, EchoVersionReplyFn on_reply, void* user_data, const EchoVersionReq* req); int echo_destroy(void* ctx); #ifdef __cplusplus @@ -60,6 +53,7 @@ typedef void (*EchoCreateFn)(int err_code, EchoCtx* ctx, const char* err_msg, vo typedef struct { EchoCreateFn fn; void* user_data; } EchoCreateBox; static void echo_create_trampoline(int ret, const char* ctx_addr, const char* err_msg, void* ud) { EchoCreateBox* box = (EchoCreateBox*)ud; + if (!box) return; if (!box->fn) { free(box); return; } if (ret != 0) { box->fn(ret, NULL, err_msg ? err_msg : "FFI create failed", box->user_data); @@ -113,10 +107,4 @@ static inline int echo_ctx_shout(const EchoCtx* ctx, const ShoutRequest* req, Ec return echo_shout(ctx->ptr, on_reply, user_data, &ffi_req); } -static inline int echo_ctx_version(const EchoCtx* ctx, EchoVersionReplyFn on_reply, void* user_data) { - EchoVersionReq ffi_req; - memset(&ffi_req, 0, sizeof(ffi_req)); - return echo_version(ctx->ptr, on_reply, user_data, &ffi_req); -} - #endif /* NIM_FFI_LIB_ECHO_C_ABI_H_INCLUDED */ diff --git a/ffi/codegen/c_abi.nim b/ffi/codegen/c_abi.nim index 1cf3a93..b58bf42 100644 --- a/ffi/codegen/c_abi.nim +++ b/ffi/codegen/c_abi.nim @@ -143,6 +143,28 @@ func paramByValue(nimType: string, ridesAsPtr: bool): bool = return true leafCTypeAbi(nimType.strip()).ok +proc reqParamsAndAssigns( + reg: var AbiReg, extraParams: seq[FFIParamMeta] +): tuple[params, assigns: seq[string]] = + ## The C parameter list + `ffi_req` field assignments shared by the ctor and + ## method wrappers: by-value params copy straight into the request struct, + ## by-const-pointer aggregates are dereferenced in. + var params, assigns: seq[string] = @[] + for ep in extraParams: + let rides = ep.ridesAsPtr() + let cType = + if rides: + CPtrType + else: + wireValueCType(reg, ep.typeName) + if paramByValue(ep.typeName, rides): + params.add(cType & " " & ep.name) + assigns.add(" ffi_req." & ep.name & " = " & ep.name & ";") + else: + params.add("const " & cType & "* " & ep.name) + assigns.add(" ffi_req." & ep.name & " = *" & ep.name & ";") + (params, assigns) + proc methodReplyInfo( reg: var AbiReg, libType: string, m: FFIProcMeta ): tuple[fnType, replyParam: string] = @@ -248,6 +270,7 @@ proc emitCtxAndCtor( "(int ret, const char* ctx_addr, const char* err_msg, void* ud) {" ) lines.add(" " & createBox & "* box = (" & createBox & "*)ud;") + lines.add(" if (!box) return;") lines.add(" if (!box->fn) { free(box); return; }") lines.add(" if (ret != 0) {") lines.add( @@ -281,21 +304,7 @@ proc emitCtxAndCtor( lines.add("") for ctor in ctors: let reqStruct = reqStructName(ctor) - var params: seq[string] = @[] - var assigns: seq[string] = @[] - for ep in ctor.extraParams: - let rides = ep.ridesAsPtr() - let cType = - if rides: - CPtrType - else: - wireValueCType(reg, ep.typeName) - if paramByValue(ep.typeName, rides): - params.add(cType & " " & ep.name) - assigns.add(" ffi_req." & ep.name & " = " & ep.name & ";") - else: - params.add("const " & cType & "* " & ep.name) - assigns.add(" ffi_req." & ep.name & " = *" & ep.name & ";") + let (params, assigns) = reqParamsAndAssigns(reg, ctor.extraParams) let head = "static inline int " & libName & "_ctx_create(" let sig = if params.len > 0: @@ -342,21 +351,7 @@ proc emitMethod( let stripped = stripLibPrefix(m.procName, m.libName) let reqStruct = reqStructName(m) let info = methodReplyInfo(reg, libType, m) - var params: seq[string] = @[] - var assigns: seq[string] = @[] - for ep in m.extraParams: - let rides = ep.ridesAsPtr() - let cType = - if rides: - CPtrType - else: - wireValueCType(reg, ep.typeName) - if paramByValue(ep.typeName, rides): - params.add(cType & " " & ep.name) - assigns.add(" ffi_req." & ep.name & " = " & ep.name & ";") - else: - params.add("const " & cType & "* " & ep.name) - assigns.add(" ffi_req." & ep.name & " = *" & ep.name & ";") + let (params, assigns) = reqParamsAndAssigns(reg, m.extraParams) let head = "static inline int " & libName & "_ctx_" & stripped & "(const " & ctxType & "* ctx, " let sig = @@ -404,11 +399,9 @@ proc generateCAbiLibHeader*( lines.add("#include ") lines.add("#include ") lines.add("") - lines.add("#ifndef NIMFFI_RET_OK") lines.add("#define NIMFFI_RET_OK 0") lines.add("#define NIMFFI_RET_ERR 1") lines.add("#define NIMFFI_RET_MISSING_CALLBACK 2") - lines.add("#endif") lines.add("") lines.add("/* Flat wire structs — the C ABI. Strings are borrowed, NUL-terminated") lines.add(" `const char*` valid only for the duration of the call they cross. */") diff --git a/ffi/internal/c_macro_helpers.nim b/ffi/internal/c_macro_helpers.nim index f88e6a3..7d4d312 100644 --- a/ffi/internal/c_macro_helpers.nim +++ b/ffi/internal/c_macro_helpers.nim @@ -632,9 +632,6 @@ proc registerCAbiCtor*( ) ) -proc isStringResp(t: NimNode): bool = - t.kind == nnkIdent and ($t == "string" or $t == "cstring") - proc cdeclReplyPragma(): NimNode = nnkPragma.newTree( ident("cdecl"), @@ -683,7 +680,8 @@ proc objectTrampBody(boxName, respType, respWire: NimNode): NimNode = ## Reply trampoline for an object return: recover the box, deliver a transport ## error as a copied NUL-terminated string, else CBOR-decode the reply, ## `cwirePack` it into the flat wire struct, hand a pointer to the caller, and - ## release the wire. + ## release the wire. `err_msg` is always a non-nil string; the `reply` struct + ## pointer is nil only on error, gated by a non-`RET_OK` `err_code`. quote: let box = cast[ptr `boxName`](ud) if box.isNil(): @@ -706,7 +704,7 @@ proc objectTrampBody(boxName, respType, respWire: NimNode): NimNode = else: var wire: `respWire` cwirePack(wire, decoded.get()) - box.fn(RET_OK, addr wire, nil, box.ud) + box.fn(RET_OK, addr wire, "".cstring, box.ud) cwireFree(wire) except CatchableError as e: box.fn(RET_ERR, nil, e.msg.cstring, box.ud) @@ -714,7 +712,9 @@ proc objectTrampBody(boxName, respType, respWire: NimNode): NimNode = proc stringTrampBody(boxName: NimNode): NimNode = ## Reply trampoline for a `string` return (and the ctor's address string): ## CBOR-decode the reply into a Nim string and hand its (NUL-terminated) - ## `cstring` to the caller for the duration of the call. + ## `cstring` to the caller for the duration of the call. Reply and error + ## strings are always non-nil empty strings on the paths they don't apply to, + ## so a consumer can `strlen`/print either unconditionally without a nil deref. quote: let box = cast[ptr `boxName`](ud) if box.isNil(): @@ -728,16 +728,16 @@ proc stringTrampBody(boxName: NimNode): NimNode = var em = newString(int(len)) if int(len) > 0: copyMem(addr em[0], msg, int(len)) - box.fn(ret, nil, em.cstring, box.ud) + box.fn(ret, "".cstring, em.cstring, box.ud) return let decoded = cborDecodePtr(cast[ptr UncheckedArray[byte]](msg), int(len), string) if decoded.isErr(): - box.fn(RET_ERR, nil, decoded.error.cstring, box.ud) + box.fn(RET_ERR, "".cstring, decoded.error.cstring, box.ud) else: let replyStr = decoded.get() - box.fn(RET_OK, replyStr.cstring, nil, box.ud) + box.fn(RET_OK, replyStr.cstring, "".cstring, box.ud) except CatchableError as e: - box.fn(RET_ERR, nil, e.msg.cstring, box.ud) + box.fn(RET_ERR, "".cstring, e.msg.cstring, box.ud) proc exportedMethodProc( spec: CAbiSpec, boxName, envWire, trampName, poolIdent, cbType: NimNode @@ -751,11 +751,18 @@ proc exportedMethodProc( let envName = spec.envelope let libFFICtx = nnkPtrTy.newTree(nnkBracketExpr.newTree(ident("FFIContext"), spec.libType)) + # A string reply is an empty (non-nil) cstring on the error path, matching the + # trampoline; an object reply is a nil struct pointer gated by `err_code`. + let emptyReply = + if isStringType(spec.respType): + newDotExpr(newLit(""), ident("cstring")) + else: + newNilLit() let body = quote: if onReply.isNil(): return RET_MISSING_CALLBACK if not `poolIdent`.isValidCtx(cast[pointer](ctx)): - onReply(RET_ERR, nil, "ctx is not a valid FFI context".cstring, userData) + onReply(RET_ERR, `emptyReply`, "ctx is not a valid FFI context".cstring, userData) return RET_ERR var reqObj: `envName` = cwireUnpack(req[]) let enc = cborEncodeShared(reqObj) @@ -771,10 +778,10 @@ proc exportedMethodProc( let sendRes = try: ffi_context.sendRequestToFFIThread(ctx, reqPtr) - except Exception as exc: - Result[void, string].err("sendRequestToFFIThread exception: " & exc.msg) + except Exception as e: + Result[void, string].err("sendRequestToFFIThread exception: " & e.msg) if sendRes.isErr(): - onReply(RET_ERR, nil, sendRes.error.cstring, userData) + onReply(RET_ERR, `emptyReply`, sendRes.error.cstring, userData) return RET_ERR return RET_OK newProc( @@ -815,7 +822,7 @@ proc exportedCtorProc( if not onCreated.isNil(): onCreated( RET_ERR, - nil, + "".cstring, ("ffiCtor: failed to create FFIContext: " & $ctxRes.error).cstring, userData, ) @@ -835,11 +842,11 @@ proc exportedCtorProc( let sendRes = try: ctx.sendRequestToFFIThread(reqPtr) - except Exception as exc: - Result[void, string].err("sendRequestToFFIThread exception: " & exc.msg) + except Exception as e: + Result[void, string].err("sendRequestToFFIThread exception: " & e.msg) if sendRes.isErr(): if not onCreated.isNil(): - onCreated(RET_ERR, nil, sendRes.error.cstring, userData) + onCreated(RET_ERR, "".cstring, sendRes.error.cstring, userData) return nil return cast[pointer](ctx) body.insert(0, initGuard) @@ -899,7 +906,7 @@ proc flushCAbiDispatch*(): NimNode {.compileTime.} = sink.add(exportedCtorProc(spec, boxName, envWire, trampName, poolIdent, cbType)) of cakMethod: let rt = spec.respType - if isStringResp(rt): + if isStringType(rt): let cbType = cAbiCbType(ident("cstring")) sink.add(boxTypeDef(boxName, cbType)) sink.add(replyTrampProc(trampName, stringTrampBody(boxName))) diff --git a/ffi/internal/ffi_macro.nim b/ffi/internal/ffi_macro.nim index fb819d4..69be972 100644 --- a/ffi/internal/ffi_macro.nim +++ b/ffi/internal/ffi_macro.nim @@ -1871,8 +1871,7 @@ macro genBindings*( ) of "c_abi": generateCAbiBindings( - ffiProcRegistry, ffiTypeRegistry, libName, outputDir, nimSrcRelPath, - ffiEventRegistry, + genProcs, ffiTypeRegistry, libName, outputDir, nimSrcRelPath, ffiEventRegistry ) of "cddl": generateCddlBindings(genProcs, ffiTypeRegistry, libName, outputDir, nimSrcRelPath) diff --git a/tests/e2e/c_abi/test_echo_c_abi.c b/tests/e2e/c_abi/test_echo_c_abi.c index a880279..87d6f97 100644 --- a/tests/e2e/c_abi/test_echo_c_abi.c +++ b/tests/e2e/c_abi/test_echo_c_abi.c @@ -1,10 +1,13 @@ /* 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 string-returning method, the error channel 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. */ + * async, callback-per-call surface — constructor, an object-returning 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. A string-returning method (echoVersion) rides the CBOR-free scalar + * fast path instead of a flat `_CWire` wrapper, so it has no c_abi binding yet + * (foreign codegen for the scalar shape is a follow-up) and isn't exercised + * here. */ #include "echo.h" #include #include @@ -87,27 +90,9 @@ static void test_shout(EchoCtx* ctx) { assert(strcmp(w.text_b, "c-abi") == 0); } -static void on_version(int ec, const char* reply, const char* em, void* ud) { - ReplyWaiter* w = (ReplyWaiter*)ud; - w->err_code = ec; - if (reply) snprintf(w->text_a, sizeof(w->text_a), "%s", reply); - if (em) snprintf(w->err, sizeof(w->err), "%s", em); - atomic_store_explicit(&w->done, 1, memory_order_release); -} - -static void test_version(EchoCtx* ctx) { - ReplyWaiter w; - memset(&w, 0, sizeof(w)); - echo_ctx_version(ctx, on_version, &w); - wait_done(&w.done); - assert(w.err_code == 0); - assert(strcmp(w.text_a, "nim-echo v0.1.0") == 0); -} - int main(void) { EchoCtx* ctx = make_ctx(); test_shout(ctx); - test_version(ctx); echo_ctx_destroy(ctx); printf("all abi=c echo e2e checks passed\n"); return 0;