[X86] Fix more -Os + EH issues

[mkuper]

Summary

This does two things:

1. Disables push generation on Darwin whenever EH is used and FP is not present.
2. Generates .cfa_adjust_cfa_offset directives to make the CFA offset correct at each call site.

Generating more precise offsets adjustments for debug info will be a separate patch.

[kbsmith1]

Please see inline comments.

lib/Target/X86/X86FrameLowering.cpp

2108

I think this code needs to go after line 2128 in order to create the CFI adjust after the stack adjustment instruction has happened rather than before.

test/CodeGen/X86/push-cfi-obj.ll

27

I don't understand what the specific differences in these values in Section Data coma about from. Do you know?

test/CodeGen/X86/push-cfi.ll

14

I don't think this is really the correct spot for the cfi_adjust. I think it should occur after all the pushes have happenned, so right before the call instruction. Once you get to having a different cfi_adjust per push, each of these should follow immediately after the instruction, not before. Now I understand that you are not yet getting to the point of emitting a cfi_adjust per instruction, but it seems like even if you are accumulating them so that they are only correct at the call, then they should be after all the pushes.

[mkuper]

Thanks, Kevin!

lib/Target/X86/X86FrameLowering.cpp

2108

Since to handle EH we only care about it being correct at the call site, I don't think it matters whether it's before or after the adjustment.

I could do it at line 2128, but that would be slightly less cumbersome, since then I'd need to keep track of both the original Amount and Amount - InternalAmt.

test/CodeGen/X86/push-cfi-obj.ll

27

More or less. :-)

I mean, the part that *didn't* change encodes the GNU_ARGS_SIZE, and this is what this test is supposed to check (it was introduced by a previous commit that made some changes to the code that outputs the dwarf binary encoding).
The newly added part encodes the def_cfa_offset. I didn't actually verify it's correct because I didn't change the encoding code for that. I'll parse it manually and see it really is correct.

test/CodeGen/X86/push-cfi.ll

14

I agree with you that it's not the correct spot, in theory - but in terms of being correct at the call site, it doesn't matter if it's before or after the pushes.

The problem is that the way things work right now, eliminateCallFramePseudoInstr() doesn't actually get a pointer to the call, it gets the ADJDOWN/ADJUP pseudos. In theory, there doesn't even *have* to be a call between them.
So, the easiest way is to just put the CFA adjust immediately after those pseudos. I could have the eliminate() code actively look for a call, but I'm not sure that's the best idea. What I would prefer for the debug-info version would be to put the CFA adjust for the sub where the current adjust is (well, after the sub - in line 2128), and the adjust for each push after the push. So that case wouldn't have to search for a call either.

[rnk]

lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp

231–233

Hah, I guess we have MC support for .cfi_adjust_offset, but never used if from the compiler. :)

lib/Target/X86/X86CallFrameOptimization.cpp

140

Isn't STI.getFrameLowering() just TFL?

lib/Target/X86/X86FrameLowering.cpp

2097

Can we just assert up front that we don't need to produce UNWIND_INFO .xdata for Windows? This call frame optimization stuff only fires for 32-bit x86, which doesn't use UNWIND_INFO.

2110–2111

I think we should address this now. We know exactly where we're inserting the pushes in X86CallFrameOptimization::adjustCallSequence, so all we have to do is:

• Insert .cfi_adjust_cfa_offset 4 after each push
• Insert .cfi_adjust_cfa_offset InternalAmt for frame destruction here
• Insert .cfi_adjust_cfa_offset Amount after BuildStackAdjustment below

[mkuper]

Thanks, Reid!

lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp

231–233

Yup.
It's used by the assembler, though. :-)

lib/Target/X86/X86CallFrameOptimization.cpp

140

Right, thanks!

lib/Target/X86/X86FrameLowering.cpp

2097

Sure. Should I just assert on usesWindowsCFI()?

2110–2111

The question is whether we want to always have the precise adjusts or have two separate code paths for the precise version and the call-site-only version. This depends on whether we care about the eh_frame size for -Os, or not.

If we want it to always be precise, then what you're suggesting is the right way to go. If we don't, then there should be two separate code paths, the one here, and the one you suggest, and I wanted them to be two separate patches.

[mkuper]

lib/Target/X86/X86FrameLowering.cpp

2097

On second thought, no, we can't. It's possible not to have a reserved call frame even without pushes (currently happens e.g. if there are variable-length arrays on the stack). In this case we'll also have an FP, so outgoing arguments will use ebp-based movs.

[mkuper]

The patch now has both the precise and the precise-at-call-site code-paths.

[mkuper]

lib/Target/X86/X86CallFrameOptimization.cpp

503

Argh, this should have been a cast<>, not a static_cast<>.
Also, I can avoid this completely and copy the code from the BuildCFI() helper, but I think it's better this way.

[friss]

test/CodeGen/X86/push-cfi-obj.ll

27

I'm sorry I didn't look at the patch in more details, but does the above mean that you emit GNU_args_size and def_fa_offset in the same function? If that's the case, it seems wrong: if the offset is reflected by def_cfa_offsets, you don't need the GNU_args_size anymore.

[rnk]

lib/Target/X86/X86CallFrameOptimization.cpp

503

You could also change TFL to hold an X86FrameLowering. If you ask X86Subtarget for the framelowering, it actually uses covariant return types to return you an X86FrameLowering instead of a TargetFrameLowering.

test/CodeGen/X86/push-cfi-debug.ll

17–21 ↗

(On Diff #37742)

Can you add a test for a callee-cleanup convention like stdcall? I think with the code as written we'll get something like this on Linux, where the stack is 16-byte aligned:

subl $4, %esp
.cfi_adjust_cfa_offset 4
pushl $3
.cfi_adjust_cfa_offset 4
pushl $2
.cfi_adjust_cfa_offset 4
pushl $1
.cfi_adjust_cfa_offset 4
calll _bar
addl $4, %esp
.cfi_adjust_cfa_offset -16

Which isn't *strictly* speaking correct, it should be:

calll bar
.cfi_adjust_cfa_offset -12
addl $4, %esp
.cfi_adjust_cfa_offset -4

We don't need to fix the issue in this change, I just like having test cases with FIXMEs.

[mkuper]

Thanks Reid, Frederic.

This addresses the comments from the last revision.
In particular, gnu_args_size is now only generated for fp-based CFA. Frederic, does that sounds right to you?

[rnk]

In D13767#272094, @mkuper wrote:

In particular, gnu_args_size is now only generated for fp-based CFA. Frederic, does that sounds right to you?

Hm, I think there might be an issue here. The cfa_adjust directives don't instruct the unwinder to adjust ESP before transferring to the landingpad, right? They just help indicate where the return address lives in memory.

The test case for this situation is something like:

int main() {
  // force stack realignment, FP usage, and use of SP to address local variables
  int __attribute__((align(32))) x = 42;
  try {
    throw 1;
  } catch (int) {
    return x;
  }
}

If ESP is wrong in the landingpad, we'll return the wrong value of x.

[kbsmith1]

I would still have preferred that in the "only correct for call-site" case that the cfi_adjust occurred after the pushes rather than before. But I don't feel strongly about that, and so, am good with the changes now.

So, LGTM from me.

[friss]

In D13767#272344, @rnk wrote:

In D13767#272094, @mkuper wrote:

In particular, gnu_args_size is now only generated for fp-based CFA. Frederic, does that sounds right to you?

Hm, I think there might be an issue here. The cfa_adjust directives don't instruct the unwinder to adjust ESP before transferring to the landingpad, right? They just help indicate where the return address lives in memory.

I think you are right, sorry for the confusion. What's the exact semantic of GNU_args_size? Is it only to be used when branching to a landing pad and ignored when just traversing the function?

[DavidKreitzer]

In D13767#272789, @friss wrote:

In D13767#272344, @rnk wrote:

In D13767#272094, @mkuper wrote:

In particular, gnu_args_size is now only generated for fp-based CFA. Frederic, does that sounds right to you?

Hm, I think there might be an issue here. The cfa_adjust directives don't instruct the unwinder to adjust ESP before transferring to the landingpad, right? They just help indicate where the return address lives in memory.

I think you are right, sorry for the confusion. What's the exact semantic of GNU_args_size? Is it only to be used when branching to a landing pad and ignored when just traversing the function?

Yes, exactly. GNU_args_size is the delta between the stack pointer value immediately before the invoke that triggered the exception and the stack pointer value on entry to the landing pad. It is ignored for unwinding purposes.

The value that the unwinder substitutes for %esp/%rsp in CFA expressions & callee-save register location expressions is the value of %esp/%rsp immediately prior to the call/invoke.

lib/Target/X86/X86FrameLowering.cpp

2100

Why are you suppressing the GNU_ARGS_SIZE directives for !hasFP? I think they are still needed. If you agree, some corresponding changes will be needed in the tests.

2124

It would be nice to encapsulate the hasDebugInfo checks into a separate method, e.g. usePreciseUnwindInfo. That serves a useful documentation purpose and paves the way for supporting asynchronous EH, which also requires precise unwind info.

[mkuper]

Thanks, everyone. Looks like I got confused about the semantics, will upload a new patch.
Sorry again for the amount of noise this is causing.

lib/Target/X86/X86FrameLowering.cpp

2100

Yes, this is a mistake, I misinterpreted Frederic's comment. Will make the appropriate changes, thanks.

2124

Right, will do.

[mkuper]

Do *not* suppress gnu_args_size regardless of FP.
Also added the encapsulation Dave suggested.

[mkuper]

ping

[rnk]

lgtm

lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp

 
 void AsmPrinter::emitCFIInstruction(const MCCFIInstruction &Inst) const {
   switch (Inst.getOperation()) {
   default:
     llvm_unreachable("Unexpected instruction");
   case MCCFIInstruction::OpDefCfaOffset:
     OutStreamer->EmitCFIDefCfaOffset(Inst.getOffset());
     break;
+  case MCCFIInstruction::OpAdjustCfaOffset:
+    OutStreamer->EmitCFIAdjustCfaOffset(Inst.getOffset());
+    break;
   case MCCFIInstruction::OpDefCfa:
     OutStreamer->EmitCFIDefCfa(Inst.getRegister(), Inst.getOffset());
     break;
   case MCCFIInstruction::OpDefCfaRegister:
     OutStreamer->EmitCFIDefCfaRegister(Inst.getRegister());
     break;
   case MCCFIInstruction::OpOffset:
     OutStreamer->EmitCFIOffset(Inst.getRegister(), Inst.getOffset());

lib/Target/X86/X86CallFrameOptimization.cpp

bool X86CallFrameOptimization::isLegal(MachineFunction &MF) {
   // are passed on the stack.
   // No point in running this in 64-bit mode, since some arguments are
   // passed in-register in all common calling conventions, so the pattern
   // we're looking for will never match.
   const X86Subtarget &STI = MF.getSubtarget<X86Subtarget>();
   if (STI.is64Bit())
     return false;
 
-  // We can't encode multiple DW_CFA_GNU_args_size in the compact
-  // unwind encoding that Darwin uses.
-  if (STI.isTargetDarwin() && !MF.getMMI().getLandingPads().empty())
+  // We can't encode multiple DW_CFA_GNU_args_size or DW_CFA_def_cfa_offset
+  // in the compact unwind encoding that Darwin uses. So, bail if there
+  // is a danger of that being generated.
+  if (STI.isTargetDarwin() && 
+     (!MF.getMMI().getLandingPads().empty() || 
+       (MF.getFunction()->needsUnwindTableEntry() && 
+        !STI.getFrameLowering()->hasFP(MF))))
     return false;
 
   // You would expect straight-line code between call-frame setup and
   // call-frame destroy. You would be wrong. There are circumstances (e.g.
   // CMOV_GR8 expansion of a select that feeds a function call!) where we can
   // end up with the setup and the destroy in different basic blocks.
   // This is bad, and breaks SP adjustment.
   // So, check that all of the frames in the function are closed inside
if (MOV->getOpcode() == X86::MOV32mi) {
         BuildMI(MBB, Context.Call, DL, TII->get(X86::PUSH32r))
             .addReg(Reg)
             .getInstr();
       }
     }
 
     MBB.erase(MOV);
   }
 
   // The stack-pointer copy is no longer used in the call sequences.
   // There should not be any other users, but we can't commit to that, so:
   if (MRI->use_empty(Context.SPCopy->getOperand(0).getReg()))
     Context.SPCopy->eraseFromParent();
 
   // Once we've done this, we need to make sure PEI doesn't assume a reserved
   // frame.
   X86MachineFunctionInfo *FuncInfo = MF.getInfo<X86MachineFunctionInfo>();

lib/Target/X86/X86FrameLowering.cpp

if (!reserveCallFrame) {
     // If we have any exception handlers in this function, and 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.
     bool HasDwarfEHHandlers =
       !MF.getTarget().getMCAsmInfo()->usesWindowsCFI() &&
       !MF.getMMI().getLandingPads().empty();
 
     if (HasDwarfEHHandlers && !isDestroy && 
         MF.getInfo<X86MachineFunctionInfo>()->getHasPushSequences())
       BuildCFI(MBB, I, DL,
                MCCFIInstruction::createGnuArgsSize(nullptr, Amount));
 
     if (Amount == 0)
       return;
 
+    // Similarly, if we don't have FP, but need to generate unwind information,
+    // we need to set the correct CFA offset at every call site.
+    // TODO: Debug info really needs precise CFA offsets after each stack
+    // adjustment, it's not enough for it to be correct at callsites. 
+    if (MF.getFunction()->needsUnwindTableEntry() && !hasFP(MF))
+      BuildCFI(MBB, I, DL, 
+               MCCFIInstruction::createAdjustCfaOffset(nullptr, 
+                 isDestroy ? -Amount : Amount));
+
     // 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;
 
       if (!(MF.getFunction()->optForMinSize() && 
             adjustStackWithPops(MBB, I, DL, Offset)))
         BuildStackAdjustment(MBB, I, DL, Offset, /*InEpilogue=*/false);
     }
 
     return;
   }
 

test/CodeGen/X86/fold-push.ll

 ; RUN: llc < %s -mtriple=i686-windows | FileCheck %s -check-prefix=CHECK -check-prefix=NORMAL
 ; RUN: llc < %s -mtriple=i686-windows -mattr=call-reg-indirect | FileCheck %s -check-prefix=CHECK -check-prefix=SLM
 
 declare void @foo(i32 %r)
 
-define void @test(i32 %a, i32 %b) optsize {
+define void @test(i32 %a, i32 %b) optsize nounwind {
 ; CHECK-LABEL: test:
 ; CHECK: movl [[EAX:%e..]], (%esp)
 ; CHECK-NEXT: pushl [[EAX]]
 ; CHECK-NEXT: calll
 ; CHECK-NEXT: addl $4, %esp
 ; CHECK: nop
 ; NORMAL: pushl (%esp)
 ; SLM: movl (%esp), [[RELOAD:%e..]]
 ; SLM-NEXT: pushl [[RELOAD]]
 ; CHECK: calll
 ; CHECK-NEXT: addl $4, %esp
   %c = add i32 %a, %b
   call void @foo(i32 %c)
   call void asm sideeffect "nop", "~{ax},~{bx},~{cx},~{dx},~{bp},~{si},~{di}"()
   call void @foo(i32 %c)
   ret void
 }
 
-define void @test_min(i32 %a, i32 %b) minsize {
+define void @test_min(i32 %a, i32 %b) minsize nounwind {
 ; CHECK-LABEL: test_min:
 ; CHECK: movl [[EAX:%e..]], (%esp)
 ; CHECK-NEXT: pushl [[EAX]]
 ; CHECK-NEXT: calll
 ; CHECK-NEXT: popl
 ; CHECK: nop
 ; CHECK: pushl (%esp)
 ; CHECK: calll
 ; CHECK-NEXT: popl
   %c = add i32 %a, %b
   call void @foo(i32 %c)
   call void asm sideeffect "nop", "~{ax},~{bx},~{cx},~{dx},~{bp},~{si},~{di}"()
   call void @foo(i32 %c)
   ret void
 }

test/CodeGen/X86/pop-stack-cleanup.ll

 ; RUN: llc < %s -mtriple=i686-windows | FileCheck %s -check-prefix=CHECK 
 ; RUN: llc < %s -mtriple=x86_64-linux | FileCheck %s -check-prefix=LINUX64
 
 declare void @param1(i32 %a)
 declare i32 @param2_ret(i32 %a, i32 %b)
 declare i64 @param2_ret64(i32 %a, i32 %b)
 declare void @param2(i32 %a, i32 %b)
 declare void @param3(i32 %a, i32 %b, i32 %c)
 declare void @param8(i64, i64, i64, i64, i64, i64, i64, i64)
 
 
-define void @test() minsize {
+define void @test() minsize nounwind {
 ; CHECK-LABEL: test:
 ; CHECK: calll _param1
 ; CHECK-NEXT: popl %eax
 ; CHECK: calll _param2
 ; CHECK-NEXT: popl %eax
 ; CHECK-NEXT: popl %ecx
 ; CHECK: calll _param2_ret
 ; CHECK-NEXT: popl %ecx

 ; CHECK-NEXT: movl %ebp, %esp
   %v = alloca i32, i32 %k
   call void @param1(i32 1)
   call void @param2(i32 1, i32 2)
   call void @param3(i32 1, i32 2, i32 3)
   ret void
 }
 
-define void @spill(i32 inreg %a, i32 inreg %b, i32 inreg %c) minsize {
+define void @spill(i32 inreg %a, i32 inreg %b, i32 inreg %c) minsize nounwind {
 ; CHECK-LABEL: spill:
 ; CHECK-DAG: movl %ecx,
 ; CHECK-DAG: movl %edx,
 ; CHECK: calll _param2_ret
 ; CHECK-NEXT: popl %ecx
 ; CHECK-NEXT: popl %edx
 ; CHECK-DAG: movl {{.*}}, %ecx
 ; CHECK-DAG: movl {{.*}}, %edx
 ; CHECK: calll _spill
   %i = call i32 @param2_ret(i32 1, i32 2)
   call void @spill(i32 %a, i32 %b, i32 %c)
   ret void
 }
 
-define void @test_linux64(i32 %size) minsize {
+define void @test_linux64(i32 %size) minsize nounwind {
 ; LINUX64-LABEL: test_linux64:
 ; LINUX64: pushq %rbp
 ; LINUX64: callq param8
 ; LINUX64-NEXT: popq %rax
 ; LINUX64-NEXT: popq %rcx
 
   %a = alloca i64, i32 %size, align 8
   call void @param8(i64 1, i64 2, i64 3, i64 4, i64 5, i64 6, i64 7, i64 8)
   ret void
 }

test/CodeGen/X86/push-cfi-obj.ll

-; RUN: llc < %s -mtriple=i686-pc-linux -filetype=obj | llvm-readobj -s -sr -sd | FileCheck %s
+; RUN: llc < %s -mtriple=i686-pc-linux -filetype=obj | llvm-readobj -s -sr -sd | FileCheck %s -check-prefix=LINUX
 ; RUN: llc < %s -mtriple=i686-darwin-macosx10.7 -filetype=obj | llvm-readobj -sections | FileCheck -check-prefix=DARWIN %s
 
 ; On darwin, check that we manage to generate the compact unwind section
 ; DARWIN: Name: __compact_unwind
 ; DARWIN: Segment: __LD
 
-; CHECK:         Index: 8
-; 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: 0x64
-; CHECK-NEXT:    Size: 60
-; 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: 1C000000 00000000 017A504C 5200017C  |.........zPLR..||
-; CHECK-NEXT:      0010: 08070000 00000000 1B0C0404 88010000  |................|
-; CHECK-NEXT:      0020: 18000000 24000000 00000000 19000000  |....$...........|
-; CHECK-NEXT:      0030: 04000000 00430E10 2E100000           |.....C......|
-; CHECK-NEXT:    )
+; LINUX:         Index: 8
+; LINUX-NEXT:    Name: .eh_frame (41)
+; LINUX-NEXT:    Type: SHT_PROGBITS (0x1)
+; LINUX-NEXT:    Flags [ (0x2)
+; LINUX-NEXT:      SHF_ALLOC (0x2)
+; LINUX-NEXT:    ]
+; LINUX-NEXT:    Address: 0x0
+; LINUX-NEXT:    Offset: 0x64
+; LINUX-NEXT:    Size: 64
+; LINUX-NEXT:    Link: 0
+; LINUX-NEXT:    Info: 0
+; LINUX-NEXT:    AddressAlignment: 4
+; LINUX-NEXT:    EntrySize: 0
+; LINUX-NEXT:    Relocations [
+; LINUX-NEXT:    ]
+; LINUX-NEXT:    SectionData (
+; LINUX-NEXT:      0000: 1C000000 00000000 017A504C 5200017C  |.........zPLR..||
+; LINUX-NEXT:      0010: 08070000 00000000 1B0C0404 88010000  |................|
+; LINUX-NEXT:      0020: 1C000000 24000000 00000000 19000000  |....$...........|
+; LINUX-NEXT:      0030: 04000000 00430E10 2E100020 4D0E1000  |.....C..... M...|
+; LINUX-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:
   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
 }

test/CodeGen/X86/push-cfi.ll

-; RUN: llc < %s -mtriple=i686-pc-linux | FileCheck %s
+; RUN: llc < %s -mtriple=i686-pc-linux | FileCheck %s -check-prefix=LINUX
+; RUN: llc < %s -mtriple=i686-apple-darwin | FileCheck %s -check-prefix=DARWIN
 
 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
+; before the invocation, as well as .cfi_adjust_cfa_offset before and after.
+; Darwin should not generate pushes.
 ; 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
+; LINUX: .cfi_escape 0x2e, 0x10
+; LINUX: .cfi_adjust_cfa_offset 16
+; LINUX-NEXT: pushl   $4
+; LINUX-NEXT: pushl   $3
+; LINUX-NEXT: pushl   $2
+; LINUX-NEXT: pushl   $1
+; LINUX-NEXT: call
+; LINUX: .cfi_adjust_cfa_offset -16
+; LINUX-NEXT: addl $16, %esp
+; DARWIN-NOT: .cfi_escape
+; DARWIN-NOT: pushl
 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
 }
 
 ; If the function has no handlers, we don't need to generate GNU_ARGS_SIZE,
-; even if it has an unwind table.
+; even if it has an unwind table. We still need cfi_adjust_cfa_offset, though.
+; Darwin should not generate pushes.
 ; CHECK-LABEL: test2:
-; 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
+; LINUX-NOT: .cfi_escape
+; LINUX: .cfi_adjust_cfa_offset 16
+; LINUX-NEXT: pushl   $4
+; LINUX-NEXT: pushl   $3
+; LINUX-NEXT: pushl   $2
+; LINUX-NEXT: pushl   $1
+; LINUX-NEXT: call
+; LINUX: .cfi_adjust_cfa_offset -16
+; LINUX-NEXT: addl $16, %esp
+; DARWIN-NOT: .cfi_escape
+; DARWIN-NOT: pushl
 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
 }
 
-; If we did not end up using any pushes, no need for GNU_ARGS_SIZE anywhere
+; If we did not end up using any pushes, no need for GNU_ARGS_SIZE or
+; cfi_adjust_cfa_offset.
 ; CHECK-LABEL: test3:
-; CHECK-NOT: .cfi_escape
-; CHECK-NOT: pushl
-; CHECK: retl
+; LINUX-NOT: .cfi_escape
+; LINUX-NOT: .cfi_adjust_cfa_offset
+; LINUX-NOT: pushl
+; LINUX: retl
 define void @test3() optsize personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
 entry:
   invoke void @empty()
           to label %continue unwind label %cleanup
 continue:
   ret void
 cleanup:  
   landingpad { i8*, i32 }
      cleanup
   ret void
 }
 
 ; Different sized stacks need different GNU_ARGS_SIZEs
 ; CHECK-LABEL: test4:
-; 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
+; LINUX: .cfi_escape 0x2e, 0x10
+; LINUX: .cfi_adjust_cfa_offset 16
+; LINUX-NEXT: pushl   $4
+; LINUX-NEXT: pushl   $3
+; LINUX-NEXT: pushl   $2
+; LINUX-NEXT: pushl   $1
+; LINUX-NEXT: call
+; LINUX: .cfi_adjust_cfa_offset -16
+; LINUX-NEXT: addl $16, %esp
+; LINUX: .cfi_escape 0x2e, 0x20
+; LINUX: .cfi_adjust_cfa_offset 32
+; LINUX-NEXT: subl    $8, %esp
+; LINUX-NEXT: pushl   $11
+; LINUX-NEXT: pushl   $10
+; LINUX-NEXT: pushl   $9
+; LINUX-NEXT: pushl   $8
+; LINUX-NEXT: pushl   $7
+; LINUX-NEXT: pushl   $6
+; LINUX-NEXT: calll   large
+; LINUX: .cfi_adjust_cfa_offset -32
+; LINUX-NEXT: addl $32, %esp
 define void @test4() optsize personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
 entry:
   invoke void @good(i32 1, i32 2, i32 3, i32 4)
           to label %continue1 unwind label %cleanup
 continue1:
   invoke void @large(i32 6, i32 7, i32 8, i32 9, i32 10, i32 11)
           to label %continue2 unwind label %cleanup
 continue2:
   ret void          
 cleanup:  
   landingpad { i8*, i32 }
      cleanup
   ret void
 }
 
 ; If we did use pushes, we need to reset GNU_ARGS_SIZE before a call
-; without parameters
+; without parameters, but don't need to adjust the cfa offset
 ; 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, 0x00
-; CHECK-NEXT: call
+; LINUX: .cfi_escape 0x2e, 0x10
+; LINUX: .cfi_adjust_cfa_offset 16
+; LINUX-NEXT: pushl   $4
+; LINUX-NEXT: pushl   $3
+; LINUX-NEXT: pushl   $2
+; LINUX-NEXT: pushl   $1
+; LINUX-NEXT: call
+; LINUX: .cfi_adjust_cfa_offset -16
+; LINUX-NEXT: addl $16, %esp
+; LINUX: .cfi_escape 0x2e, 0x00
+; LINUX-NOT: .cfi_adjust_cfa_offset
+; LINUX: call
 define void @test5() optsize personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
 entry:
   invoke void @good(i32 1, i32 2, i32 3, i32 4)
           to label %continue1 unwind label %cleanup
 continue1:
   invoke void @empty()
           to label %continue2 unwind label %cleanup
 continue2:
   ret void          
 cleanup:  
   landingpad { i8*, i32 }
      cleanup
   ret void
 }
 
 ; This is actually inefficient - we don't need to repeat the .cfi_escape twice.
 ; CHECK-LABEL: test6:
-; CHECK: .cfi_escape 0x2e, 0x10
-; CHECK: call
-; CHECK: .cfi_escape 0x2e, 0x10
-; CHECK: call
+; LINUX: .cfi_escape 0x2e, 0x10
+; LINUX: call
+; LINUX: .cfi_escape 0x2e, 0x10
+; LINUX: call
 define void @test6() optsize personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
 entry:
   invoke void @good(i32 1, i32 2, i32 3, i32 4)
           to label %continue1 unwind label %cleanup
 continue1:
   invoke void @good(i32 5, i32 6, i32 7, i32 8)
           to label %continue2 unwind label %cleanup
 continue2:
   ret void          
 cleanup:  
   landingpad { i8*, i32 }
      cleanup
   ret void
 }
+
+; Darwin should generate pushes in the presense of FP and an unwind table,
+; but not FP and invoke.
+; CHECK-LABEL: test7:
+; DARWIN: pushl %ebp
+; DARWIN: movl %esp, %ebp
+; DARWIN: .cfi_def_cfa_register %ebp
+; DARWIN-NOT: .cfi_adjust_cfa_offset
+; DARWIN: pushl   $4
+; DARWIN-NEXT: pushl   $3
+; DARWIN-NEXT: pushl   $2
+; DARWIN-NEXT: pushl   $1
+; DARWIN-NEXT: call
+define void @test7() optsize #0 personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+entry:
+  call void @good(i32 1, i32 2, i32 3, i32 4)
+  ret void
+}
+
+; CHECK-LABEL: test8:
+; DARWIN: pushl %ebp
+; DARWIN: movl %esp, %ebp
+; DARWIN-NOT: .cfi_adjust_cfa_offset
+; DARWIN-NOT: pushl
+define void @test8() optsize #0 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
+}
+
+attributes #0 = { optsize "no-frame-pointer-elim"="true" }