Always dump stack trace to binary term#47
Conversation
WalkthroughAdd propagation and formatting of native Zig stack traces into Elixir exceptions via a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant BEAM as BEAM / Elixir
participant NIF as Zig NIF
participant Formatter as Kinda.CallError.message/1
rect rgba(120,200,180,0.06)
Note left of NIF: native error occurs in NIF
NIF->>NIF: build optional StackTrace (debug)
NIF->>NIF: writeStackTraceToBuffer(env, stack_trace) -> binary | nil
NIF->>BEAM: return exception map {"message": msg, "error_return_trace": trace | nil}
end
BEAM->>Formatter: construct Kinda.CallError(map)
alt error_return_trace == nil
Formatter->>BEAM: message() -> plain message
else error_return_trace present
Formatter->>BEAM: message() -> message + "\n" + ANSI_RESET + error_return_trace
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/elixir.yml(1 hunks)kinda_example/test/kinda_example_test.exs(2 hunks)lib/kinda/error/nif_call.ex(1 hunks)src/beam.zig(1 hunks)src/kinda.zig(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
kinda_example/test/kinda_example_test.exs (1)
lib/kinda/error/nif_call.ex (1)
message(7-9)
🔇 Additional comments (6)
src/kinda.zig (1)
13-13: LGTM! Type simplification is appropriate.The change from
std.array_list.AlignedManaged(u8, null)tostd.array_list.Managed(u8)is a good simplification. Since the alignment parameter wasnull(default alignment), usingManageddirectly is cleaner and equivalent.Also applies to: 19-19
src/beam.zig (2)
1406-1415: Stack trace formatting implementation looks correct.The function properly:
- Creates a managed buffer with appropriate cleanup via
defer- Formats the stack trace using the adapted writer interface
- Converts to a BEAM slice term
The use of
adaptToNewApisuggests this is handling a Zig standard library API transition, which is a reasonable approach.
1417-1432: LGTM! Exception enhancement integrates cleanly.The changes properly:
- Initialize
stack_traceto nil as a sensible default- Conditionally format the trace with error handling that falls back to nil on failure
- Extend the exception map with the new
error_return_tracefieldThis provides graceful degradation if trace formatting fails while still propagating stack trace information when available.
kinda_example/test/kinda_example_test.exs (2)
11-11: LGTM! Test assertions properly updated.The pattern matches correctly account for the new
error_return_tracefield inKinda.CallError. Using_to match any trace value is appropriate since the exact trace content may vary.Also applies to: 30-30
22-22: LGTM! Message format and trace content assertions are thorough.The tests verify:
- ANSI reset sequence is properly included (matching the
message/1implementation)- Stack trace contains expected source file paths (
src/beam.zigandkinda_example/native/zig-src/main.zig)These assertions ensure the trace formatting works correctly end-to-end.
Also applies to: 28-29
lib/kinda/error/nif_call.ex (1)
2-2: LGTM! Exception enhancement is well-implemented.The changes cleanly:
- Add the
error_return_tracefield to the exception- Handle both nil and present trace cases with pattern matching
- Include ANSI reset sequence for proper terminal formatting when displaying traces
This implementation aligns with the Zig-side changes and test expectations.
Also applies to: 5-9
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/debug.zig (2)
6-6: Clarify the unusedself_debug_infovariable.The global variable
self_debug_infois declared but never used in this file. Consider:
- Removing it if it's dead code
- Adding a TODO comment if it's intended for future use
- Moving it to where it's actually used if it belongs elsewhere
-var self_debug_info: ?SelfInfo = null; -
19-19: Consider documenting the allocator choice.The code uses
std.heap.c_allocatorfor debug info operations instead ofbeam.allocator. While this might be intentional for debug information handling (which may need to persist beyond BEAM-managed memory), documenting this choice would help future maintainers understand the reasoning.Consider adding a comment:
// Use c_allocator for debug info as it may need to persist beyond BEAM memory lifecycle const symbol_info = module.getSymbolAtAddress(std.heap.c_allocator, address) catch {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/elixir.yml(2 hunks)src/beam.zig(1 hunks)src/debug.zig(1 hunks)
🔇 Additional comments (6)
.github/workflows/elixir.yml (1)
67-70: LGTM! Dedicated gdb installation step is correctly implemented.The extraction of gdb installation into a dedicated step improves workflow organization and readability. The implementation is standard and the placement before gdb-based tests is correct.
Note: There's a past review comment about PR title mismatch, but it references "Fix typo in GHA" while the current PR title is "Always dump stack trace to binary term". The current changes align with the PR's stack trace handling goals, so that past comment may no longer be relevant.
src/beam.zig (3)
1406-1412: Verify the writer type parameter.The function signature uses
*std.io.Writeras the parameter type, which is unusual in Zig code. Typically, writer parameters are either:
anytypefor generic writers- A specific concrete writer type (e.g.,
std.ArrayList(u8).Writer)The usage of
adaptToNewApiinwriteStackTraceToBuffersuggests you're managing a writer API transition in Zig 0.15.1. While this might be correct for your version, ensure this approach is stable and won't break with future Zig updates.Consider documenting why this specific writer type and API adaptation is needed, as it's non-standard and future maintainers might question it.
1413-1422: LGTM! Good error handling with fallback.The implementation correctly:
- Buffers the stack trace output using ArrayList
- Adapts the writer API to handle compatibility
- Catches errors and falls back to nil rather than propagating failures
This defensive approach ensures that stack trace formatting failures don't crash the entire error reporting mechanism.
1424-1439: LGTM! Clean integration of stack trace into exceptions.The modification correctly:
- Initializes
stack_traceas nil by default- Conditionally formats the trace when
error_traceis provided- Adds the new
error_return_tracefield to the exception map alongside existing fieldsThis approach maintains backward compatibility while adding the new trace functionality.
src/debug.zig (2)
12-36: Address the commented-out cleanup code.Line 17 contains a commented-out
defer module.deinit(debug_info_allocator.?)which suggests either:
- A potential memory leak if cleanup is needed but not performed
- The API changed and cleanup is no longer required
- The cleanup is handled elsewhere
Please clarify why this cleanup is commented out and whether memory management is correct here. If cleanup is not needed, remove the comment to avoid confusion.
38-64: LGTM! Stack trace formatting is well-implemented.The function correctly:
- Handles the case where debug info is stripped
- Uses a circular buffer approach to iterate through frames
- Formats each frame with proper indentation and numbering
- Provides clear fallbacks for missing information
The frame iteration logic properly handles the circular buffer by counting down from the most recent frame, which is the standard way to present stack traces.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/debug.zig(1 hunks)
🔇 Additional comments (1)
src/debug.zig (1)
48-56: Verify the frame numbering convention.The current implementation numbers frames in reverse order compared to typical stack trace conventions:
- Oldest (outermost) frame is printed as #N
- Newest (innermost) frame is printed as #1
Most languages (C, Python, Rust, etc.) use the opposite convention where #0 is the innermost/newest frame and higher numbers represent outer frames.
This might be intentional for Elixir's conventions, but please verify this is the desired behavior.
| const beam = @import("beam.zig"); | ||
| const SelfInfo = std.debug.SelfInfo; | ||
|
|
||
| var self_debug_info: ?SelfInfo = null; |
There was a problem hiding this comment.
Remove unused global variable.
The global variable self_debug_info is never initialized or referenced anywhere in this file. The formatStackTrace function receives debug_info as a parameter instead.
Apply this diff to remove the dead code:
-var self_debug_info: ?SelfInfo = null;
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var self_debug_info: ?SelfInfo = null; |
🤖 Prompt for AI Agents
In src/debug.zig around line 6, remove the unused global variable declaration
`var self_debug_info: ?SelfInfo = null;` because it is never initialized or
referenced; update the file by deleting that line so only the active
declarations and the parameter-passed `debug_info` remain.
1 participant
getenv() is not thread-safe in glibc—if another thread (e.g., Python’s setenv()) modifies the environment concurrently, it can corrupt memory or crash due to reallocation of environment variables.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores