This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Specify various encodings for DWARF exception handling
AbandonedPublic

Authored by edward-jones on Jun 17 2019, 4:38 AM.

Details

Summary

This specifies the encodings used for the personality function, LSDA and type info for exception handling.

Diff Detail

Event Timeline

edward-jones created this revision.Jun 17 2019, 4:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2019, 4:38 AM
edward-jones retitled this revision from [WIP][RISCV] Specify various encodings for DWARF exception handling to [RISCV] Specify various encodings for DWARF exception handling.
edward-jones edited the summary of this revision. (Show Details)

Added tests, rebased against master

I think D63409, D63411, D63415, and D63416 could be merged as one patch. They are all related to exception handling for RISC-V.

I think D63409, D63411, D63415, and D63416 could be merged as one patch. They are all related to exception handling for RISC-V.

I kept them as separate changes since they're incremental and I thought it would make things easier to review, but I can squash them into one patch if that is more convenient. At the very least I plan on keeping D63415 separate as it includes generic changes.

I think D63409, D63411, D63415, and D63416 could be merged as one patch. They are all related to exception handling for RISC-V.

I kept them as separate changes since they're incremental and I thought it would make things easier to review, but I can squash them into one patch if that is more convenient. At the very least I plan on keeping D63415 separate as it includes generic changes.

I found that the test case in this patch will trigger an assertion failed without D63411 and there is no test case for D63411. So, I suggest these two patches could be combined at least. It will make review and verification easier.

If you want to keep D63415 as a stand alone patch, you should create a test case for it. I think D63415 and D63416 could be combined. You could think D63416 is a show case about how to specify “call site encoding” in architecture dependent part and why you want to do these generic changes. It is only my opinion. For your reference. BTW, your modification looks good.

asb added a comment.Jul 16 2019, 1:52 AM

Hi Ed, this is really hard to review without any documentation in the psABI or otherwise. Can you confirm that you've verified this matches gcc/gas? Are you planning to submit document to riscv-elf-psabi-doc to reflect this?

In D63409#1587117, @asb wrote:

Hi Ed, this is really hard to review without any documentation in the psABI or otherwise. Can you confirm that you've verified this matches gcc/gas? Are you planning to submit document to riscv-elf-psabi-doc to reflect this?

I've opened a pull request on the psABI docs (https://github.com/riscv/riscv-elf-psabi-doc/pull/110). I'm not entirely sure the details here are relevant to the ABI though. As far as I'm aware provided the individual CIE and FDEs are well-formed it shouldn't matter if the encodings differ from those specified in other CIE and FDEs.

On another note, after taking a look at the GCC code for this I'm not convinced the non-PIC values for the encodings match. GCC appears to just encode everything as pc-relative sdata4, whereas the above encodes things as absolute when PIC is not specified.

asb added a comment.Jul 16 2019, 4:56 AM
In D63409#1587117, @asb wrote:

Hi Ed, this is really hard to review without any documentation in the psABI or otherwise. Can you confirm that you've verified this matches gcc/gas? Are you planning to submit document to riscv-elf-psabi-doc to reflect this?

I've opened a pull request on the psABI docs (https://github.com/riscv/riscv-elf-psabi-doc/pull/110). I'm not entirely sure the details here are relevant to the ABI though. As far as I'm aware provided the individual CIE and FDEs are well-formed it shouldn't matter if the encodings differ from those specified in other CIE and FDEs.

On another note, after taking a look at the GCC code for this I'm not convinced the non-PIC values for the encodings match. GCC appears to just encode everything as pc-relative sdata4, whereas the above encodes things as absolute when PIC is not specified.

Yes, I was trying to cross-reference to what GCC does and couldn't see a match for everything you specify here.

Many thanks for submitting that PR, as you say it's not clear where it belongs but it's certainly useful to have it written down somewhere!

asb resigned from this revision.Aug 19 2019, 6:24 AM

Resigning as a reviewer, as the intent of this change is captured in rL366327.

edward-jones abandoned this revision.Aug 20 2019, 1:42 AM

Abandoned since this was superceded by rL366327.