Deprecated allocator initialization + tests fix LLVM crash by AI #10
Deprecated allocator initialization + tests fix LLVM crash by AI #10koko1123 merged 4 commits intoStrobeLabs:mainfrom
Conversation
|
@Mario-SO is attempting to deploy a commit to the koko-strobeorg's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAllocator initialization syntax updated in docs/examples; src/utils/units.zig gains f64↔u256 conversion helpers, refactors parse/format functions to use them, and adds tests. Public APIs unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🧹 Nitpick comments (1)
src/utils/units.zig (1)
51-92: Consider adding edge-case tests for the new conversion helpers.The existing tests cover typical cases well. Consider adding tests for:
- Very large u256 values that exercise the hi/lo split path (values > 2^128)
- Boundary precision cases
🧪 Example additional test cases
test "u256ToF64 large value with hi bits" { // Value with both hi and lo components: 2^128 + 1 const large: u256 = (`@as`(u256, 1) << 128) | 1; const result = u256ToF64(large); // Should be approximately 2^128 (lo component may be lost to f64 precision) try std.testing.expect(result >= TWO_POW_128_F64); } test "parseEther very small fractional" { // 1 wei = 10^-18 ether const one_wei = parseEther(1e-18); // Due to f64 precision limits, this may not be exactly 1 try std.testing.expect(one_wei <= 2); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/units.zig` around lines 51 - 92, Add edge-case tests to exercise hi/lo u256 conversion and boundary precision: create a test that constructs a u256 with high bits set (e.g., (`@as`(u256,1) << 128) | 1) and assert u256ToF64(large) behaves (e.g., result >= TWO_POW_128_F64) to verify the hi-path; add tests for tiny fractional values via parseEther(1e-18) and parseGwei with values near precision limits to assert reasonable behaviour/bounds (not exact equality), and add a large-ether parse test (value > 2^128) to ensure parseEther/parseGwei and roundtrip with formatEther/formatGwei handle hi/lo splits; reference tests by name (e.g., add "u256ToF64 large value with hi bits", "parseEther very small fractional") and reuse existing helpers u256ToF64, parseEther, parseGwei, formatEther, formatGwei.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/units.zig`:
- Around line 22-28: Validate the f64 before converting in f64ToU256Trunc: at
the top of f64ToU256Trunc check that value >= 0.0 and that it's finite (use
std.math.isFinite or equivalent and std.math.isnan checks) and if the check
fails, abort with a clear `@panic` or return an error (choose panic if you want no
API change), so `@intFromFloat` is never called on negative/NaN/inf; alternatively
move the same validation into the public parseEther/parseGwei functions if you
prefer to return a caller-facing error union instead of panicking.
---
Nitpick comments:
In `@src/utils/units.zig`:
- Around line 51-92: Add edge-case tests to exercise hi/lo u256 conversion and
boundary precision: create a test that constructs a u256 with high bits set
(e.g., (`@as`(u256,1) << 128) | 1) and assert u256ToF64(large) behaves (e.g.,
result >= TWO_POW_128_F64) to verify the hi-path; add tests for tiny fractional
values via parseEther(1e-18) and parseGwei with values near precision limits to
assert reasonable behaviour/bounds (not exact equality), and add a large-ether
parse test (value > 2^128) to ensure parseEther/parseGwei and roundtrip with
formatEther/formatGwei handle hi/lo splits; reference tests by name (e.g., add
"u256ToF64 large value with hi bits", "parseEther very small fractional") and
reuse existing helpers u256ToF64, parseEther, parseGwei, formatEther,
formatGwei.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/content/docs/introduction.mdxexamples/02_check_balance.zigexamples/04_send_transaction.zigexamples/05_read_erc20.zigsrc/utils/units.zig
|
Thanks so much for the PR! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Investigation: LLVM crash reproduction and alternative fixHey @Mario-SO, thanks for the PR! I investigated this in depth and wanted to share findings. Root causeThis is a known upstream LLVM bug: ziglang/zig#18820. LLVM cannot lower Key finding: The crash happens on Zig 0.15.2 when running ReproductionMinimal reproducer on aarch64: const std = @import("std");
const ETHER: u256 = 1_000_000_000_000_000_000;
fn parseEther(ether: f64) u256 {
const wei_f = ether * @as(f64, @floatFromInt(ETHER));
return @intFromFloat(wei_f);
}
test "parse ether" {
const x = parseEther(1.0);
try std.testing.expect(x > 0);
}# Crashes on aarch64-macos with Zig 0.15.2 and 0.16.0-dev
zig test /tmp/repro.zig
# LLVM ERROR: Unsupported library call operation!Proposed alternative fixThe current hi/lo u128 decomposition in this PR adds intermediate arithmetic that isn't necessary. A simpler approach: route conversions through const std = @import("std");
pub const ETHER: u256 = 1_000_000_000_000_000_000;
pub const GWEI: u256 = 1_000_000_000;
pub const WEI: u256 = 1;
// Comptime-evaluated f64 constants. Avoids runtime @floatFromInt on u256
// which crashes LLVM on aarch64 (https://github.com/ziglang/zig/issues/18820).
const ETHER_F64: f64 = @as(f64, @floatFromInt(ETHER));
const GWEI_F64: f64 = @as(f64, @floatFromInt(GWEI));
/// f64 -> u256 via u128 intermediate. LLVM supports f64<->u128 on all
/// backends; direct f64<->u256 crashes on aarch64 with LLVM 21+.
inline fn f64ToU256(val: f64) u256 {
return @as(u256, @as(u128, @intFromFloat(val)));
}
/// u256 -> f64 via u128 intermediate. Values <= u128 max use a single
/// @floatFromInt; larger values decompose into hi/lo halves.
inline fn u256ToF64(val: u256) f64 {
const hi: u128 = @truncate(val >> 128);
if (hi == 0) {
return @as(f64, @floatFromInt(@as(u128, @truncate(val))));
}
const lo: u128 = @truncate(val);
return @as(f64, @floatFromInt(hi)) * (2.0 * @as(f64, @floatFromInt(@as(u128, 1) << 127))) + @as(f64, @floatFromInt(lo));
}
pub fn parseEther(ether: f64) u256 {
return f64ToU256(ether * ETHER_F64);
}
pub fn parseGwei(gwei: f64) u256 {
return f64ToU256(gwei * GWEI_F64);
}
pub fn formatEther(wei: u256) f64 {
return u256ToF64(wei) / ETHER_F64;
}
pub fn formatGwei(wei: u256) f64 {
return u256ToF64(wei) / GWEI_F64;
}Why this is better:
Test fix: The test "parseEther large value" {
// 9007.0 is exact in f64, but 9007.0 * 1e18 exceeds f64 mantissa precision.
// Allow up to 1 ULP of error at this magnitude (~2^20 = 1048576).
const result = parseEther(9007.0);
const expected: u256 = 9007_000_000_000_000_000_000;
const diff = if (result > expected) result - expected else expected - result;
try std.testing.expect(diff < 1_048_576);
}Verified locally (aarch64-macos, Zig 0.15.2):
GPA init changesThe Next stepsWe're filing this as an LLVM regression on the Zig issue tracker with the minimal reproducer. Would you be open to updating this PR with the simpler |
|
wow ok, nice job going deeper here, ok will do your suggestions, it'd be nice if you linked me or this PR in the filing of the zig issue, so there is full history there. 🫡 |
will do! you are already linked in the LLVM issue :) |
|
koko1123
left a comment
koko1123
left a comment
There was a problem hiding this comment.
LGTM!
tagged you in the LLVM issue: llvm/llvm-project#183747
2 participants
What
This contribution ended up as two commits:
Why
LLVM ERROR: Unsupported library call operation!. The crash was triggered by direct @intFromFloat/@floatFromInt with u256 in units.zig, so tests could not run reliably on Zig 0.15.2.How
Checklist
EDIT: Recommendation from Kristoff is to wait to 0.16 because it may have being solved already, anyway, i think that tests on current codebase should pass at least.
Summary by CodeRabbit
Documentation
Refactor
Tests