This is an archive of the discontinued LLVM Phabricator instance.

[AsmPrinter] Make the encoding of call sites in .gcc_except_table configurable
ClosedPublic

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

Details

Summary

The original behavior was to always emit the offsets to each call site in the call site table as uleb128 values, however on some architectures (eg RISCV) these uleb128 offsets into the code cannot always be resolved until link time (because relaxation will invalidate any calculated offsets), and there are no appropriate relocations for uleb128 values. As a consequence it needs to be possible to specify an alternative.

This also switches RISCV to use DW_EH_PE_udata4 for call side encodings in .gcc_except_table

Diff Detail

Event Timeline

edward-jones created this revision.Jun 17 2019, 4:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2019, 4:48 AM
edward-jones retitled this revision from [WIP][AsmPrinter] Make the encoding of call sites in .gcc_except_table configurable to [AsmPrinter] Make the encoding of call sites in .gcc_except_table configurable.
edward-jones edited the summary of this revision. (Show Details)

Rebased against master

asb added a comment.Jul 2 2019, 3:15 AM

Hi Ed, I think it does make sense to merge D63416 into this one. Sometimes it makes sense to separate generic changes from the target-dependent code that uses them, but given that:

  • This addition is motivated by the needs of RISC-V
  • D63416 is the only reasonable way to test the change
  • D63416 is a tiny patch anyway

I think it makes sense to combine in this case, so we can review+commit in an atomic unit.

Is this something that should be specified in the RISC-V psabi doc do you think? If so, might you prepare a pull request for it?

luismarques added inline comments.
lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
189

Regarding the 0x7 magic constant, it's not very clear in this context why that is the right value. Can we replace that with a symbolic constant, or at least add some context (a comment, etc.) to make it clear to readers the logic behind that value?

197

Ditto magic constant.

edward-jones edited the summary of this revision. (Show Details)

Merged D63416 into this patch, rebased.

I've stuck a comment above the magic 0x7 constant because there's not a constant specified for the mask value already.

I'll look into changes that would need to be made to the psabi docs too.

lenary added a subscriber: lenary.Jul 12 2019, 3:50 AM
lenary added inline comments.
test/CodeGen/RISCV/exception.ll
94 ↗(On Diff #207766)

Do you think that extra patches will be required to emit these relocations correctly?

We're running into errors with overlapping FDEs (even after applying this patch), and fear it may be an interaction of eh_frame data, relocations and relaxation (which also seems to have an effect).

edward-jones marked an inline comment as done.Jul 15 2019, 4:06 AM
edward-jones added inline comments.
test/CodeGen/RISCV/exception.ll
94 ↗(On Diff #207766)

I've just submitted another patch (D64715) which might help with an "overlapping FDEs" error.

asb accepted this revision.Jul 17 2019, 6:58 AM

LGTM, thanks. I've spent quite a bit of time comparing output vs gcc so I'm pretty confident here now. I committed a slightly different test strategy in the DWARF encoding work I just committed, so will adjust this patch to build on that.

This revision is now accepted and ready to land.Jul 17 2019, 6:58 AM
This revision was automatically updated to reflect the committed changes.