Avoid emitting unreachable SP adjustments after `throw`

[rnk]

Summary

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

[vitalybuka]

This patch breaks windows bot http://lab.llvm.org:8011/builders/sanitizer-windows/builds/59930

FAIL: AddressSanitizer-x86_64-windows :: TestCases/intercept-rethrow-exception.cpp (585 of 651)
******************** TEST 'AddressSanitizer-x86_64-windows :: TestCases/intercept-rethrow-exception.cpp' FAILED ********************
Script:
--
: 'RUN: at line 4';      C:/b/slave/sanitizer-windows/build/stage1/./bin/clang.exe  -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -gcodeview -gcolumn-info    -fexceptions -O0 C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\test\asan\TestCases\intercept-rethrow-exception.cpp -o C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\asan\X86_64WindowsConfig\TestCases\Output\intercept-rethrow-exception.cpp.tmp
: 'RUN: at line 5';    C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\asan\X86_64WindowsConfig\TestCases\Output\intercept-rethrow-exception.cpp.tmp
--
Exit Code: 255

Command Output (stdout):
--
$ ":" "RUN: at line 4"
$ "C:/b/slave/sanitizer-windows/build/stage1/./bin/clang.exe" "-fsanitize=address" "-mno-omit-leaf-frame-pointer" "-fno-omit-frame-pointer" "-fno-optimize-sibling-calls" "-gline-tables-only" "-gcodeview" "-gcolumn-info" "-fexceptions" "-O0" "C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\test\asan\TestCases\intercept-rethrow-exception.cpp" "-o" "C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\asan\X86_64WindowsConfig\TestCases\Output\intercept-rethrow-exception.cpp.tmp"
# command output:
   Creating library C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\asan\X86_64WindowsConfig\TestCases\Output\intercept-rethrow-exception.cpp.lib and object C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\asan\X86_64WindowsConfig\TestCases\Output\intercept-rethrow-exception.cpp.exp


$ ":" "RUN: at line 5"
$ "C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\asan\X86_64WindowsConfig\TestCases\Output\intercept-rethrow-exception.cpp.tmp"
note: command had no output on stdout or stderr
error: command failed with exit status: 255

[thakis]

Amy bisected https://bugs.chromium.org/p/chromium/issues/detail?id=1062021 down to this. Not clear if that's a compiler bug or some ODR bug like last time, but it's probably good to limit the amount of breakage on trunk.

[thakis]

Somehow forgot to mention the most important bit: I've reverted this in 4e0fe038f438ae1679eae9e156e1f248595b2373 for now.

llvm/lib/Target/X86/X86FrameLowering.cpp

eliminateCallFramePseudoInstr(MachineFunction &MF, MachineBasicBlock &MBB,
   unsigned Opcode = I->getOpcode();
   bool isDestroy = Opcode == TII.getCallFrameDestroyOpcode();
   DebugLoc DL = I->getDebugLoc();
   uint64_t Amount = TII.getFrameSize(*I);
   uint64_t InternalAmt = (isDestroy || Amount) ? TII.getFrameAdjustment(*I) : 0;
   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
     // adjcallstackdown instruction into 'add ESP, <amt>'
 
     // 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.
if (DwarfCFI && !hasFP(MF)) {
                  MCCFIInstruction::createAdjustCfaOffset(nullptr,
                                                          CfaAdjustment));
       }
     }
 
     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())
       --CI;
     BuildStackAdjustment(MBB, CI, DL, -InternalAmt, /*InEpilogue=*/false);
   }
 
   return I;

llvm/lib/Target/X86/X86ISelLowering.cpp

else if (!Is64Bit && !canGuaranteeTCO(CallConv) &&
     // If this is a call to a struct-return function, the callee
     // pops the hidden struct pointer, so we have to push it back.
     // This is common for Darwin/X86, Linux & Mingw32 targets.
     // For MSVC Win32 targets, the caller pops the hidden struct pointer.
     NumBytesForCalleeToPop = 4;
   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,
                                DAG.getIntPtrConstant(NumBytesToPop, dl, true),
                                DAG.getIntPtrConstant(NumBytesForCalleeToPop, dl,
                                                      true),
                                InFlag, dl);
     InFlag = Chain.getValue(1);

llvm/test/CodeGen/X86/noreturn-call-win64.ll

 ; 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:
   %call = call i32 @cond()
   %tobool = icmp ne i32 %call, 0
   br i1 %tobool, label %if.then, label %if.end
 
 if.then:                                          ; preds = %entry

 ; CHECK: callq abort3
 ; CHECK-NEXT: int3
 
 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