eh_artiq: handle catch clauses appropriately #161

pca006132 · 2022-01-06T13:49:12+08:00

pca006132 commented

2022-01-06 13:49:12 +08:00

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

acaf388dbb eh_artiq: handle catch clauses appropriately

pca006132 added 1 commit 2022-01-06 14:04:13 +08:00

97ca72f7f1 libunwind: enable lto

sb10q commented

2022-01-07 17:24:13 +08:00

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

Poster

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

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

Poster

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

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.

No reviewers