[RISCV] Missing cfi_def_cfa_offset?

[topperc]

For some stack adjustmnets, the RISC-V backend may emit a 2 adjustments of the stack pointer without a cfi_def_cfa_offset between them. This occurs when the stack adjust is roughly between +/-2048 and +/-4096. In that case we use 2 ADDIs instead of LUI+ADDI+ADD. gcc does something similar, but issues a cfi_def_cfa_offset between them. Should LLVM be emitting a cfi_def_cfa_offset

Example comparing llvm and gcc https://godbolt.org/z/eh63zn7b1

CC: @lenary @ilovepi @MaskRay @preames

[llvmbot]

@llvm/issue-subscribers-backend-risc-v

Author: Craig Topper (topperc)

For some stack adjustmnets, the RISC-V backend may emit a 2 adjustments of the stack pointer without a cfi_def_cfa_offset between them. This occurs when the stack adjust is roughly between +/-2048 and +/-4096. In that case we use 2 ADDIs instead of LUI+ADDI+ADD. gcc does something similar, but issues a cfi_def_cfa_offset between them. Should LLVM be emitting a cfi_def_cfa_offset

Example comparing llvm and gcc https://godbolt.org/z/eh63zn7b1

CC: @lenary @ilovepi @MaskRay @preames

[lenary]

I think the answer depends on whether we want CFI to be accurate at every instruction in the function (including the prolog), or only once the prolog is complete.

I forget which one of those options we're following for RISC-V, and which choice GCC has made there.

From the output, it doesn't look like GCC is accurate at every instruction in the prolog, because they have delayed emitting info about ra's offset (but there's not an instruction where the calculated CFA is inaccurate, unlike with clang's sequence).

I think keeping the CFA accurate is probably more important than immediately noting where a specific (and not-yet-clobbered) register has been saved.

I don't have a problem with emitting it to be closer to GCC, I presume there's a size overhead but we'd find it acceptable for the improved accuracy.

[ilovepi]

I think the answer depends on whether we want CFI to be accurate at every instruction in the function (including the prolog), or only once the prolog is complete.

I forget which one of those options we're following for RISC-V, and which choice GCC has made there.

AFAIK DWARF CFI is supposed to be accurate at every instruction in the prolog to support even basic unwinding. The various codegen options allow users to specify how accurate they want the DWARF for (e.g. full async unwind info vs only accurate at call sites, etc). I think anything that needs the more detailed unwind info, like C++ exceptions, needs the CFA to be accurate at every instruction, but thats not an aspect I've had to deal w/ too much.

In my experience, GCC is typically much better about generating debug info than we are, and seems to be doing the right thing.

From the output, it doesn't look like GCC is accurate at every instruction in the prolog, because they have delayed emitting info about ra's offset (but there's not an instruction where the calculated CFA is inaccurate, unlike with clang's sequence).

I think keeping the CFA accurate is probably more important than immediately noting where a specific (and not-yet-clobbered) register has been saved.

Agreed. This should be fine since ra hasn't been updated yet, and its current value as being defined as same_value is still correct.

I don't have a problem with emitting it to be closer to GCC, I presume there's a size overhead but we'd find it acceptable for the improved accuracy.

IDK if its accuracy, so much as w/in that part of the prologue you'd potentially load the wrong RA in the debugger if you were stepping through it/unwinding.

[lenary]

I must be misremembering the difference between async and sync unwind tables. I think a debugger with Dwarf info (e.g. async unwind tables) will only stop at the end of the prolog when doing so automatically, but I see the value in ensuring the CFA is accurate throughout.

We landed #110810 about a year ago which aimed to make CFI information correct in the epilog, but there have been other changes to the code structure since, which might need looking at.

[MaskRay]

The more accurate DWARF CFI can be implemented in the -fasynchronous-unwind-tables mode. The -fno-asynchronous-unwind-tables -funwind-tables mode can omit them for smaller .eh_frame. If RISC-V implements -fstack-clash-protection, there will be more complexity. IMO the needs for accurate asynchronous unwind information is quite low.

Thanks for tagging me and providing this example. I see that prologue instructions are quite irregular for RISC-V, making asynchronous compact unwind information challenging. I am adding more notes to the RISC-V section of https://maskray.me/blog/2020-11-08-stack-unwinding#compact-unwind-information

[lenary]

With regards to this particular sequence, the problem is that RISCVRegisterInfo::adjustReg may emit two ADDIs:

llvm-project/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

Lines 335 to 340 in 91010a3

	// Try to split the offset across two ADDIs. We need to keep the intermediate
	// result aligned after each ADDI. We need to determine the maximum value we
	// can put in each ADDI. In the negative direction, we can use -2048 which is
	// always sufficiently aligned. In the positive direction, we need to find the
	// largest 12-bit immediate that is aligned. Exclude -4096 since it can be
	// created with LUI.

And evidently we don't have a way of emitting a .cfa_def_cfa_offset between them.

I'm not sure what the right approach is here, because this function isn't exactly straightforward. I don't know if we want to add another parameter either, saying something like "emit only one instruction", as it makes the codegen worse.

I'm also not super into the idea of looking at what adjustReg inserted, and working out whether to emit a CFI instruction between them.

Does anyone have thoughts about this?

[lenary]

I looked at using a scratch register in adjustReg, but this causes more problems than it solves - the addis wouldn't have a hope of being compressible (at most one will be today), and we don't necessarily have enough spill slots for the virtual scratch register (and allocating another seems silly).