[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

2072

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

2072

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

1025

!Values.empty()

lib/Target/X86/X86FrameLowering.cpp

2073

Why not use DW_CFA_GNU_args_size ?

test/CodeGen/X86/push-cfi.ll

72

"resest"

99

resest

117

actually efficient

[mkuper]

Thanks, David!

lib/MC/MCAsmStreamer.cpp

1025

Right, thanks!

lib/Target/X86/X86FrameLowering.cpp

2073

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

1038

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.)

1041

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

2052

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

1038

For consistency with the rest of the interface, mostly.

1041

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

2052

Makes sense. I'll refine the condition.

[mkuper]

lib/Target/X86/X86FrameLowering.cpp

2052

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

2056

That sounds right to me.

[mkuper]

ping

[rnk]

lgtm

Thanks for the ping and the fix!

lib/Target/X86/X86FrameLowering.cpp

2072

Hm, got it.

include/llvm/MC/MCDwarf.h

enum OpType {
     OpDefCfaOffset,
     OpDefCfa,
     OpRelOffset,
     OpAdjustCfaOffset,
     OpEscape,
     OpRestore,
     OpUndefined,
     OpRegister,
-    OpWindowSave
+    OpWindowSave,
+    OpGnuArgsSize
   };
 
 private:
   OpType Operation;
   MCSymbol *Label;
   unsigned Register;
   union {
     int Offset;
public:
   }
 
   /// \brief .cfi_escape Allows the user to add arbitrary bytes to the unwind
   /// info.
   static MCCFIInstruction createEscape(MCSymbol *L, StringRef Vals) {
     return MCCFIInstruction(OpEscape, L, 0, 0, Vals);
   }
 
+  /// \brief A special wrapper for .cfi_escape that indicates GNU_ARGS_SIZE
+  static MCCFIInstruction createGnuArgsSize(MCSymbol *L, int Size) {
+    return MCCFIInstruction(OpGnuArgsSize, L, 0, Size, "");
+  }
+
   OpType getOperation() const { return Operation; }
   MCSymbol *getLabel() const { return Label; }
 
   unsigned getRegister() const {
     assert(Operation == OpDefCfa || Operation == OpOffset ||
            Operation == OpRestore || Operation == OpUndefined ||
            Operation == OpSameValue || Operation == OpDefCfaRegister ||
            Operation == OpRelOffset || Operation == OpRegister);
     return Register;
   }
 
   unsigned getRegister2() const {
     assert(Operation == OpRegister);
     return Register2;
   }
 
   int getOffset() const {
     assert(Operation == OpDefCfa || Operation == OpOffset ||
            Operation == OpRelOffset || Operation == OpDefCfaOffset ||
-           Operation == OpAdjustCfaOffset);
+           Operation == OpAdjustCfaOffset || Operation == OpGnuArgsSize);
     return Offset;
   }
 
   StringRef getValues() const {
     assert(Operation == OpEscape);
     return StringRef(&Values[0], Values.size());
   }
 };

include/llvm/MC/MCStreamer.h

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

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::OpGnuArgsSize:
+    OutStreamer->EmitCFIGnuArgsSize(Inst.getOffset());
+    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

 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSectionCOFF.h"
 #include "llvm/MC/MCSectionMachO.h"
 #include "llvm/MC/MCSymbolELF.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormattedStream.h"
+#include "llvm/Support/LEB128.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/Path.h"
 #include <cctype>
 using namespace llvm;
 
 namespace {
 
 class MCAsmStreamer final : public MCStreamer {
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 EmitCFIGnuArgsSize(int64_t Size) 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();
 }
 
+static void PrintCFIEscape(llvm::formatted_raw_ostream &OS, StringRef Values) {
+  OS << "\t.cfi_escape ";
+  if (!Values.empty()) {
+    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]));
+  }
+}
+
+void MCAsmStreamer::EmitCFIEscape(StringRef Values) {
+  MCStreamer::EmitCFIEscape(Values);
+  PrintCFIEscape(OS, Values);
+  EmitEOL();
+}
+
+void MCAsmStreamer::EmitCFIGnuArgsSize(int64_t Size) {
+  MCStreamer::EmitCFIGnuArgsSize(Size);
+  
+  uint8_t Buffer[9] = { dwarf::DW_CFA_GNU_args_size };
+  unsigned Len = encodeULEB128(Size, Buffer + 1) + 1;
+  
+  PrintCFIEscape(OS, StringRef((const char *)&Buffer[0], Len));
+  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) {

lib/MC/MCDwarf.cpp

void FrameEmitterImpl::EmitCFIInstruction(MCObjectStreamer &Streamer,
   }
   case MCCFIInstruction::OpRestore: {
     unsigned Reg = Instr.getRegister();
     if (!IsEH)
       Reg = MRI->getDwarfRegNum(MRI->getLLVMRegNum(Reg, true), false);
     Streamer.EmitIntValue(dwarf::DW_CFA_restore | Reg, 1);
     return;
   }
+  case MCCFIInstruction::OpGnuArgsSize: {
+    Streamer.EmitIntValue(dwarf::DW_CFA_GNU_args_size, 1);
+    Streamer.EmitULEB128IntValue(Instr.getOffset());
+    return;
+  }
   case MCCFIInstruction::OpEscape:
     Streamer.EmitBytes(Instr.getValues());
     return;
   }
   llvm_unreachable("Unhandled case in switch");
 }
 
 /// Emit frame instructions to describe the layout of the frame.

lib/MC/MCStreamer.cpp

 
 void MCStreamer::EmitCFIEscape(StringRef Values) {
   MCSymbol *Label = EmitCFICommon();
   MCCFIInstruction Instruction = MCCFIInstruction::createEscape(Label, Values);
   MCDwarfFrameInfo *CurFrame = getCurrentDwarfFrameInfo();
   CurFrame->Instructions.push_back(Instruction);
 }
 
+void MCStreamer::EmitCFIGnuArgsSize(int64_t Size) {
+  MCSymbol *Label = EmitCFICommon();
+  MCCFIInstruction Instruction = 
+    MCCFIInstruction::createGnuArgsSize(Label, Size);
+  MCDwarfFrameInfo *CurFrame = getCurrentDwarfFrameInfo();
+  CurFrame->Instructions.push_back(Instruction);
+}
+
 void MCStreamer::EmitCFISignalFrame() {
   EnsureValidDwarfFrame();
   MCDwarfFrameInfo *CurFrame = getCurrentDwarfFrameInfo();
   CurFrame->IsSignalFrame = true;
 }
 
 void MCStreamer::EmitCFIUndefined(int64_t Register) {
   MCSymbol *Label = EmitCFICommon();

lib/Target/X86/X86FrameLowering.cpp

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())
+      BuildCFI(MBB, I, DL,
+               MCCFIInstruction::createGnuArgsSize(nullptr, Amount));
+
+    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-obj.ll

+; RUN: llc < %s -mtriple=i686-pc-linux -filetype=obj | llvm-readobj -s -sr -sd | FileCheck %s
+
+; CHECK:         Index: 7
+; CHECK-NEXT:    Name: .eh_frame (41)
+; CHECK-NEXT:    Type: SHT_PROGBITS (0x1)
+; CHECK-NEXT:    Flags [ (0x2)
+; CHECK-NEXT:      SHF_ALLOC (0x2)
+; CHECK-NEXT:    ]
+; CHECK-NEXT:    Address: 0x0
+; CHECK-NEXT:    Offset: 0x4C
+; CHECK-NEXT:    Size: 48
+; CHECK-NEXT:    Link: 0
+; CHECK-NEXT:    Info: 0
+; CHECK-NEXT:    AddressAlignment: 4
+; CHECK-NEXT:    EntrySize: 0
+; CHECK-NEXT:    Relocations [
+; CHECK-NEXT:    ]
+; CHECK-NEXT:    SectionData (
+; CHECK-NEXT:      0000: 14000000 00000000 017A5200 017C0801  |.........zR..|..|
+; CHECK-NEXT:      0010: 1B0C0404 88010000 14000000 1C000000  |................|
+; CHECK-NEXT:      0020: 00000000 17000000 00430E10 2E100000  |.........C......|
+; CHECK-NEXT:    )
+
+declare i32 @__gxx_personality_v0(...)
+declare void @good(i32 %a, i32 %b, i32 %c, i32 %d)
+
+define void @test() optsize personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+entry:
+  call void @good(i32 1, i32 2, i32 3, i32 4)
+  ret void
+}

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 reset 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 reset 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 actually inefficient - 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
+}