[X86] Emit .cfi_escape GNU_ARGS_SIZE when adjusting the stack before calls

[mkuper]

Summary

This should hopefully fix the exception handling issues in PR24792.

There are two things I'd appreciate an opinion on:

a) Does it make sense to expose it like this to FrameLowering, or should the interface be more abstract, and actually turned into a .cfi_escape somewhere lower?

b) I'm not sure how to optimize away consequent directives that provide the same size. I don't think this can be done in eliminateCallFramePseudoInstr(), because (1) it's not guaranteed to be called in program order (even though in practice I think that is what happens when we have pushes), and (2) I think the basic-block order can change later on.
It may make sense to eliminate the redundant directives somewhere in the MC layer, but I'm not sure where that can fit in. Any suggestions?

[rnk]

a) Does it make sense to expose it like this to FrameLowering, or should the interface be more abstract, and actually turned into a .cfi_escape somewhere lower?

I'd make it more abstract internally. I'm thinking MCStreamer::EmitGnuArgsSize, and then the two streamer impls can add the 0x2e internally.

b) I'm not sure how to optimize away consequent directives that provide the same size. I don't think this can be done in eliminateCallFramePseudoInstr(), because (1) it's not guaranteed to be called in program order (even though in practice I think that is what happens when we have pushes), and (2) I think the basic-block order can change later on.
It may make sense to eliminate the redundant directives somewhere in the MC layer, but I'm not sure where that can fit in. Any suggestions?

I wouldn't worry about it now. If we keep it abstract, we can do the optimization as part of EmitCFIInstructions without digging into escaped CFI sequences.

lib/Target/X86/X86FrameLowering.cpp

2073

We shouldn't emit this if we have an FP for the frame.

[mkuper]

Thanks Reid,
I'll upload an updated patch with a cleaner interface.

lib/Target/X86/X86FrameLowering.cpp

2073

That was my original thought too, but I talked about it with Dave, and he suggested that was incorrect.
E.g.:

while (...)

try {
   foo(...);
   ...
} catch (...) {
   ...
}

}

While all stack references will be fine without gnu_args_size (assuming FP), the stack may overflow due to the missing cleanups.

[majnemer]

lib/MC/MCAsmStreamer.cpp

1023

!Values.empty()

lib/Target/X86/X86FrameLowering.cpp

2074

Why not use DW_CFA_GNU_args_size ?

test/CodeGen/X86/push-cfi.ll

71

"resest"

98

resest

116

actually efficient

[mkuper]

Thanks, David!

lib/MC/MCAsmStreamer.cpp

1023

Right, thanks!

lib/Target/X86/X86FrameLowering.cpp

2074

No good reason, only noticed it exists after I uploaded the patch.

[mkuper]

Updated the interface.
I abused the "Offset" field of MCCFIInstruction for the size - but I think this actually makes sense. If anyone prefers a new field, let me know.

The optimization of removing duplicate .cfi_escapes will go into a separate commit, once I figure out where it fits in.

[DavidKreitzer]

lib/MC/MCAsmStreamer.cpp

1036

Is there a reason you made Size signed? DW_CFA_GNU_args_size takes an unsigned value. (Same question for the other routines that take a signed Size.)

1039

It would be safer to make Buffer a little bigger so that you could encode any possible uint64_t. I think the maximum ULEB encoding for a 64-bit integer would be 10 bytes?

lib/Target/X86/X86FrameLowering.cpp

2053

I believe DW_CFA_GNU_args_size is only needed in routines that contain EH handlers. It is not needed for debugging, nor is it needed for routines that need unwind table entries but have no handlers. So I don't think this is quite the right condition. It will result in more directives than necessary.

[mkuper]

Thanks, Dave!

lib/MC/MCAsmStreamer.cpp

1036

For consistency with the rest of the interface, mostly.

1039

Oh my. Of course!
I copied this from a different place that uses an 8-byte buffer, but there it's safer because it feeds an unsigned int directly into encodeULEB128()...

lib/Target/X86/X86FrameLowering.cpp

2053

Makes sense. I'll refine the condition.

[mkuper]

lib/Target/X86/X86FrameLowering.cpp

2053

Actually, if I understand things correctly now (and thanks for the patience!), we only need it before what was in IR an invoke, never before an IR call.
Is that right?

[mkuper]

Fixed buffer overflow, and changed the condition to only add GNU_ARGS_SIZE if there are any landing pads in the function.
We can probably do better (by adding them only for invokes), but I'd rather leave that for a separate patch.

[DavidKreitzer]

lib/Target/X86/X86FrameLowering.cpp

2057

That sounds right to me.

[mkuper]

ping

[rnk]

lgtm

Thanks for the ping and the fix!

lib/Target/X86/X86FrameLowering.cpp

2073

Hm, got it.

lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp

case MCCFIInstruction::OpRegister:
     OutStreamer->EmitCFIRegister(Inst.getRegister(), Inst.getRegister2());
     break;
   case MCCFIInstruction::OpWindowSave:
     OutStreamer->EmitCFIWindowSave();
     break;
   case MCCFIInstruction::OpSameValue:
     OutStreamer->EmitCFISameValue(Inst.getRegister());
     break;
+  case MCCFIInstruction::OpEscape:
+    OutStreamer->EmitCFIEscape(Inst.getValues());
+    break;
   }
 }
 
 void AsmPrinter::emitDwarfDIE(const DIE &Die) const {
   // Emit the code (index) for the abbreviation.
   if (isVerbose())
     OutStreamer->AddComment("Abbrev [" + Twine(Die.getAbbrevNumber()) + "] 0x" +
                             Twine::utohexstr(Die.getOffset()) + ":0x" +

lib/MC/MCAsmStreamer.cpp

public:
   void EmitCFIOffset(int64_t Register, int64_t Offset) override;
   void EmitCFIPersonality(const MCSymbol *Sym, unsigned Encoding) override;
   void EmitCFILsda(const MCSymbol *Sym, unsigned Encoding) override;
   void EmitCFIRememberState() override;
   void EmitCFIRestoreState() override;
   void EmitCFISameValue(int64_t Register) override;
   void EmitCFIRelOffset(int64_t Register, int64_t Offset) override;
   void EmitCFIAdjustCfaOffset(int64_t Adjustment) override;
+  void EmitCFIEscape(StringRef Values) override;
   void EmitCFISignalFrame() override;
   void EmitCFIUndefined(int64_t Register) override;
   void EmitCFIRegister(int64_t Register1, int64_t Register2) override;
   void EmitCFIWindowSave() override;
 
   void EmitWinCFIStartProc(const MCSymbol *Symbol) override;
   void EmitWinCFIEndProc() override;
   void EmitWinCFIStartChained() override;

 }
 
 void MCAsmStreamer::EmitCFIDefCfaOffset(int64_t Offset) {
   MCStreamer::EmitCFIDefCfaOffset(Offset);
   OS << "\t.cfi_def_cfa_offset " << Offset;
   EmitEOL();
 }
 
+void MCAsmStreamer::EmitCFIEscape(StringRef Values) {
+  MCStreamer::EmitCFIEscape(Values);
+  OS << "\t.cfi_escape ";
+  if (Values.size()) {
+    size_t e = Values.size() - 1;
+    for (size_t i = 0; i < e; ++i)
+      OS << format("0x%02x", uint8_t(Values[i])) << ", ";
+    OS << format("0x%02x", uint8_t(Values[e]));
+  }
+
+  EmitEOL();
+}
+
 void MCAsmStreamer::EmitCFIDefCfaRegister(int64_t Register) {
   MCStreamer::EmitCFIDefCfaRegister(Register);
   OS << "\t.cfi_def_cfa_register ";
   EmitRegisterName(Register);
   EmitEOL();
 }
 
 void MCAsmStreamer::EmitCFIOffset(int64_t Register, int64_t Offset) {
   this->MCStreamer::EmitCFIOffset(Register, Offset);
   OS << "\t.cfi_offset ";
   EmitRegisterName(Register);
   OS << ", " << Offset;
   EmitEOL();
 }
 

lib/Target/X86/X86FrameLowering.cpp

 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/WinEHFuncInfo.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Function.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/Target/TargetOptions.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/LEB128.h"
 #include <cstdlib>
 
 using namespace llvm;
 
 X86FrameLowering::X86FrameLowering(const X86Subtarget &STI,
                                    unsigned StackAlignOverride)
     : TargetFrameLowering(StackGrowsDown, StackAlignOverride,
                           STI.is64Bit() ? -8 : -4),
eliminateCallFramePseudoInstr(MachineFunction &MF, MachineBasicBlock &MBB,
   bool reserveCallFrame = hasReservedCallFrame(MF);
   unsigned Opcode = I->getOpcode();
   bool isDestroy = Opcode == TII.getCallFrameDestroyOpcode();
   DebugLoc DL = I->getDebugLoc();
   uint64_t Amount = !reserveCallFrame ? I->getOperand(0).getImm() : 0;
   uint64_t InternalAmt = (isDestroy || Amount) ? I->getOperand(1).getImm() : 0;
   I = MBB.erase(I);
 
+  bool NeedsDwarfCFI =
+      !MF.getTarget().getMCAsmInfo()->usesWindowsCFI() && 
+      (MF.getMMI().hasDebugInfo() || MF.getFunction()->needsUnwindTableEntry());
+
   if (!reserveCallFrame) {
     // If the stack pointer can be changed after prologue, turn the
     // adjcallstackup instruction into a 'sub ESP, <amt>' and the
     // adjcallstackdown instruction into 'add ESP, <amt>'
-    if (Amount == 0)
-      return;
 
     // We need to keep the stack aligned properly.  To do this, we round the
     // amount of space needed for the outgoing arguments up to the next
     // alignment boundary.
     unsigned StackAlign = getStackAlignment();
     Amount = RoundUpToAlignment(Amount, StackAlign);
 
+    // If we adjust the SP before calls, we may need to indicate this to
+    // the unwinder, using GNU_ARGS_SIZE. Note that this may be necessary
+    // even when Amount == 0, because the preceding function may have
+    // set a non-0 GNU_ARGS_SIZE.
+    // TODO: We don't need to reset this between subsequent functions,
+    // if it didn't change.
+    if (NeedsDwarfCFI && !isDestroy && 
+        MF.getInfo<X86MachineFunctionInfo>()->getHasPushSequences()) {
+      const uint8_t GNU_ARGS_SIZE = 0x2e;
+      uint8_t Buffer[9] = { GNU_ARGS_SIZE };
+      unsigned Len = encodeULEB128(Amount, Buffer + 1) + 1;
+      BuildCFI(MBB, I, DL, MCCFIInstruction::createEscape(
+                               nullptr, StringRef((const char *)&Buffer[0], 
+                                                  Len)));
+    }
+
+    if (Amount == 0)
+      return;
+
     // Factor out the amount that gets handled inside the sequence
     // (Pushes of argument for frame setup, callee pops for frame destroy)
     Amount -= InternalAmt;
 
     if (Amount) {
       // Add Amount to SP to destroy a frame, and subtract to setup.
       int Offset = isDestroy ? Amount : -Amount;
 

test/CodeGen/X86/push-cfi.ll

+; RUN: llc < %s -mtriple=i686-pc-linux | FileCheck %s
+
+declare i32 @__gxx_personality_v0(...)
+declare void @good(i32 %a, i32 %b, i32 %c, i32 %d)
+declare void @large(i32 %a, i32 %b, i32 %c, i32 %d, i32 %e, i32 %f)
+declare void @empty()
+
+; We use an invoke, and expect a .cfi_escape GNU_ARGS_SIZE with size 16
+; before the invocation
+; CHECK-LABEL: test1:
+; CHECK: .cfi_escape 0x2e, 0x10
+; CHECK-NEXT: pushl   $4
+; CHECK-NEXT: pushl   $3
+; CHECK-NEXT: pushl   $2
+; CHECK-NEXT: pushl   $1
+; CHECK-NEXT: call
+; CHECK-NEXT: addl $16, %esp
+define void @test1() optsize personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+entry:
+  invoke void @good(i32 1, i32 2, i32 3, i32 4)
+          to label %continue unwind label %cleanup
+continue:
+  ret void
+cleanup:  
+  landingpad { i8*, i32 }
+     cleanup
+  ret void
+}
+
+; Same for a call
+; CHECK-LABEL: test2:
+; CHECK: .cfi_escape 0x2e, 0x10
+; CHECK-NEXT: pushl   $4
+; CHECK-NEXT: pushl   $3
+; CHECK-NEXT: pushl   $2
+; CHECK-NEXT: pushl   $1
+; CHECK-NEXT: call
+; CHECK-NEXT: addl $16, %esp
+define void @test2() optsize personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+entry:
+  call void @good(i32 1, i32 2, i32 3, i32 4)
+  ret void
+}
+
+; But not if the function is nounwind
+; CHECK-LABEL: test3:
+; CHECK-NOT: .cfi_escape
+; CHECK: pushl   $4
+; CHECK-NEXT: pushl   $3
+; CHECK-NEXT: pushl   $2
+; CHECK-NEXT: pushl   $1
+; CHECK-NEXT: call
+; CHECK-NEXT: addl $16, %esp
+define void @test3() nounwind optsize personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+entry:
+  call void @good(i32 1, i32 2, i32 3, i32 4)
+  ret void
+}
+
+; If we did not end up using any pushes, no need for GNU_ARGS_SIZE anywhere
+; CHECK-LABEL: test4:
+; CHECK-NOT: .cfi_escape
+; CHECK-NOT: pushl
+; CHECK: retl
+define void @test4() optsize personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+entry:
+  call void @empty()
+  ret void
+}
+
+; If we did use pushes, we need to resest GNU_ARGS_SIZE before a call
+; without parameters
+; CHECK-LABEL: test5:
+; CHECK: .cfi_escape 0x2e, 0x10
+; CHECK-NEXT: pushl   $4
+; CHECK-NEXT: pushl   $3
+; CHECK-NEXT: pushl   $2
+; CHECK-NEXT: pushl   $1
+; CHECK-NEXT: call
+; CHECK-NEXT: addl $16, %esp
+; CHECK: .cfi_escape 0x2e, 0x20
+; CHECK-NEXT: subl    $8, %esp
+; CHECK-NEXT: pushl   $11
+; CHECK-NEXT: pushl   $10
+; CHECK-NEXT: pushl   $9
+; CHECK-NEXT: pushl   $8
+; CHECK-NEXT: pushl   $7
+; CHECK-NEXT: pushl   $6
+; CHECK-NEXT: calll   large
+; CHECK-NEXT: addl $32, %esp
+define void @test5() optsize personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+entry:
+  call void @good(i32 1, i32 2, i32 3, i32 4)
+  call void @large(i32 6, i32 7, i32 8, i32 9, i32 10, i32 11)
+  ret void
+}
+
+; If we did use pushes, we need to resest GNU_ARGS_SIZE before a call
+; without parameters
+; CHECK-LABEL: test6:
+; CHECK: .cfi_escape 0x2e, 0x10
+; CHECK-NEXT: pushl   $4
+; CHECK-NEXT: pushl   $3
+; CHECK-NEXT: pushl   $2
+; CHECK-NEXT: pushl   $1
+; CHECK-NEXT: call
+; CHECK-NEXT: addl $16, %esp
+; CHECK: .cfi_escape 0x2e, 0x00
+; CHECK-NEXT: call
+define void @test6() optsize personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+entry:
+  call void @good(i32 1, i32 2, i32 3, i32 4)
+  call void @empty()
+  ret void
+}
+; This is actuall enfficient - we don't need to repeat the .cfi_escape twice.
+; CHECK-LABEL: test7:
+; CHECK: .cfi_escape 0x2e, 0x10
+; CHECK: call
+; CHECK: .cfi_escape 0x2e, 0x10
+; CHECK: call
+define void @test7() optsize personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+entry:
+  call void @good(i32 1, i32 2, i32 3, i32 4)
+  call void @good(i32 5, i32 6, i32 7, i32 8)
+  ret void
+}
 No newline at end of file