Skip to content

chore(nim-bindings): replace dynlib dlopen with plain importc#61

Merged
osmaczko merged 4 commits intomainfrom
chore/remove-dynamic-loading
Feb 25, 2026
Merged

chore(nim-bindings): replace dynlib dlopen with plain importc#61
osmaczko merged 4 commits intomainfrom
chore/remove-dynamic-loading

Conversation

@osmaczko
Copy link
Contributor

The dynlib pragma hard-coded a library path and resolved it via dlopen() at runtime, preventing static linking and forcing a specific load-time path. Using bare {.importc.} lets consumers choose: link liblibchat dynamically at link time (--passL:-llibchat) or link it statically into their binary.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the Nim FFI bindings to stop using Nim’s dynlib runtime loading/path resolution and instead rely on plain {.importc.} so consumers can choose dynamic or static linking at link time.

Changes:

  • Removed compile-time/runtime library path resolution logic (previous CONVERSATIONS_LIB/relative target/{release,debug} discovery).
  • Updated all exported FFI procs to use {.importc.} without dynlib:.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -1,29 +1,5 @@
# Nim FFI bindings for libchat conversations library
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With dynlib: removed, these FFI procs now require link-time configuration (e.g., --passL:-L... --passL:-llibchat or static .a) to resolve symbols. Please add/update package-level documentation (and any repo examples/build tasks) to ensure consumers (and nimble pingpong) know how to link against the Rust-built target/release artifacts; otherwise builds will fail with unresolved symbols.

Suggested change
# Nim FFI bindings for libchat conversations library
# Nim FFI bindings for libchat conversations library
#
# Linking / build notes
# ----------------------
# These procs are declared with `{.importc.}` and *no* `dynlib` pragma, so the
# Nim compiler expects all symbols to be resolved by the linker at build time.
# You must build the Rust library and tell Nim how to link against it.
#
# 1. Build the Rust library
# From the Rust crate that defines the FFI (e.g. `libchat`), run:
#
# cargo build --release
#
# This will typically produce artifacts under:
# target/release/libchat.a (static library)
# target/release/libchat.so (Linux, shared library)
# target/release/libchat.dylib (macOS, shared library)
# target/release/chat.dll (Windows, shared library)
#
# 2. Tell Nim how to link
# Use `--passL` to add the Rust `target/release` directory to the linker
# search path, and link the `chat` library:
#
# nim c \
# --passL:-L/path/to/rust-crate/target/release \
# --passL:-lchat \
# src/your_app.nim
#
# Adjust the `-L` path to point at your Rust crate's `target/release`
# directory. The `-lchat` part must match the library name produced by
# Rust (`libchat.*` -> `-lchat`).
#
# 3. Using Nimble (including `nimble pingpong`)
# To ensure `nimble build`, `nimble test`, and `nimble pingpong` all link
# correctly without extra command-line flags, add the linker switches to
# your `.nimble` file, for example:
#
# # in yourpkg.nimble
# switch("passL", "-Lpath/to/rust-crate/target/release")
# switch("passL", "-lchat")
#
# If your layout is different (e.g. this Nim package lives inside the Rust
# repo), you can use a relative path instead:
#
# switch("passL", "-L./target/release")
# switch("passL", "-lchat")
#
# Without these link-time settings, builds that use the procs below will fail
# with unresolved symbol / linker errors, even though the Nim code compiles.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@jazzz jazzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks nimble build for nim-bindings/conversation_example.nimble

CC: std/objectdollar.nim
CC: system/dollars.nim
CC: system.nim
CC: options.nim
CC: results.nim
CC: bindings.nim
CC: libchat.nim
CC: pingpong.nim
Hint:  [Link]
Undefined symbols for architecture arm64:
  "_create_context", referenced from:
      _newConversationsContext__OOZsrcZlibchat_u11 in @plibchat.nim.c.o
  "_create_intro_bundle", referenced from:
      _createIntroductionBundle__OOZsrcZlibchat_u44 in @plibchat.nim.c.o
  "_create_new_private_convo", referenced from:
      _createNewPrivateConvo__OOZsrcZlibchat_u118 in @plibchat.nim.c.o
  "_destroy_convo_result", referenced from:
      _createNewPrivateConvo__OOZsrcZlibchat_u118 in @plibchat.nim.c.o
      _createNewPrivateConvo__OOZsrcZlibchat_u118 in @plibchat.nim.c.o
  "_destroy_intro_result", referenced from:
      _createIntroductionBundle__OOZsrcZlibchat_u44 in @plibchat.nim.c.o
  "_handle_payload", referenced from:
      _handlePayload__OOZsrcZlibchat_u882 in @plibchat.nim.c.o
ld: symbol(s) not found for architecture arm64

The dynlib pragma hard-coded a library path and resolved it via dlopen() at
runtime, preventing static linking and forcing a specific load-time path.
Using bare {.importc.} lets consumers choose: link liblibchat dynamically
at link time (--passL:-llibchat) or link it statically into their binary.
@osmaczko
Copy link
Contributor Author

This change breaks nimble build for nim-bindings/conversation_example.nimble

CC: std/objectdollar.nim
CC: system/dollars.nim
CC: system.nim
CC: options.nim
CC: results.nim
CC: bindings.nim
CC: libchat.nim
CC: pingpong.nim
Hint:  [Link]
Undefined symbols for architecture arm64:
  "_create_context", referenced from:
      _newConversationsContext__OOZsrcZlibchat_u11 in @plibchat.nim.c.o
  "_create_intro_bundle", referenced from:
      _createIntroductionBundle__OOZsrcZlibchat_u44 in @plibchat.nim.c.o
  "_create_new_private_convo", referenced from:
      _createNewPrivateConvo__OOZsrcZlibchat_u118 in @plibchat.nim.c.o
  "_destroy_convo_result", referenced from:
      _createNewPrivateConvo__OOZsrcZlibchat_u118 in @plibchat.nim.c.o
      _createNewPrivateConvo__OOZsrcZlibchat_u118 in @plibchat.nim.c.o
  "_destroy_intro_result", referenced from:
      _createIntroductionBundle__OOZsrcZlibchat_u44 in @plibchat.nim.c.o
  "_handle_payload", referenced from:
      _handlePayload__OOZsrcZlibchat_u882 in @plibchat.nim.c.o
ld: symbol(s) not found for architecture arm64

I believe I fixed, there is a segfault though.

@jazzz jazzz mentioned this pull request Feb 24, 2026
@jazzz
Copy link
Collaborator

jazzz commented Feb 24, 2026

After digging seems like there are larger issues: #62

Its presented as a PR to this PR

* Use correct build hook

* force sret like return from rust code for nim compatibility

* Fix target mismatch

* Update usages
@jazzz jazzz self-requested a review February 24, 2026 23:34
Copy link
Collaborator

@jazzz jazzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test Passing, Nim bindings are linking. Looks good now.

@osmaczko osmaczko force-pushed the chore/remove-dynamic-loading branch 2 times, most recently from f2b7450 to b3378ee Compare February 25, 2026 18:26
@osmaczko osmaczko requested a review from jazzz February 25, 2026 18:27
@osmaczko
Copy link
Contributor Author

@jazzz please take a look at the latest commit.

Copy link
Collaborator

@jazzz jazzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

…d defer-based cleanup

Nim's C backend silently transforms large struct parameters (>16 bytes) into
pointer parameters when calling importc functions. The destroy_* functions were
declared taking T by value in Rust, but Nim always passed &T — causing Rust to
read garbage from the stack on x86-64 (SIGILL on CI) while accidentally working
on ARM64 macOS due to that ABI coincidentally also using pointers for large structs.

Fix by changing all destroy_* functions to take &mut T and using drop_in_place,
which is the correct idiom for dropping a value through a pointer.

On the Nim side, replace scattered manual destroy calls with defer, which
guarantees cleanup on all exit paths and prevents use-after-destroy bugs.
@osmaczko osmaczko force-pushed the chore/remove-dynamic-loading branch from b3378ee to 1f8c38d Compare February 25, 2026 19:03
@osmaczko osmaczko merged commit fa79b1c into main Feb 25, 2026
5 checks passed
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.

3 participants