Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update zig @panic calls to roc_panic for numeric errors #6062

Merged
merged 17 commits into from
Dec 5, 2023
Merged

Update zig @panic calls to roc_panic for numeric errors #6062

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
737d766
add node to dev inputs
JRMurr Nov 21, 2023
23e2269
switch @panic to roc_panic to really throw panics for bad numeric ops
JRMurr Nov 21, 2023
191752e
add zls to flake
JRMurr Nov 21, 2023
5df2199
add test for dec division
JRMurr Nov 21, 2023
863ecd8
be excited to panic...
JRMurr Nov 21, 2023
abc92de
add test for divCeil by 0
JRMurr Nov 21, 2023
14478c8
remove bad copy paste
JRMurr Nov 21, 2023
c9e1456
fix formatting in flake
JRMurr Nov 22, 2023
925cba4
update docs for wasm repl dev
JRMurr Nov 22, 2023
38207cf
fix typo
JRMurr Nov 22, 2023
4fe4041
remove node from dev shell
JRMurr Nov 24, 2023
2a762f1
revert change of integer => Integer in panic messages
JRMurr Nov 28, 2023
a5180be
make zig `roc_panic` return type be `noreturn` and remove some `unrea…
JRMurr Nov 28, 2023
d6f0709
feedback: update some panic messages
JRMurr Nov 29, 2023
d37dd44
undo change in debug server
JRMurr Nov 29, 2023
298f93d
update a bunch of panic message to be capitalized
JRMurr Nov 29, 2023
20bcd70
Merge branch 'main' into div-0-should-crash
ayazhafiz Dec 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions crates/compiler/builtins/bitcode/src/dec.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,6 @@ pub const RocDec = extern struct {

if (answer.has_overflowed) {
roc_panic("Decimal addition overflowed!", 0);
unreachable;
} else {
return answer.value;
}
Expand Down Expand Up @@ -283,7 +282,6 @@ pub const RocDec = extern struct {

if (answer.has_overflowed) {
roc_panic("Decimal subtraction overflowed!", 0);
unreachable;
} else {
return answer.value;
}
Expand Down Expand Up @@ -347,7 +345,6 @@ pub const RocDec = extern struct {

if (answer.has_overflowed) {
roc_panic("Decimal multiplication overflowed!", 0);
unreachable;
} else {
return answer.value;
}
Expand Down Expand Up @@ -398,7 +395,6 @@ pub const RocDec = extern struct {
return self;
} else {
roc_panic("Decimal division overflow in numerator!", 0);
unreachable;
}
};
const numerator_u128 = @as(u128, @intCast(numerator_abs_i128));
Expand All @@ -412,7 +408,6 @@ pub const RocDec = extern struct {
return other;
} else {
roc_panic("Decimal division overflow in denominator!", 0);
unreachable;
}
};
const denominator_u128 = @as(u128, @intCast(denominator_abs_i128));
Expand Down Expand Up @@ -634,7 +629,7 @@ fn mul_and_decimalize(a: u128, b: u128) i128 {
const d = answer[0];

if (overflowed == 1) {
roc_panic("Decimal multiplication overflow!", 0);
roc_panic("Decimal multiplication overflow22!", 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
roc_panic("Decimal multiplication overflow22!", 0);
roc_panic("Decimal multiplication overflow!", 0);

}

// Final 512bit value is d, c, b, a
Expand Down Expand Up @@ -1211,7 +1206,6 @@ pub fn fromF64C(arg: f64) callconv(.C) i128 {
return dec.num;
} else {
roc_panic("Decimal conversion from f64!", 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
roc_panic("Decimal conversion from f64!", 0);
roc_panic("Decimal conversion from f64 failed!", 0);

unreachable;
}
}

Expand All @@ -1221,7 +1215,6 @@ pub fn fromF32C(arg_f32: f32) callconv(.C) i128 {
return dec.num;
} else {
roc_panic("Decimal conversion from f32!", 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
roc_panic("Decimal conversion from f32!", 0);
roc_panic("Decimal conversion from f32 failed!", 0);

unreachable;
}
}

Expand All @@ -1237,7 +1230,6 @@ pub fn exportFromInt(comptime T: type, comptime name: []const u8) void {
const answer = @mulWithOverflow(this, RocDec.one_point_zero_i128);
if (answer[1] == 1) {
roc_panic("Decimal conversion from integer!", 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
roc_panic("Decimal conversion from integer!", 0);
roc_panic("Decimal conversion from integer failed!", 0);

unreachable;
} else {
return answer[0];
}
Expand Down Expand Up @@ -1265,14 +1257,12 @@ pub fn neqC(arg1: RocDec, arg2: RocDec) callconv(.C) bool {
pub fn negateC(arg: RocDec) callconv(.C) i128 {
return if (@call(.always_inline, RocDec.negate, .{arg})) |dec| dec.num else {
roc_panic("Decimal negation overflow!", 0);
unreachable;
};
}

pub fn absC(arg: RocDec) callconv(.C) i128 {
const result = @call(.always_inline, RocDec.abs, .{arg}) catch {
roc_panic("Decimal absolute value overflow!", 0);
unreachable;
};
return result.num;
}
Expand Down
4 changes: 0 additions & 4 deletions crates/compiler/builtins/bitcode/src/num.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,6 @@ pub fn exportDivCeil(comptime T: type, comptime name: []const u8) void {
fn func(a: T, b: T) callconv(.C) T {
return math.divCeil(T, a, b) catch {
roc_panic("integer division by 0!", 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be worth trying to convert these all back to Integer. Probably will require update some tests. I assume most in cargo test-gen-llvm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ill give that a shot

Copy link
Contributor Author

@JRMurr JRMurr Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Did a bunch of find replace and made all the panics i found capitalize Integer

unreachable;
};
}
}.func;
Expand Down Expand Up @@ -383,7 +382,6 @@ pub fn exportAddOrPanic(comptime T: type, comptime name: []const u8) void {
const result = addWithOverflow(T, self, other);
if (result.has_overflowed) {
roc_panic("integer addition overflowed!", 0);
unreachable;
} else {
return result.value;
}
Expand Down Expand Up @@ -441,7 +439,6 @@ pub fn exportSubOrPanic(comptime T: type, comptime name: []const u8) void {
const result = subWithOverflow(T, self, other);
if (result.has_overflowed) {
roc_panic("integer subtraction overflowed!", 0);
unreachable;
} else {
return result.value;
}
Expand Down Expand Up @@ -626,7 +623,6 @@ pub fn exportMulOrPanic(comptime T: type, comptime W: type, comptime name: []con
const result = @call(.always_inline, mulWithOverflow, .{ T, W, self, other });
if (result.has_overflowed) {
roc_panic("integer multiplication overflowed!", 0);
unreachable;
} else {
return result.value;
}
Expand Down
6 changes: 3 additions & 3 deletions crates/compiler/builtins/bitcode/src/panic.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ const std = @import("std");
const RocStr = @import("str.zig").RocStr;

// Signals to the host that the program has panicked
extern fn roc_panic(msg: *const RocStr, tag_id: u32) callconv(.C) void;
extern fn roc_panic(msg: *const RocStr, tag_id: u32) callconv(.C) noreturn;

pub fn panic_help(msg: []const u8, tag_id: u32) void {
pub fn panic_help(msg: []const u8, tag_id: u32) noreturn {
var str = RocStr.init(msg.ptr, msg.len);
roc_panic(&str, tag_id);
}

// must export this explicitly because right now it is not used from zig code
pub fn panic(msg: *const RocStr, alignment: u32) callconv(.C) void {
pub fn panic(msg: *const RocStr, alignment: u32) callconv(.C) noreturn {
return roc_panic(msg, alignment);
}
Comment on lines +5 to 15
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was having a weird error in CI were valgrind was crashing but was not happening locally

My gut feeling is that us adding unreachable after a roc_panic call is whats causing the issue.

Looks like unreachable is undefined behavior when compiled with RelaseFast https://ziglang.org/documentation/master/#try which i think we do

This return type lets you do

if (some_cond) {
    roc_panic(...)
} else {
    return foo
}

without zig type errors

Copy link
Contributor

Choose a reason for hiding this comment

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

So glad this worked. Thanks for the find