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

JRMurr · 2023-11-22T04:08:06Z

fyi I dont really know zig so please let me know if i did something wrong

The issue

calls to @panic in the zig bytecode generator would not bubble up the rest of the compiler so we would get nonsene numbers back instead of raise the error to the roc code that should have errored. For example 1dec / 0 would give a giant number in wasm and 0 in llvm

What I did

updated all the numeric related calls to @panic to roc_panic. TBH not sure on the difference between the two but added some tests and they only fail when expected when using roc_panic

Some errors I'm seeing

I posted some info in zulip

TLDR we still show "normal" output when a panic or call to crash happens.

I could see the argument that its nice to display the type signature still but maybe we hid the value for crashes?

Misc changes

added node and zls to the dev shell since npx is needed for the site dev and zls is nice when editing zig.

flake.nix

JRMurr · 2023-11-23T00:27:05Z

looking into failures. The apple-silicon failure looks like #5932 so possibly a flake?

The valgrind error i'm having trouble reproducing locally. will investigate

…chable` calls

JRMurr · 2023-11-28T05:07:16Z

crates/compiler/builtins/bitcode/src/panic.zig

+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);
 }


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

So glad this worked. Thanks for the find

bhansconnect · 2023-11-28T05:27:05Z

crates/compiler/builtins/bitcode/src/dec.zig

@@ -632,7 +629,7 @@ fn mul_and_decimalize(a: u128, b: u128) i128 {
    const d = answer[0];

    if (overflowed == 1) {
-        @panic("TODO runtime exception for overflow!");
+        roc_panic("Decimal multiplication overflow22!", 0);


Suggested change

roc_panic("Decimal multiplication overflow22!", 0);

roc_panic("Decimal multiplication overflow!", 0);

bhansconnect · 2023-11-28T05:27:22Z

crates/compiler/builtins/bitcode/src/dec.zig

@@ -1208,7 +1205,7 @@ pub fn fromF64C(arg: f64) callconv(.C) i128 {
    if (@call(.always_inline, RocDec.fromF64, .{arg})) |dec| {
        return dec.num;
    } else {
-        @panic("TODO runtime exception failing convert f64 to RocDec");
+        roc_panic("Decimal conversion from f64!", 0);


Suggested change

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

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

bhansconnect · 2023-11-28T05:27:32Z

crates/compiler/builtins/bitcode/src/dec.zig

@@ -1217,7 +1214,7 @@ pub fn fromF32C(arg_f32: f32) callconv(.C) i128 {
    if (@call(.always_inline, RocDec.fromF64, .{arg_f64})) |dec| {
        return dec.num;
    } else {
-        @panic("TODO runtime exception failing convert f64 to RocDec");
+        roc_panic("Decimal conversion from f32!", 0);


Suggested change

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

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

bhansconnect · 2023-11-28T05:27:47Z

crates/compiler/builtins/bitcode/src/dec.zig

@@ -1232,7 +1229,7 @@ pub fn exportFromInt(comptime T: type, comptime name: []const u8) void {

            const answer = @mulWithOverflow(this, RocDec.one_point_zero_i128);
            if (answer[1] == 1) {
-                @panic("TODO runtime exception failing convert integer to RocDec");
+                roc_panic("Decimal conversion from integer!", 0);


Suggested change

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

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

bhansconnect · 2023-11-28T05:28:55Z

crates/compiler/builtins/bitcode/src/num.zig

@@ -233,7 +233,9 @@ pub fn exportCeiling(comptime F: type, comptime T: type, comptime name: []const
 pub fn exportDivCeil(comptime T: type, comptime name: []const u8) void {
    comptime var f = struct {
        fn func(a: T, b: T) callconv(.C) T {
-            return math.divCeil(T, a, b) catch @panic("TODO runtime exception for dividing by 0!");
+            return math.divCeil(T, a, b) catch {
+                roc_panic("integer division by 0!", 0);


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

Ill give that a shot

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

bhansconnect · 2023-11-28T05:29:07Z

crates/compiler/builtins/bitcode/src/panic.zig

+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);
 }


So glad this worked. Thanks for the find

bhansconnect · 2023-11-28T05:30:04Z

crates/lang_srv/debug_server.sh

@@ -1,4 +1,4 @@
-#!/bin/bash


It looks like you have a number of unrelated changes in this file and below. Probably don't want those here?

ahh yea this file is not related to this branch. I can remove

the readme/gitignore updates were useful when i was testing the wasm repl so i figured those were fine to leave in but can move out

bhansconnect · 2023-11-30T20:35:43Z

crates/repl_wasm/README.md

Still a bit confused about the random repl and flake changes, but otherwise, this pr looks great.

The docs here were out of date, linking to the public dir did not seem to hot reload for me

bhansconnect · 2023-12-02T20:35:08Z

Do to some ci testing changes, I think this needs rebased or have main merged then tests should run fine and it should be good to merge.

JRMurr · 2023-12-04T03:28:01Z

@bhansconnect you ok with merging this with the doc changes or do you want me to pull those out?

folkertdev reviewed Nov 22, 2023

View reviewed changes

flake.nix Outdated Show resolved Hide resolved

JRMurr mentioned this pull request Nov 22, 2023

wasm repl: panics/crashing still displays "normal" output #6066

Open

JRMurr force-pushed the div-0-should-crash branch from 54ec0c8 to 9f78ab9 Compare November 23, 2023 01:08

JRMurr added 11 commits November 24, 2023 10:07

add node to dev inputs

737d766

switch @Panic to roc_panic to really throw panics for bad numeric ops

23e2269

add zls to flake

191752e

add test for dec division

5df2199

be excited to panic...

863ecd8

add test for divCeil by 0

abc92de

remove bad copy paste

14478c8

fix formatting in flake

c9e1456

update docs for wasm repl dev

925cba4

fix typo

38207cf

remove node from dev shell

4fe4041

JRMurr force-pushed the div-0-should-crash branch from 9f78ab9 to 4fe4041 Compare November 24, 2023 15:07

JRMurr added 2 commits November 27, 2023 19:59

revert change of integer => Integer in panic messages

2a762f1

make zig roc_panic return type be noreturn and remove some `unrea…

a5180be

…chable` calls

JRMurr commented Nov 28, 2023

View reviewed changes

bhansconnect reviewed Nov 28, 2023

View reviewed changes

JRMurr added 3 commits November 28, 2023 20:21

feedback: update some panic messages

d6f0709

undo change in debug server

d37dd44

update a bunch of panic message to be capitalized

298f93d

JRMurr requested a review from bhansconnect November 30, 2023 01:12

bhansconnect reviewed Nov 30, 2023

View reviewed changes

Merge branch 'main' into div-0-should-crash

20bcd70

bhansconnect approved these changes Dec 5, 2023

View reviewed changes

bhansconnect merged commit e336aa5 into roc-lang:main Dec 5, 2023
17 checks passed

bhansconnect mentioned this pull request Dec 5, 2023

Improve perf of Dict and Set #6176

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JRMurr commented Nov 22, 2023

JRMurr commented Nov 23, 2023

JRMurr Nov 28, 2023

bhansconnect Nov 28, 2023

bhansconnect Nov 28, 2023

bhansconnect Nov 28, 2023

bhansconnect Nov 28, 2023

bhansconnect Nov 28, 2023

bhansconnect Nov 28, 2023

JRMurr Nov 29, 2023

JRMurr Nov 29, 2023 •

edited

Loading

bhansconnect Nov 28, 2023

bhansconnect Nov 28, 2023

JRMurr Nov 29, 2023

bhansconnect Nov 30, 2023

JRMurr Dec 1, 2023

bhansconnect commented Dec 2, 2023

JRMurr commented Dec 4, 2023

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

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

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

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

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

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

Conversation

JRMurr commented Nov 22, 2023

The issue

What I did

Some errors I'm seeing

Misc changes

JRMurr commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JRMurr Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhansconnect commented Dec 2, 2023

JRMurr commented Dec 4, 2023

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

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

JRMurr Nov 29, 2023 •

edited

Loading