Page MenuHomePhabricator

[RISCV] Fix wrong CFI directives
ClosedPublic

Authored by luismarques on Nov 1 2019, 11:20 AM.

Details

Summary

Currently there are CFI directives being emitted whose effects incorrectly propagate beyond the basic block they are in. For instance:

define void @branch_and_tail_call(i1 %a) {
; RV32-LABEL: branch_and_tail_call:
; RV32:       # %bb.0:
; RV32-NEXT:    addi sp, sp, -16
; RV32-NEXT:    .cfi_def_cfa_offset 16
; RV32-NEXT:    sw ra, 12(sp)
; RV32-NEXT:    .cfi_offset ra, -4
; RV32-NEXT:    andi a0, a0, 1
; RV32-NEXT:    beqz a0, .LBB2_2
; RV32-NEXT:  # %bb.1: # %blue_pill
; RV32-NEXT:    lw ra, 12(sp)
; RV32-NEXT:    .cfi_restore ra
; RV32-NEXT:    addi sp, sp, 16
; RV32-NEXT:    .cfi_def_cfa_offset 0
; RV32-NEXT:    tail foo
; RV32-NEXT:  .LBB2_2: # %red_pill
; RV32-NEXT:    call bar
; RV32-NEXT:    lw ra, 12(sp)
; RV32-NEXT:    .cfi_restore ra
; RV32-NEXT:    addi sp, sp, 16
; RV32-NEXT:    .cfi_def_cfa_offset 0
; RV32-NEXT:    ret

The last .cfi_restore ra will try to restore ra based on the incorrect offset of the .cfi_def_cfa_offset 0, when it should still be based on a .cfi_def_cfa_offset 16.

The emission of incorrect offsets affects the stack unwinding. For instance, without this patch the following program will not correctly unwind, and will get stuck (compile with clang, link with a gnu toolchain, test with qemu):

void three() {
    throw 7;
}

void two(void) {
    try {
        three();
    } catch(int &c) {
        throw 42;
    }
}

int main() {
    try {
        two();
    }
    catch(...) {
    }
    return 0;
}

This patch tries to solve this problem by not emitting the epilogue CFI directives, as is done for other LLVM targets (e.g. MIPS). The epilogue CFI directives were probably added to follow the footsteps of GCC. But GCC is able to solve this problem by, for instance, making use of .cfi_remember_state and .cfi_restore_state when necessary to make sure that the effects of CFI directives does not propagate beyond the intended BBs. Since adding support for that approach would probably not be trivial, for now we just disable emitting the epilogue directives. Feedback on whether there are issues with this simpler approach (and why it doesn't affect other LLVM targets) would be appreciated.

Diff Detail

Event Timeline

luismarques created this revision.Nov 1 2019, 11:20 AM

Hi @luismarques,
I tried to generate .cfi_remember_state and .cfi_restore_state in emitEpilogue. It could pass the test case.
But there is an issue that .cfi_restore_state will become incorrect if the shrink wrapping hoists the epilogue.
So I think to remove CFI directives from the epilogue as other targets do might be a reasonable step.

luismarques retitled this revision from [RISCV][RFC] Fix wrong CFI directives to [RISCV] Fix wrong CFI directives.

Rebase patch.
Remove [RFC] From title.

luismarques edited the summary of this revision. (Show Details)

Rebase again, after adding nounwind to some tests in master.
Remove outdated summary text.

shiva0217 added inline comments.Nov 11 2019, 9:25 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
270–271

Should we remove this cfi directive, too?

lenary accepted this revision.Nov 12 2019, 7:13 AM

LGTM. Remove the lines noted by @shiva0217, and then I think this can be committed.

This revision is now accepted and ready to land.Nov 12 2019, 7:13 AM

Remove remaining CFI directive from epilogue.

luismarques marked 2 inline comments as done.Nov 12 2019, 7:26 AM
luismarques added inline comments.
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
270–271

Yup, that was a silly oversight. Thanks!

This revision was automatically updated to reflect the committed changes.
luismarques marked an inline comment as done.

Update the test DebugInfo/RISCV/relax-debug-frame.ll, which wasn't included in the original patch.
Please re-review the impact of the patch in that test.

luismarques reopened this revision.Nov 13 2019, 7:52 AM

Re-opening, to make sure the changes to relax-debug-frame.ll are reasonable.

This revision is now accepted and ready to land.Nov 13 2019, 7:52 AM
shiva0217 added inline comments.Nov 14 2019, 7:39 AM
llvm/test/DebugInfo/RISCV/relax-debug-frame.ll
15–16

I think it might a reasonable changing. According to https://github.com/riscv/riscv-binutils-gdb/blob/riscv-binutils-2.29/gas/config/tc-riscv.c#L1940, R_RISCV_SET6 and R_RISCV_SUB6 are relocation types for DW_CFA_advance_loc. It seems that the check line didn't show all the relocation types of the object, the number of the reduction of R_RISCV_SET6 and R_RISCV_SUB6 reflect the reduction of the CFI directives.

luismarques marked 2 inline comments as done.Nov 14 2019, 9:10 AM
luismarques added inline comments.
llvm/test/DebugInfo/RISCV/relax-debug-frame.ll
15–16

Thanks, that's exactly the kind of reassuring feedback I was looking for! :-)

This revision was automatically updated to reflect the committed changes.
luismarques marked an inline comment as done.
HsiangKai added inline comments.
llvm/test/DebugInfo/RISCV/relax-debug-frame.ll
15–16

@shiva0217 is right. In DWARF definition, DW_CFA_advance_loc's operand is 6-bits width offset. These two relocation types are for 6-bits offset. These two relocation types should be similar to R_RISCV_ADD32 and R_RISCV_SUB32 besides the data width.