Page MenuHomePhabricator

[PowerPC] Fix unwind info with dynamic stack realignment
ClosedPublic

Authored by foad on Nov 25 2014, 3:01 PM.

Details

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.

Diff Detail

Event Timeline

foad updated this revision to Diff 16633.Nov 25 2014, 3:01 PM
foad retitled this revision from to [PowerPC] Fix unwind info with dynamic stack realignment.
foad updated this object.
foad edited the test plan for this revision. (Show Details)
foad added reviewers: hfinkel, willschm.
foad added a subscriber: Unknown Object (MLST).
willschm edited edge metadata.Nov 26 2014, 7:11 AM

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
734

s/Otherwise, we/We/

736

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 added inline comments.Nov 26 2014, 7:14 AM
lib/Target/PowerPC/PPCFrameLowering.cpp
734

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 added inline comments.Nov 26 2014, 7:59 AM
lib/Target/PowerPC/PPCFrameLowering.cpp
734

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 updated this revision to Diff 16654.Nov 26 2014, 8:30 AM
foad edited edge metadata.

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 added a comment.Nov 26 2014, 9:40 AM

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 edited edge metadata.Nov 27 2014, 6:14 PM

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 added a comment.Nov 27 2014, 11:14 PM
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 edited edge metadata.Nov 28 2014, 4:22 AM
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 updated this revision to Diff 16737.Nov 28 2014, 7:23 AM
foad edited edge metadata.
  • Add some comments prompted by Will.
  • Refer to "call frame information" instead of "frame moves" or "machine moves".
foad added a comment.Nov 28 2014, 7:29 AM

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 accepted this revision.Nov 30 2014, 4:51 PM
hfinkel edited edge metadata.
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.

This revision is now accepted and ready to land.Nov 30 2014, 4:51 PM
foad closed this revision.Dec 1 2014, 1:42 AM