[PowerPC] Fix unwind info with dynamic stack realignment

[foad]

Summary

PowerPC DWARF unwind info defined CFA as SP + offset even in a function
where the stack had been dynamically realigned. This clearly doesn't
work because the offset from SP to CFA is not a constant. Fix it by
defining CFA as BP instead.

This was causing the AddressSanitizer null_deref test to fail 50% of
the time, depending on whether SP happened to be 32-byte aligned on
entry to a particular function or not.

[willschm]

This looks straightforward.. A cosmetic comment inline, but I've no issues with the patch.
I defer to Uli/Hal for their approval.

lib/Target/PowerPC/PPCFrameLowering.cpp

737

s/Otherwise, we/We/

739

I'd be tempted to use something other than 'Reg' here, but looks like many other callers to this function also use Reg, so.. just a passing thought, no objection. :-)

[foad]

lib/Target/PowerPC/PPCFrameLowering.cpp

737

The "otherwise" is there for a reason: we only get to choose between BP and SP if there has *not* been any dynamic stack realignment. I'm open to ideas for rewording the whole thing it it'll make it easier to understand.

[willschm]

lib/Target/PowerPC/PPCFrameLowering.cpp

737

Ok, yeah, so partly due to the location of the comment, i misinterpreted the intent of 'otherwise' :-) How about pulling the comment out of the if statement, and adding a hint for the (HasBP) if/otherwise like so ?

	​ // If the stack has been dynamically realigned (HasBP) then we must define CFA in
	​ // terms of BP, because it's not at a fixed offset from SP.
	​ // Otherwise, we could define the CFA in terms of either BP or SP, but BP is
	​ // easier because the offset is always zero.
	​ if (HasBP) {

[foad]

Move the comment as suggested by Will. It got a bit more verbose because
I wanted to be clear that "stack was realigned" implies HasBP, but not
vice versa.

[foad]

Incidentally, is there any need to have accurate unwind info *before* we copy SP to FP? If not, we could do it all after the copy to FP, simplifying the code a bit and probably saving the odd byte in the CFI encoding:

if (HasBP)
  .cfi_def_cfa_register BP
else if (HasFP)
  .cfi_def_cfa FP, NegFrameSize
else
  .cfi_def_cfa_offset NegFrameSize

[hfinkel]

Thanks for working on this. I know very little about the CFI directives, and so I'm afraid this might not be the end of the story; specifically, I'm worried about the meaning of the offsets used for the other registers.

When we have a stack pointer or a frame pointer, these store the values of those registers after being adjusted for the local stack size. When we use a base pointer, the base pointer hold the pre-adjusted r1 value. So in PPCRegisterInfo::eliminateFrameIndex we have this:

if (!MF.getFunction()->getAttributes().
      hasAttribute(AttributeSet::FunctionIndex, Attribute::Naked)) {
  if (!(hasBasePointer(MF) && FrameIndex < 0))
    Offset += MFI->getStackSize();
}

because to access parent-frame relative objects using the stack/frame pointer we need to add back the local stack size, but if we're using a base pointer, we don't. Maybe this is all fine, but using the same offsets when calling MCCFIInstruction::createOffset for the sp/fp and the bp seems suspicious. Can you please confirm that the offsets are indeed correct in all cases, and regardless, please add some comments explaining your conclusion.

[foad]

In D6410#11, @hfinkel wrote:

Thanks for working on this. I know very little about the CFI directives, and so I'm afraid this might not be the end of the story; specifically, I'm worried about the meaning of the offsets used for the other registers.

The CFI directives are doing two things:

1. The createDefCfa* functions define the Canonical Frame Address, i.e. the value of SP on entry to the function, in terms of some native register.
2. The createOffset function says where a register was saved on the stack, as an offset from the CFA.

I have fixed (1) in a function that dynamically realigns the stack. This should not affect (2) in any way. Assuming the offsets passed into createOffset were correct before, they should still be correct now.

I've just had a look at all the calls to createOffset and they look OK to me, except that I don't understand all the complexities in handling CR. But like I said, my changes should not affect their correctness.

Maybe this is all fine, but using the same offsets when calling MCCFIInstruction::createOffset for the sp/fp and the bp seems suspicious. Can you please confirm that the offsets are indeed correct in all cases, and regardless, please add some comments explaining your conclusion.

I think it's all fine, but it'd be really nice to have some end-to-end tests to verify that a debugger/unwinder can successfully debug/unwind the generated code.

How about a one-line comment before the CFI stuff for each saved register, like:

// Describe where FP was saved as an offset from the CFA.
if (HasFP) {
  unsigned Reg = MRI->getDwarfRegNum(FPReg, true);
  CFIIndex = MMI.addFrameInst(
      MCCFIInstruction::createOffset(nullptr, Reg, FPOffset));
  BuildMI(MBB, MBBI, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
      .addCFIIndex(CFIIndex);
}

[uweigand]

In D6410#10, @foad wrote:

Incidentally, is there any need to have accurate unwind info *before* we copy SP to FP? If not, we could do it all after the copy to FP, simplifying the code a bit and probably saving the odd byte in the CFI encoding:

if (HasBP)
  .cfi_def_cfa_register BP
else if (HasFP)
  .cfi_def_cfa FP, NegFrameSize
else
  .cfi_def_cfa_offset NegFrameSize

Yes, we should have accurate unwind info at any position in the function, so that debugging / stack backtraces etc. work correctly. This means we need to define CFA in terms of SP first, and then switch to a definition in terms of FP/BP as soon as the original definition becomes invalid due to code changing SP.

[foad]

• Add some comments prompted by Will.
• Refer to "call frame information" instead of "frame moves" or "machine moves".

[foad]

Yes, we should have accurate unwind info at any position in the function, so that debugging / stack backtraces etc. work correctly. This means we need to define CFA in terms of SP first, and then switch to a definition in terms of FP/BP as soon as the original definition becomes invalid due to code changing SP.

OK, thanks. I think the current patch gets this right. If we're using BP then the definition CFA=BP kicks in immediately after SP is updated. I haven't changed the FP case, but it looks correct too.

[hfinkel]

In D6410#16, @foad wrote:

Yes, we should have accurate unwind info at any position in the function, so that debugging / stack backtraces etc. work correctly. This means we need to define CFA in terms of SP first, and then switch to a definition in terms of FP/BP as soon as the original definition becomes invalid due to code changing SP.

OK, thanks. I think the current patch gets this right. If we're using BP then the definition CFA=BP kicks in immediately after SP is updated. I haven't changed the FP case, but it looks correct too.

Okay, thanks. LGTM.

lib/Target/PowerPC/PPCFrameLowering.cpp

BuildMI(MBB, MBBI, dl, StoreUpdtIdxInst, SPReg)
       .addReg(SPReg, RegState::Kill)
       .addReg(SPReg)
       .addReg(ScratchReg);
   }
 
   // Add the "machine moves" for the instructions we generated above, but in
   // reverse order.
   if (needsFrameMoves) {
+    unsigned CFIIndex;
+
+    if (HasBP) {
+      // If the stack has been dynamically realigned then we must define CFA in
+      // terms of BP, because it's not at a fixed offset from SP.
+      // Otherwise, we could define it in terms of either BP or SP, but BP is
+      // easier because the offset is always zero.
+      unsigned Reg = MRI->getDwarfRegNum(BPReg, true);
+      CFIIndex = MMI.addFrameInst(
+          MCCFIInstruction::createDefCfaRegister(nullptr, Reg));
+    } else {
       // Show update of SP.
       assert(NegFrameSize);
-    unsigned CFIIndex = MMI.addFrameInst(
+      CFIIndex = MMI.addFrameInst(
           MCCFIInstruction::createDefCfaOffset(nullptr, NegFrameSize));
+    }
     BuildMI(MBB, MBBI, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
         .addCFIIndex(CFIIndex);
 
     if (HasFP) {
       unsigned Reg = MRI->getDwarfRegNum(FPReg, true);
       CFIIndex = MMI.addFrameInst(
           MCCFIInstruction::createOffset(nullptr, Reg, FPOffset));
       BuildMI(MBB, MBBI, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
void PPCFrameLowering::emitPrologue(MachineFunction &MF) const {
   }
 
   // If there is a frame pointer, copy R1 into R31
   if (HasFP) {
     BuildMI(MBB, MBBI, dl, OrInst, FPReg)
       .addReg(SPReg)
       .addReg(SPReg);
 
-    if (needsFrameMoves) {
+    if (!HasBP && needsFrameMoves) {
       // Mark effective beginning of when frame pointer is ready.
       unsigned Reg = MRI->getDwarfRegNum(FPReg, true);
       unsigned CFIIndex = MMI.addFrameInst(
           MCCFIInstruction::createDefCfaRegister(nullptr, Reg));
 
       BuildMI(MBB, MBBI, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
           .addCFIIndex(CFIIndex);
     }

test/CodeGen/PowerPC/stack-realign.ll

 ; CHECK-DAG: mflr 0
 ; CHECK-DAG: rldicl [[REG:[0-9]+]], 1, 0, 59
 ; CHECK-DAG: std 30, -16(1)
 ; CHECK-DAG: mr 30, 1
 ; CHECK-DAG: std 0, 16(1)
 ; CHECK-DAG: subfic 0, [[REG]], -160
 ; CHECK: stdux 1, 1, 0
 
+; CHECK: .cfi_def_cfa_register r30
 ; CHECK: .cfi_offset r30, -16
 ; CHECK: .cfi_offset lr, 16
 
 ; CHECK: std 3, 48(30)
 
 ; CHECK: ld 1, 0(1)
 ; CHECK-DAG: ld 0, 16(1)
 ; CHECK-DAG: ld 30, -16(1)
 ; CHECK-DAG: mtlr 0
 ; CHECK: blr
 
 ; CHECK-FP-LABEL: @goo
 
 ; CHECK-FP-DAG: mflr 0
 ; CHECK-FP-DAG: rldicl [[REG:[0-9]+]], 1, 0, 59
 ; CHECK-FP-DAG: std 31, -8(1)
 ; CHECK-FP-DAG: std 30, -16(1)
 ; CHECK-FP-DAG: mr 30, 1
 ; CHECK-FP-DAG: std 0, 16(1)
 ; CHECK-FP-DAG: subfic 0, [[REG]], -160
 ; CHECK-FP: stdux 1, 1, 0
 
+; CHECK-FP: .cfi_def_cfa_register r30
 ; CHECK-FP: .cfi_offset r31, -8
 ; CHECK-FP: .cfi_offset r30, -16
 ; CHECK-FP: .cfi_offset lr, 16
 
 ; CHECK-FP: mr 31, 1
 
 ; CHECK-FP: std 3, 48(30)
 

 ; CHECK-DAG: mflr 0
 ; CHECK-DAG: ori [[REG2:[0-9]+]], [[REG1]], 51808
 ; CHECK-DAG: std 30, -16(1)
 ; CHECK-DAG: mr 30, 1
 ; CHECK-DAG: std 0, 16(1)
 ; CHECK-DAG: subfc 0, [[REG3]], [[REG2]]
 ; CHECK: stdux 1, 1, 0
 
+; CHECK: .cfi_def_cfa_register r30
+
 ; CHECK: blr
 
 ; CHECK-32-LABEL: @hoo
 
 ; CHECK-32-DAG: lis [[REG1:[0-9]+]], -13
 ; CHECK-32-DAG: rlwinm [[REG3:[0-9]+]], 1, 0, 27, 31
 ; CHECK-32-DAG: mflr 0
 ; CHECK-32-DAG: ori [[REG2:[0-9]+]], [[REG1]], 51904

 ; CHECK-DAG: mflr 0
 ; CHECK-DAG: rldicl [[REG:[0-9]+]], 1, 0, 59
 ; CHECK-DAG: std 30, -32(1)
 ; CHECK-DAG: mr 30, 1
 ; CHECK-DAG: std 0, 16(1)
 ; CHECK-DAG: subfic 0, [[REG]], -192
 ; CHECK: stdux 1, 1, 0
 
+; CHECK: .cfi_def_cfa_register r30
+
 ; CHECK: stfd 30, -16(30)
 
 ; CHECK: blr
 
 ; CHECK-FP-LABEL: @loo
 
 ; CHECK-FP-DAG: mflr 0
 ; CHECK-FP-DAG: rldicl [[REG:[0-9]+]], 1, 0, 59
 ; CHECK-FP-DAG: std 31, -24(1)
 ; CHECK-FP-DAG: std 30, -32(1)
 ; CHECK-FP-DAG: mr 30, 1
 ; CHECK-FP-DAG: std 0, 16(1)
 ; CHECK-FP-DAG: subfic 0, [[REG]], -192
 ; CHECK-FP: stdux 1, 1, 0
 
+; CHECK-FP: .cfi_def_cfa_register r30
+
 ; CHECK-FP: stfd 30, -16(30)
 
 ; CHECK-FP: blr