Debugging load and upgrade #398
Conversation
WalkthroughThe changes modify the Native Implemented Function (NIF) implementation in the Changes
Sequence DiagramsequenceDiagram
participant Beam VM
participant NIF
Beam VM->>NIF: nif_load(env, priv_data, term)
NIF-->>Beam VM: Initialize resources
Beam VM->>NIF: nif_upgrade(env, old_priv_data, new_priv_data, term)
NIF-->>Beam VM: Upgrade resources
Beam VM->>NIF: nif_unload(env, priv_data)
NIF-->>Beam VM: Cleanup resources
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
native/src/main.zig (1)
64-65: Remove or update the outdated comment.The comment "currently unsupported" for the upgrade field is no longer accurate since
nif_upgradeis now implemented.- .upgrade = nif_upgrade, - .unload = nif_unload, // currently unsupported + .upgrade = nif_upgrade, // handles code upgrades during hot code swapping + .unload = nif_unload, // handles cleanup during module unload🧰 Tools
🪛 GitHub Actions: Elixir CI
[error] File failed Zig format check. Run 'zig fmt' to format the file according to standard Zig formatting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
native/src/main.zig(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Elixir CI
native/src/main.zig
[error] File failed Zig format check. Run 'zig fmt' to format the file according to standard Zig formatting.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: arm_docker (linux/amd64, amd64, https://ziglang.org/download/0.13.0/zig-linux-x86_64-0.13.0.tar.xz)
- GitHub Check: NIF 26 (1.15)
- GitHub Check: arm_docker (linux/arm64/v8, arm64, https://ziglang.org/download/0.13.0/zig-linux-aarch64-0.13.0.t...
- GitHub Check: NIF 24.2 (1.14)
| fn nif_unload(_: beam.env, _: ?*anyopaque) callconv(.C) void { | ||
| std.debug.print("nif_unload\n", .{}); | ||
| // Perform any necessary cleanup steps here | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement resource cleanup in nif_unload.
The function currently only logs its invocation but doesn't perform any actual cleanup. Consider implementing proper resource cleanup for all opened resources.
Would you like me to help implement the cleanup logic for the resources opened in init_resources?
🧰 Tools
🪛 GitHub Actions: Elixir CI
[error] File failed Zig format check. Run 'zig fmt' to format the file according to standard Zig formatting.
| fn nif_upgrade(env: beam.env, _: [*c]?*anyopaque, _: [*c]?*anyopaque, _: beam.term) callconv(.C) c_int { | ||
| std.debug.print("nif_upgrade\n", .{}); | ||
| kinda.open_internal_resource_types(env); | ||
| kinda.Internal.OpaqueStruct.open_all(env); | ||
| mlir_capi.open_all(env); | ||
| memref.open_all(env); | ||
| logical_mutex.open_all(env); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common initialization code to reduce duplication.
The resource opening calls are duplicated between nif_load and nif_upgrade. Consider extracting them into a shared function.
+fn init_resources(env: beam.env) void {
+ kinda.open_internal_resource_types(env);
+ kinda.Internal.OpaqueStruct.open_all(env);
+ mlir_capi.open_all(env);
+ memref.open_all(env);
+ logical_mutex.open_all(env);
+}
+
fn nif_upgrade(env: beam.env, _: [*c]?*anyopaque, _: [*c]?*anyopaque, _: beam.term) callconv(.C) c_int {
std.debug.print("nif_upgrade\n", .{});
- kinda.open_internal_resource_types(env);
- kinda.Internal.OpaqueStruct.open_all(env);
- mlir_capi.open_all(env);
- memref.open_all(env);
- logical_mutex.open_all(env);
+ init_resources(env);
return 0;
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Elixir CI
[error] File failed Zig format check. Run 'zig fmt' to format the file according to standard Zig formatting.
| fn nif_load(env: beam.env, priv_data: [*c]?*anyopaque, _: beam.term) callconv(.C) c_int { | ||
| std.debug.print("nif_load {?}\n", .{env}); | ||
| std.debug.print("priv_data {*}\n", .{priv_data.*}); | ||
| if (priv_data.*) |data| { | ||
| std.debug.print("priv_data {*}\n", .{data}); | ||
| } else { | ||
| std.debug.print("priv_data is null\n", .{}); | ||
| priv_data.* = env; | ||
| std.debug.print("enif_priv_data: {*}\n", .{e.enif_priv_data(env)}); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Storing env in priv_data is unsafe and should be avoided
The NIF environment (env) is only valid during NIF function calls and should not be stored for later use. Storing it in priv_data could lead to undefined behavior, especially during module upgrades when the old environment becomes invalid.
- Consider using
priv_datafor storing module-specific state instead - If you need to maintain state between calls, create a proper state structure
🔗 Analysis chain
Format the code according to Zig standards.
The pipeline indicates formatting issues. Run zig fmt to fix the formatting.
-fn nif_load(env: beam.env, priv_data: [*c]?*anyopaque, _: beam.term) callconv(.C) c_int {
+fn nif_load(env: beam.env, priv_data: [*c]?*anyopaque, _: beam.term) callconv(.C) c_int {Consider using conditional compilation for debug prints.
The debug prints might impact production performance. Consider wrapping them in debug build conditions.
+const is_debug = @import("builtin").mode == .Debug;
fn nif_load(env: beam.env, priv_data: [*c]?*anyopaque, _: beam.term) callconv(.C) c_int {
- std.debug.print("nif_load {?}\n", .{env});
- std.debug.print("priv_data {*}\n", .{priv_data.*});
+ if (is_debug) {
+ std.debug.print("nif_load {?}\n", .{env});
+ std.debug.print("priv_data {*}\n", .{priv_data.*});
+ }Verify priv_data assignment safety.
The assignment of env to priv_data needs verification to ensure it's the intended behavior and doesn't cause memory issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns in other Erlang NIFs
rg -A 5 "enif_priv_data"
rg -A 5 "priv_data.*=.*env"
Length of output: 1068
Script:
#!/bin/bash
# Check for NIF upgrade handling
ast-grep --pattern 'fn nif_upgrade($$$) callconv(.C) $$ {
$$$
}'
# Look for NIF environment usage patterns
rg -A 5 "beam.env"
Length of output: 27238
🧰 Tools
🪛 GitHub Actions: Elixir CI
[error] File failed Zig format check. Run 'zig fmt' to format the file according to standard Zig formatting.
Summary by CodeRabbit
New Features
Improvements