feat: add sjlj exception handling #3

abm-77 commented 2025年05月12日 06:37:37 +02:00

This is a PR to add setjmp/longjmp exception handling to libzigunwind. Fixes #2.

This is a PR to add setjmp/longjmp exception handling to libzigunwind. Fixes #2.

abm-77 commented 2025年05月12日 07:09:35 +02:00

Hi @tensorush, this is my first long-form Zig contribution, so I am sure there are many mistakes. Happy to work together to get this into a state you envision for the project. I had a couple of questions about the SJLJ port:

1. How should we be referencing longjmp in unwindPhase2 and unwindPhase2Forced? Currently, I declare it as an external symbol.
2. How should we handle the __ve__ configuration? Right now, I default to 64-bit pointers for the resumeLocation and resumeParameters, but it seems like we should only do this when __ve__ is defined 32-bit pointers otherwise. Is this something we can get from builtin or the config?
3. How do you envision we switch between exception handling implementations?
4. Do you intend to port over the tests defined in the original libunwind, or should we look to create more comprehensive tests?

Hi @tensorush, this is my first long-form Zig contribution, so I am sure there are many mistakes. Happy to work together to get this into a state you envision for the project. I had a couple of questions about the SJLJ port: 1. How should we be referencing `longjmp` in `unwindPhase2` and `unwindPhase2Forced`? Currently, I declare it as an external symbol. 2. How should we handle the `__ve__` configuration? Right now, I default to 64-bit pointers for the `resumeLocation` and `resumeParameters`, but it seems like we should only do this when `__ve__` is defined 32-bit pointers otherwise. Is this something we can get from `builtin` or the config? 3. How do you envision we switch between exception handling implementations? 4. Do you intend to port over the [tests](https://github.com/allyourcodebase/libunwind/tree/main/libunwind/test) defined in the original libunwind, or should we look to create more comprehensive tests?

tensorush commented 2025年05月12日 07:55:35 +02:00

Hi, thanks for taking this on, here're my thoughts:

1. Yes, that's fine. That function is implemented in libc, so it will have (or maybe already has) a Zig implementation as part of libzigc.
2. #if defined(__ve__) translates to Zig as if (builtin.cpu.arch == .ve). So, in that instance it means that on the Ve architecture those fields should be u64, otherwise, they should be u32.
3. That one's tricky, it mostly depends on the ABI (builtin.abi). As Alex pointed out, it's worth looking at how Clang defines those macros, like __USING_SJLJ_EXCEPTIONS__ or __SEH__. Although, I haven't looked too much into it yet, I doubt it'll be as straightforward to do in Zig, so I think we might have to add another config option to switch on manually, at least for now.
4. Yep, I intend to incorporate them as test functions at the end of respective implementation files. More tests are definitely welcome.

Hi, thanks for taking this on, here're my thoughts: 1. Yes, that's fine. That function is implemented in `libc`, so it will have (or maybe already has) a Zig implementation as part of `libzigc`. 2. `#if defined(__ve__)` translates to Zig as `if (builtin.cpu.arch == .ve)`. So, in that instance it means that on the Ve architecture those fields should be `u64`, otherwise, they should be `u32`. 3. That one's tricky, it mostly depends on the ABI (`builtin.abi`). [As Alex pointed out](https://github.com/ziglang/zig/issues/23560#issuecomment-2845440625), it's worth looking at how Clang defines those macros, like `__USING_SJLJ_EXCEPTIONS__` or `__SEH__`. Although, I haven't looked too much into it yet, I doubt it'll be as straightforward to do in Zig, so I think we might have to add another config option to switch on manually, at least for now. 4. Yep, I intend to incorporate them as test functions at the end of respective implementation files. More tests are definitely welcome.

abm-77 commented 2025年05月13日 05:43:50 +02:00

1. Gotcha, I'll leave as is for now.
2. Cool, just made that change.
3. I see. I will add a build option like use_sjlj_exceptions. We can use this to conditionally link libc (unless we need it for anything else) and eventually conditionally enabling SJLJ exception handling when we decide on how we expose the functionality to users.
4. Great, makes sense.

1. Gotcha, I'll leave as is for now. 2. Cool, just made that change. 3. I see. I will add a build option like `use_sjlj_exceptions`. We can use this to conditionally link libc (unless we need it for anything else) and eventually conditionally enabling SJLJ exception handling when we decide on how we expose the functionality to users. 4. Great, makes sense.

abm-77 commented 2025年05月13日 08:48:03 +02:00

I think at this point I have faithfully ported over the original C file. I am currently looking to create functional tests for validating the implementation.

I think at this point I have faithfully ported over the original C file. I am currently looking to create functional tests for validating the implementation.

tensorush commented 2025年05月16日 10:29:08 +02:00

LGTM. Don't forget to change the PR from Draft when you're ready to have it merged.

LGTM. Don't forget to change the PR from Draft when you're ready to have it merged.

tensorush commented 2025年05月19日 17:30:42 +02:00

I think I'll go ahead and merge it with the rest now. Feel free to contribute more in another PR. Thanks again!

I think I'll go ahead and merge it with the rest now. Feel free to contribute more in another PR. Thanks again!

abm-77 commented 2025年05月19日 22:59:12 +02:00

Great, no worries. Sorry about the delayed communication!

Great, no worries. Sorry about the delayed communication!