From 10edaa3b742fce34215204d9850dc022e2fa292d Mon Sep 17 00:00:00 2001 From: Fabiana Cecin Date: Wed, 5 Aug 2026 01:01:13 -0300 Subject: [PATCH] fix(c): register the calling thread with the GC on abi=c method entry --- .github/workflows/ci.yml | 9 + ffi/internal/c_macro_helpers.nim | 30 +-- .../fixtures/foreign_thread_c_abi_fixture.nim | 210 ++++++++++++++++++ tests/unit/test_foreign_thread_c_abi.nim | 43 ++++ 4 files changed, 279 insertions(+), 13 deletions(-) create mode 100644 tests/unit/fixtures/foreign_thread_c_abi_fixture.nim create mode 100644 tests/unit/test_foreign_thread_c_abi.nim diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f6587a5..917c575 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -80,6 +80,15 @@ jobs: nim-versions: ${{ needs.versions.outputs.nim-versions }} nimble-version: ${{ needs.versions.outputs.nimble }} + foreign-thread-c-abi: + name: Foreign Thread (abi = c) + needs: versions + uses: ./.github/workflows/test.yml + with: + test: test_foreign_thread_c_abi + nim-versions: ${{ needs.versions.outputs.nim-versions }} + nimble-version: ${{ needs.versions.outputs.nimble }} + c-wire: # The `c`-ABI cwire codec is layout-/allocator-sensitive (malloc/free, flat # struct packing, allocShared for seq/Option), so cover it across the full diff --git a/ffi/internal/c_macro_helpers.nim b/ffi/internal/c_macro_helpers.nim index a134f71..960aeb0 100644 --- a/ffi/internal/c_macro_helpers.nim +++ b/ffi/internal/c_macro_helpers.nim @@ -711,8 +711,18 @@ proc ctxBindingGuard( ): NimNode {.compileTime.} = ## Prologue that binds `ctxIdent`: a method validates the ctx it was handed, a ## static resolves the library's shared one. + # + # Any call can be a host thread's first entry. The body allocates via the GC + # on the calling thread, so register it first; initializeLibrary is idempotent. + # Raw AST: `when declared` of an undeclared symbol inside `quote` ICEs. + let initGuard = nnkWhenStmt.newTree( + nnkElifBranch.newTree( + newCall(ident("declared"), ident("initializeLibrary")), + newStmtList(newCall(ident("initializeLibrary"))), + ) + ) if not isStatic: - return quote: + let methodGuard = quote: if onReply.isNil(): return RET_MISSING_CALLBACK if not `poolIdent`.isValidCtx(cast[pointer](`ctxIdent`)): @@ -720,6 +730,8 @@ proc ctxBindingGuard( RET_ERR, `emptyReply`, "ctx is not a valid FFI context".cstring, userData ) return RET_ERR + methodGuard.insert(0, initGuard) + return methodGuard let guard = quote: if onReply.isNil(): return RET_MISSING_CALLBACK @@ -727,17 +739,7 @@ proc ctxBindingGuard( let errStr = "ffiStatic: " & error onReply(RET_ERR, `emptyReply`, errStr.cstring, userData) return RET_ERR - # A static call may be the host's first entry into the library. Raw AST, not - # `quote`: `when declared` over an undeclared symbol inside `quote` ICEs. - guard.insert( - 0, - nnkWhenStmt.newTree( - nnkElifBranch.newTree( - newCall(ident("declared"), ident("initializeLibrary")), - newStmtList(newCall(ident("initializeLibrary"))), - ) - ), - ) + guard.insert(0, initGuard) guard proc exportedProc( @@ -745,7 +747,9 @@ proc exportedProc( boxName, envWire, trampName, poolIdent, cbType: NimNode, isStatic: bool, ): NimNode = - # No `foreignThreadGc`: `cwireUnpack`/`cwirePack` alloc on the calling thread (already GC-registered); wrapping would free its live ORC heap. + # `cwireUnpack`/`cwirePack` alloc on the calling thread; `ctxBindingGuard` + # registered it. No teardown: it would free the heap of a host thread still + # calling in. A host thread that exits leaks its heap; accepted. let envName = spec.envelope let ctxIdent = ident("ctx") # String reply: empty non-nil cstring on error; object reply: nil ptr gated by err_code. diff --git a/tests/unit/fixtures/foreign_thread_c_abi_fixture.nim b/tests/unit/fixtures/foreign_thread_c_abi_fixture.nim new file mode 100644 index 0000000..1afe75a --- /dev/null +++ b/tests/unit/fixtures/foreign_thread_c_abi_fixture.nim @@ -0,0 +1,210 @@ +## Fixture for test_foreign_thread_c_abi. It calls `abi = c` method entry +## points from threads that the Nim runtime does not know. + +import std/[locks, strutils] +import results +import ffi + +type ThreadLib = object + tag: string + +# declareLibrary imports the NimMain symbol of the dylib. This fixture links as +# an executable, so it must supply an empty stub. +{.emit: "void libthreadedcabiNimMain(void) {}".} + +declareLibrary("threadedcabi", ThreadLib, defaultABIFormat = "c") + +type ThreadConfig {.ffi.} = object + tag: string + +proc threadedcabi_create*( + cfg: ThreadConfig +): Future[Result[ThreadLib, string]] {.ffiCtor.} = + return ok(ThreadLib(tag: cfg.tag)) + +proc threadedcabi_echo*( + lib: ThreadLib, text: string +): Future[Result[string, string]] {.ffi.} = + ## The method takes a string, so the request unpack allocates GC-managed + ## memory on the calling thread. That is the operation under test. + return ok(lib.tag & ":" & text) + +genBindings() + +type ReplyData = object + lock: Lock + cond: Cond + called: bool + retCode: cint + text: string + +proc initReplyData(d: var ReplyData) = + d.lock.initLock() + d.cond.initCond() + +proc deinitReplyData(d: var ReplyData) = + d.cond.deinitCond() + d.lock.deinitLock() + +proc waitReply(d: var ReplyData) = + acquire(d.lock) + while not d.called: + wait(d.cond, d.lock) + release(d.lock) + +proc onStringReply( + err: cint, reply: cstring, errMsg: cstring, ud: pointer +) {.cdecl, gcsafe, raises: [].} = + let d = cast[ptr ReplyData](ud) + acquire(d[].lock) + if err == RET_OK and not reply.isNil(): + d[].text = $reply + d[].retCode = err + d[].called = true + signal(d[].cond) + release(d[].lock) + +proc packedWire[W, R](_: typedesc[W], envelope: R): W = + var wire: W + cwirePack(wire, envelope) + wire + +proc makeCtx(tag: string): ptr FFIContext[ThreadLib] = + var d: ReplyData + initReplyData(d) + defer: + deinitReplyData(d) + + var wire = packedWire( + ThreadedcabiCreateCtorReq_CWire, + ThreadedcabiCreateCtorReq(cfg: ThreadConfig(tag: tag)), + ) + defer: + cwireFree(wire) + + doAssert not ThreadedcabiCreateCtorReqCAbiExport(addr wire, onStringReply, addr d) + .isNil() + waitReply(d) + doAssert d.retCode == RET_OK + # The ctor's reply text is the new ctx pointer as a decimal string. + cast[ptr FFIContext[ThreadLib]](cast[uint](parseBiggestUInt(d.text))) + +# Nim's createThread registers its new thread with the GC. A registered thread +# cannot show the bug. The test thread must come from the platform API. +{. + emit: """ +typedef int (*NimFfiEchoFn)(void*, void*, void*, const void*); + +typedef struct { + void* fn; void* ctx; void* cb; void* ud; const void* req; int ret; +} NimFfiForeignCall; + +static void nimffi_foreign_body(NimFfiForeignCall* c) { + c->ret = ((NimFfiEchoFn)c->fn)(c->ctx, c->cb, c->ud, c->req); +} +""" +.} + +when defined(windows): + {. + emit: """/*INCLUDESECTION*/ +#include +""" + .} + {. + emit: """ +static DWORD WINAPI nimffi_foreign_thread_main(LPVOID arg) { + nimffi_foreign_body((NimFfiForeignCall*)arg); + return 0; +} + +int nimffi_call_on_foreign_thread( + void* fn, void* ctx, void* cb, void* ud, const void* req) { + NimFfiForeignCall c; + HANDLE t; + c.fn = fn; c.ctx = ctx; c.cb = cb; c.ud = ud; c.req = req; c.ret = -1; + t = CreateThread(NULL, 0, nimffi_foreign_thread_main, &c, 0, NULL); + if (t == NULL) return -2; + WaitForSingleObject(t, INFINITE); + CloseHandle(t); + return c.ret; +} +""" + .} +else: + {. + emit: """/*INCLUDESECTION*/ +#include +""" + .} + {. + emit: """ +static void* nimffi_foreign_thread_main(void* arg) { + nimffi_foreign_body((NimFfiForeignCall*)arg); + return (void*)0; +} + +int nimffi_call_on_foreign_thread( + void* fn, void* ctx, void* cb, void* ud, const void* req) { + NimFfiForeignCall c; + pthread_t t; + c.fn = fn; c.ctx = ctx; c.cb = cb; c.ud = ud; c.req = req; c.ret = -1; + if (pthread_create(&t, (void*)0, nimffi_foreign_thread_main, &c) != 0) return -2; + pthread_join(t, (void*)0); + return c.ret; +} +""" + .} + +proc nimffi_call_on_foreign_thread( + fn, ctx, cb, ud: pointer, req: pointer +): cint {.importc, nodecl.} + +proc callOnForeignThread( + ctx: ptr FFIContext[ThreadLib], req: ptr ThreadedcabiEchoReq_CWire, d: ptr ReplyData +): cint = + ## This proc passes the export to C as an opaque pointer. Only the typedef + ## above can differ from the generated signature. + nimffi_call_on_foreign_thread( + cast[pointer](ThreadedcabiEchoReqCAbiExport), + cast[pointer](ctx), + cast[pointer](onStringReply), + cast[pointer](d), + cast[pointer](req), + ) + +proc runScenario(tag: string, rounds: int): bool = + ## Creates one context, then makes `rounds` calls to it. Each call starts a + ## new platform thread, runs the entry point on it once, and joins it. + let ctx = makeCtx(tag) + for i in 0 ..< rounds: + var d: ReplyData + initReplyData(d) + var req = + packedWire(ThreadedcabiEchoReq_CWire, ThreadedcabiEchoReq(text: "call " & $i)) + + let rc = callOnForeignThread(ctx, addr req, addr d) + waitReply(d) + let good = rc == RET_OK and d.retCode == RET_OK and d.text == tag & ":call " & $i + + cwireFree(req) + deinitReplyData(d) + if not good: + echo tag, ": round ", i, " failed: rc=", rc, " ret=", d.retCode, " text=", d.text + return false + + if ThreadLibFFIPool.destroyFFIContext(ctx).isErr(): + echo tag, ": destroyFFIContext failed" + return false + return true + +proc main(): int = + # Multiple rounds make sure that the registration is per-thread, not one + # time for the process. + if not runScenario("single", 1): + return 1 + if not runScenario("multi", 8): + return 1 + return 0 + +quit(main()) diff --git a/tests/unit/test_foreign_thread_c_abi.nim b/tests/unit/test_foreign_thread_c_abi.nim new file mode 100644 index 0000000..3258fd0 --- /dev/null +++ b/tests/unit/test_foreign_thread_c_abi.nim @@ -0,0 +1,43 @@ +## This test runs the fixture in a child process. The fixture calls `abi = c` +## entry points from threads that the Nim runtime does not know. A regression +## crashes the child, not this suite. The child uses the same --mm switch as +## this run. The regression is fatal under refc and harmless under orc. + +import std/[os, osproc, compilesettings] +import unittest2 + +const + fixture = + currentSourcePath().parentDir() / "fixtures" / "foreign_thread_c_abi_fixture.nim" + nimExe = getCurrentCompilerExe() + ffiSearchPaths = querySettingSeq(searchPaths) + mmFlag = + when compileOption("mm", "refc"): + "--mm:refc" + elif compileOption("mm", "orc"): + "--mm:orc" + elif compileOption("mm", "arc"): + "--mm:arc" + else: + "" + +proc runFixture(): tuple[output: string, exitCode: int] = + let outDir = getTempDir() / "ffi_foreign_thread_out" + let cacheDir = getTempDir() / "ffi_foreign_thread_cache" + createDir(outDir) + var cmd = quoteShell(nimExe) & " c -r --hints:off --warnings:off" + if mmFlag.len > 0: + cmd.add(" " & mmFlag) + for p in ffiSearchPaths: + cmd.add(" --path:" & quoteShell(p)) + cmd.add(" --nimcache:" & quoteShell(cacheDir)) + # Write the binary to the temp directory. The fixture directory contains only source. + cmd.add(" --outdir:" & quoteShell(outDir)) + cmd.add(" " & quoteShell(fixture)) + execCmdEx(cmd) + +suite "abi = c entry points are callable from foreign host threads": + test "method calls from unregistered host threads succeed": + let (output, code) = runFixture() + checkpoint(output) + check code == 0