Avoid emitting unreachable SP adjustments after `throw`
authorReid Kleckner <rnk@google.com>
Thu, 5 Mar 2020 23:37:21 +0000 (15:37 -0800)
committerReid Kleckner <rnk@google.com>
Fri, 6 Mar 2020 21:33:45 +0000 (13:33 -0800)
In 172eee9c, we tried to avoid these by modelling the callee as
internally resetting the stack pointer.

However, for the majority of functions with reserved stack frames, this
would lead LLVM to emit extra SP adjustments to undo the callee's
internal adjustment. This lead us to fix the problem further on down the
pipeline in eliminateCallFramePseudoInstr. In 5b79e603d3b7a2994, I added
use a heuristic to try to detect when the adjustment would be
unreachable.

This heuristic is imperfect, and when exception handling is involved, it
fails to fire. The new test is an example of this. Simply throwing an
exception with an active cleanup emits dead SP adjustments after the
throw. Not only are they dead, but if they were executed, they would be
incorrect, so they are confusing.

This change essentially reverts 172eee9c and makes the 5b79e603d3b7a2994
heuristic responsible for preventing unreachable stack adjustments. This
means we may emit unreachable stack adjustments for functions using EH
with unreserved call frames, but that is not very many these days. Back
in 2016 when this change was added, we were focused on 32-bit, which we
observed to have fewer reserved frames.

Fixes PR45064

Reviewed By: hans

Differential Revision: https://reviews.llvm.org/D75712

llvm/lib/Target/X86/X86FrameLowering.cpp
llvm/lib/Target/X86/X86ISelLowering.cpp
llvm/test/CodeGen/X86/noreturn-call-win64.ll

index 9d7d5b7..bc88401 100644 (file)
@@ -3009,6 +3009,12 @@ eliminateCallFramePseudoInstr(MachineFunction &MF, MachineBasicBlock &MBB,
   I = MBB.erase(I);
   auto InsertPos = skipDebugInstructionsForward(I, MBB.end());
 
+  // Try to avoid emitting dead SP adjustments if the block end is unreachable,
+  // typically because the function is marked noreturn (abort, throw,
+  // assert_fail, etc).
+  if (isDestroy && blockEndIsUnreachable(MBB, I))
+    return I;
+
   if (!reserveCallFrame) {
     // If the stack pointer can be changed after prologue, turn the
     // adjcallstackup instruction into a 'sub ESP, <amt>' and the
@@ -3091,13 +3097,7 @@ eliminateCallFramePseudoInstr(MachineFunction &MF, MachineBasicBlock &MBB,
     return I;
   }
 
-  if (isDestroy && InternalAmt && !blockEndIsUnreachable(MBB, I)) {
-    // If we are performing frame pointer elimination and if the callee pops
-    // something off the stack pointer, add it back.  We do this until we have
-    // more advanced stack pointer tracking ability.
-    // We are not tracking the stack pointer adjustment by the callee, so make
-    // sure we restore the stack pointer immediately after the call, there may
-    // be spill code inserted between the CALL and ADJCALLSTACKUP instructions.
+  if (InternalAmt) {
     MachineBasicBlock::iterator CI = I;
     MachineBasicBlock::iterator B = MBB.begin();
     while (CI != B && !std::prev(CI)->isCall())
index 6b32769..9610706 100644 (file)
@@ -4362,12 +4362,6 @@ X86TargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
   else
     NumBytesForCalleeToPop = 0;  // Callee pops nothing.
 
-  if (CLI.DoesNotReturn && !getTargetMachine().Options.TrapUnreachable) {
-    // No need to reset the stack after the call if the call doesn't return. To
-    // make the MI verify, we'll pretend the callee does it for us.
-    NumBytesForCalleeToPop = NumBytes;
-  }
-
   // Returns a flag for retval copy to use.
   if (!IsSibcall) {
     Chain = DAG.getCALLSEQ_END(Chain,
index 6289eef..7f9dcc0 100644 (file)
@@ -1,5 +1,8 @@
 ; RUN: llc < %s -mtriple=x86_64-windows-msvc | FileCheck %s
 
+%struct.MakeCleanup = type { i8 }
+%eh.ThrowInfo = type { i32, i32, i32, i32 }
+
 ; Function Attrs: noinline nounwind optnone uwtable
 define dso_local i32 @foo() {
 entry:
@@ -51,3 +54,58 @@ declare dso_local i32 @cond()
 declare dso_local void @abort1() noreturn
 declare dso_local void @abort2() noreturn
 declare dso_local void @abort3() noreturn
+
+define dso_local void @throw_exception() uwtable personality i32 (...)* @__CxxFrameHandler3 {
+entry:
+  %o = alloca %struct.MakeCleanup, align 1
+  %call = invoke i32 @cond()
+          to label %invoke.cont unwind label %ehcleanup
+
+invoke.cont:                                      ; preds = %entry
+  %cmp1 = icmp eq i32 0, %call
+  br i1 %cmp1, label %if.then, label %if.end
+
+if.then:                                          ; preds = %invoke.cont
+  invoke void @_CxxThrowException(i8* null, %eh.ThrowInfo* null)
+          to label %unreachable unwind label %ehcleanup
+
+if.end:                                           ; preds = %invoke.cont
+  %call2 = invoke i32 @cond()
+          to label %invoke.cont1 unwind label %ehcleanup
+
+invoke.cont1:                                     ; preds = %if.end
+  %cmp2 = icmp eq i32 0, %call2
+  br i1 %cmp2, label %if.then3, label %if.end4
+
+if.then3:                                         ; preds = %invoke.cont1
+  invoke void @_CxxThrowException(i8* null, %eh.ThrowInfo* null)
+          to label %unreachable unwind label %ehcleanup
+
+if.end4:                                          ; preds = %invoke.cont1
+  call void @"??1MakeCleanup@@QEAA@XZ"(%struct.MakeCleanup* nonnull %o)
+  ret void
+
+ehcleanup:                                        ; preds = %if.then3, %if.end, %if.then, %entry
+  %cp = cleanuppad within none []
+  call void @"??1MakeCleanup@@QEAA@XZ"(%struct.MakeCleanup* nonnull %o) [ "funclet"(token %cp) ]
+  cleanupret from %cp unwind to caller
+
+unreachable:                                      ; preds = %if.then3, %if.then
+  unreachable
+}
+
+declare dso_local i32 @__CxxFrameHandler3(...)
+declare dso_local void @_CxxThrowException(i8*, %eh.ThrowInfo*)
+declare dso_local void @"??1MakeCleanup@@QEAA@XZ"(%struct.MakeCleanup*)
+
+; CHECK-LABEL: throw_exception:
+; CHECK: callq cond
+; CHECK: je
+; CHECK: callq cond
+; CHECK: je
+; CHECK: retq
+; CHECK: callq _CxxThrowException
+; CHECK-NOT: {{(addq|subq) .*, %rsp}}
+; CHECK: callq _CxxThrowException
+; CHECK-NOT: {{(addq|subq) .*, %rsp}}
+; CHECK: .seh_handlerdata