Skip to content

catch C++ exceptions at FFI boundary, rename utility modules#8

Merged
mcharytoniuk merged 6 commits intomainfrom
ffi-error-handling-and-sample-result
Apr 2, 2026
Merged

catch C++ exceptions at FFI boundary, rename utility modules#8
mcharytoniuk merged 6 commits intomainfrom
ffi-error-handling-and-sample-result

Conversation

@mcharytoniuk
Copy link
Copy Markdown

@mcharytoniuk mcharytoniuk commented Apr 2, 2026

All C++ wrapper functions now catch exceptions and forward error messages to Rust via out_error parameter with LLAMA_LOG_ERROR at each catch point. sample() returns Result<LlamaToken, SampleError> with the original C++ error message preserved. Renamed llama_utility_* modules to descriptive names.

@mcharytoniuk mcharytoniuk requested a review from Copilot April 2, 2026 19:08
@mcharytoniuk mcharytoniuk force-pushed the ffi-error-handling-and-sample-result branch from e16fcdf to a42bb45 Compare April 2, 2026 19:11
Copy link
Copy Markdown

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 hardens the Rust ↔ C++ FFI boundary by converting C++ exceptions into Rust errors (via out-error pointers) and updates token sampling to return a richer SampleResult so grammar completion can be signaled without crashing/throwing. It also renames several “utility” modules to clearer names and updates tests accordingly.

Changes:

  • Add exception-catching + error-message forwarding in C++ wrappers and corresponding Rust error-reading helpers.
  • Change LlamaSampler::sample() to return Result<SampleResult, SampleError> and propagate grammar-completion as a first-class outcome.
  • Rename / reorganize utility modules (e.g. *_utility_* → focused modules like ggml_time_us, mmap_supported, etc.).

Reviewed changes

Copilot reviewed 16 out of 23 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
llama-cpp-bindings/src/sampling.rs sample() now returns Result<SampleResult, SampleError> and reads C++ error strings from FFI.
llama-cpp-bindings/src/error.rs Adds SampleResult / SampleError, improves FFI-related error variants/messages.
llama-cpp-bindings-sys/wrapper_common.{h,cpp} Updates FFI signatures to include out_error, adds sampler_sample, thinking_end_tag, and catches C++ exceptions.
llama-cpp-bindings/src/model.rs Adds thinking_end_tag() and new tests around sampling + grammar behavior.
llama-cpp-bindings/tests/*.rs Updates integration tests to handle SampleResult and stop on GrammarCompleted.
llama-cpp-bindings/src/lib.rs Renames/exports modules and introduces ffi_error_reader + ffi_status_*.
llama-cpp-bindings/src/json_schema_to_grammar.rs Uses new out-error plumbing for better error messages.
llama-cpp-bindings/src/{ggml_time_us,llama_time_us,max_devices,mlock_supported,mmap_supported}.rs New focused utility modules + basic tests/docs.
llama-cpp-bindings/src/test_model.rs Adds download_file_from() helper used by new model tests.

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

@mcharytoniuk mcharytoniuk force-pushed the ffi-error-handling-and-sample-result branch 2 times, most recently from d306ec3 to 0239a90 Compare April 2, 2026 19:34
@mcharytoniuk mcharytoniuk changed the title catch C++ exceptions at FFI boundary, add SampleResult, rename utility modules catch C++ exceptions at FFI boundary, rename utility modules Apr 2, 2026
@mcharytoniuk mcharytoniuk force-pushed the ffi-error-handling-and-sample-result branch from 0239a90 to 0b9aa87 Compare April 2, 2026 19:42
@mcharytoniuk mcharytoniuk requested a review from Copilot April 2, 2026 20:11
Copy link
Copy Markdown

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

Copilot reviewed 18 out of 25 changed files in this pull request and generated 6 comments.


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

…or test models, remove internal header dependency
Copy link
Copy Markdown

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

Copilot reviewed 21 out of 28 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

llama-cpp-bindings/src/json_schema_to_grammar.rs:38

  • When the FFI call fails and error_ptr is null (e.g., allocation failure), this returns a generic "unknown error" and drops the actual status code. Consider including the status (and/or mapping known non-OK statuses like LLAMA_RS_STATUS_ALLOCATION_FAILED) in the error message so callers can distinguish failure modes even without an exception string.

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

@mcharytoniuk mcharytoniuk merged commit 49d153a into main Apr 2, 2026
2 checks passed
@mcharytoniuk mcharytoniuk deleted the ffi-error-handling-and-sample-result branch April 2, 2026 21:48
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.

2 participants