M-Labs
/
artiq-zynq
10
1
Fork 19
Code Issues 13 Pull Requests Releases Wiki Activity

eh_artiq: handle catch clauses appropriately #161

Merged
sb10q merged 1 commits from pca006132/artiq-zynq:master into master 2024-08-17 17:37:22 +08:00
Conversation 5 Commits 1 Files Changed 1 +1
pca006132 commented 2022-01-06 13:49:12 +08:00
Contributor
Copy Link

Should be used together with https://github.com/m-labs/artiq/tree/landingpad

The code for the personality function and some of the DWARF decoding code is modified from gcc eh personality https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/libsupc%2B%2B/eh_personality.cc#L359

Previously, we always emit cleanup=True, executing every catch block when we unwind the stack. With this patch, we only emit cleanup=True when we really have a cleanup, i.e. a finally block. The personality function will compare the clauses and determine when to execute the catch block, resulting in about 20% performance improvement in a microbenchmark with a 1000 level of nested try catch (that does not match the raised exception).

There are still some part that I am not sure for this patch, but at least this passed all tests. I am not sure if this can be simply ported to riscv or requires some modification, as the exception handlings ABI should be different (this is EHABI). But I guess the overall code should be similar.

Should be used together with https://github.com/m-labs/artiq/tree/landingpad The code for the personality function and some of the DWARF decoding code is modified from gcc eh personality https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/libsupc%2B%2B/eh_personality.cc#L359 Previously, we always emit `cleanup=True`, executing every catch block when we unwind the stack. With this patch, we only emit `cleanup=True` when we really have a cleanup, i.e. a finally block. The personality function will compare the clauses and determine when to execute the catch block, resulting in about 20% performance improvement in a microbenchmark with a 1000 level of nested try catch (that does not match the raised exception). There are still some part that I am not sure for this patch, but at least this passed all tests. I am not sure if this can be simply ported to riscv or requires some modification, as the exception handlings ABI should be different (this is EHABI). But I guess the overall code should be similar.
pca006132 added 1 commit 2022-01-06 13:49:13 +08:00
pca006132 added 1 commit 2022-01-06 14:04:13 +08:00
sb10q commented 2022-01-07 17:24:13 +08:00
Owner
Copy Link

Should be used together with https://github.com/m-labs/artiq/tree/landingpad

Is it backward compatible though?

Why the "libunwind: enable lto" commit?

> Should be used together with https://github.com/m-labs/artiq/tree/landingpad Is it backward compatible though? Why the "libunwind: enable lto" commit?
pca006132 commented 2022-01-07 23:10:44 +08:00
Author
Contributor
Copy Link

Should be used together with https://github.com/m-labs/artiq/tree/landingpad

The compiler change: Yes, it should be. The original firmware will run the clause anyway during unwind, so the behavior should be the same.

These two changes (both compiler and firmware) would enable more efficient and idiomatic unwinding behavior, matching the LLVM documentation regarding exception handling.

Using the old compiler with this firmware would have issue due to differences in handling the except clause: the old one would emit a name but this patch expects a null pointer.

Why the "libunwind: enable lto" commit?

Enabling LTO improves the performance a bit, I thought this is a separate thing to do so I separated it into another commit.

> > Should be used together with https://github.com/m-labs/artiq/tree/landingpad The compiler change: Yes, it should be. The original firmware will run the clause anyway during unwind, so the behavior should be the same. These two changes (both compiler and firmware) would enable more efficient and idiomatic unwinding behavior, matching the LLVM documentation regarding exception handling. Using the old compiler with this firmware would have issue due to differences in handling the `except` clause: the old one would emit a name but this patch expects a null pointer. > Why the "libunwind: enable lto" commit? Enabling LTO improves the performance a bit, I thought this is a separate thing to do so I separated it into another commit.
sb10q commented 2022-01-08 12:52:36 +08:00
Owner
Copy Link

Using the old compiler with this firmware would have issue due to differences in handling the except clause: the old one would emit a name but this patch expects a null pointer.

And what about new compiler (i.e. with the landingpad change) with old firmware (i.e. without this change)?

If that doesn't work, we cannot merge until RISC-V is fixed.

> Using the old compiler with this firmware would have issue due to differences in handling the except clause: the old one would emit a name but this patch expects a null pointer. And what about new compiler (i.e. with the landingpad change) with old firmware (i.e. without this change)? If that doesn't work, we cannot merge until RISC-V is fixed.
pca006132 commented 2022-01-08 12:59:58 +08:00
Author
Contributor
Copy Link

Using the old compiler with this firmware would have issue due to differences in handling the except clause: the old one would emit a name but this patch expects a null pointer.

And what about new compiler (i.e. with the landingpad change) with old firmware (i.e. without this change)?

If that doesn't work, we cannot merge until RISC-V is fixed.

New compiler with old firmware should be fine. I haven't yet tested it though.

> > Using the old compiler with this firmware would have issue due to differences in handling the except clause: the old one would emit a name but this patch expects a null pointer. > > And what about new compiler (i.e. with the landingpad change) with old firmware (i.e. without this change)? > > If that doesn't work, we cannot merge until RISC-V is fixed. New compiler with old firmware should be fine. I haven't yet tested it though.
sb10q commented 2022-01-09 11:53:21 +08:00
Owner
Copy Link

OK, let's merge after testing.

OK, let's merge after testing.
sb10q merged commit 97ca72f7f1 into master 2022-01-10 15:54:46 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
priority:critical

   
priority:high

   
priority:low

   
priority:medium
No Label
priority:critical
priority:high
priority:low
priority:medium
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/artiq-zynq#161
No description provided.
Delete Branch "pca006132/artiq-zynq:master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?