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.

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

dev backend: roc panic #5699

Merged
merged 25 commits into from
Aug 3, 2023
Merged

dev backend: roc panic #5699

merged 25 commits into from
Aug 3, 2023

Conversation

folkertdev
Copy link
Contributor

proof of concept of roc_panic. I think a crash is the only thing that panics at the moment though, so still some work left to do after this to really get parity.

@folkertdev folkertdev changed the base branch from main to alloca-expr July 30, 2023 18:50
@lukewilliamboswell
Copy link
Collaborator

Here are the results of that test running on my Windows machine.

PS C:\Users\bosyl\Documents\GitHub\roc> cargo nextest-gen-dev a_crash
    Finished test [unoptimized + debuginfo] target(s) in 0.83s
    Starting 1 test across 2 binaries (851 skipped)
        PASS [   1.235s] test_gen::test_gen gen_primitives::a_crash
------------
     Summary [   1.236s] 1 test run: 1 passed, 851 skipped

@folkertdev
Copy link
Contributor Author

ah yes it's probably a good idea to give a status update here:

crash now works with the dev backend: the message and the panic tag are sent over correctly. This now works for both linux and windows on x86_64. Linux was already tested on CI, and I've now added two tests (one that crashes, one that doesn't) on windows too so we don't regress.

On linux we can use the two return registers to send this information over to the setjmp call (which in roc terms returns a record of two u64 fields). For windows we had to get a bit more creative, because returning such a record is done via the stack. So, the setjmp/longjmp assembly is standard, except for how values are returned (and what/how many values).

Base automatically changed from alloca-expr to main July 31, 2023 13:53
Comment on lines 1452 to 1455
// a *const RocStr
let roc_str_ptr = R11;
ASM::mov_reg64_imm64(buf, roc_str_ptr, 16 + 24); // 24 is width of a rocstr
ASM::add_reg64_reg64_reg64(buf, roc_str_ptr, roc_str_ptr, RSP);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you shed a light on the constants here? maybe it is related to
argument_offset: X86_64WindowsFastcall::SHADOW_SPACE_SIZE as i32 + 16,?

Comment on lines +524 to +525
// move the first argument to roc_panic (a *RocStr) into r8
ASM::add_reg64_reg64_imm32(buf, R8, RSP, 8);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the 8 is kind of a magic constant here...

Copy link
Contributor

Choose a reason for hiding this comment

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

Skips over the return address on the stack?

Copy link
Contributor

@bhansconnect bhansconnect left a comment

Choose a reason for hiding this comment

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

Looks great! exciting to see this working.

&mut backend,
&mut output,
"roc_getppid".into(),
"malloc".into(),
Copy link
Contributor

Choose a reason for hiding this comment

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

wrapping malloc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, roc_getppid was used somehow in the builtins (I think)? so we needed to provide the symbol. I'll dig into this a bit, because it should not be required

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 suspect it is these functions from expect.zig

extern fn roc_shm_open(name: *const i8, oflag: c_int, mode: c_uint) c_int;
extern fn roc_mmap(addr: ?*anyopaque, length: c_uint, prot: c_int, flags: c_int, fd: c_int, offset: c_uint) *anyopaque;
extern fn roc_getppid() c_int;

Comment on lines +524 to +525
// move the first argument to roc_panic (a *RocStr) into r8
ASM::add_reg64_reg64_imm32(buf, R8, RSP, 8);
Copy link
Contributor

Choose a reason for hiding this comment

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

Skips over the return address on the stack?

@folkertdev folkertdev merged commit ed9ece0 into main Aug 3, 2023
10 checks passed
@folkertdev folkertdev deleted the dev-backend-roc-panic branch August 3, 2023 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants