[StackColoring] Handle SEH catch object stack slots conservatively

The write to the SEH catch object happens before cleanuppads are
executed, while the first reference to the object will typically
be in a catchpad.

If we make use of first-use analysis, we may end up allocating
an alloca used inside the cleanuppad and the catch object at the
same stack offset, which would be incorrect.

https://reviews.llvm.org/D86673 was a previous attempt to fix it.
It used the heuristic "a slot loaded in a WinEH pad and never
written" to detect catch objects. However, because it checks
for more than one load (while probably more than zero was
intended), the fix does not actually work.

The general approach also seems dubious to me, so this patch
reverts that change entirely, and instead marks all catch object
slots as conservative (i.e. excluded from first-use analysis)
based on the WinEHFuncInfo. As far as I can tell we don't need
any heuristics here, we know exactly which slots are affected.

Fixes llvm#66984.

(cherry picked from commit b3cb4f0)

Author: nikic

llvm/lib/CodeGen/StackColoring.cpp

@@ -370,37 +370,6 @@ STATISTIC(EscapedAllocas, "Number of allocas that escaped the lifetime region");
 // If in RPO ordering chosen to walk the CFG  we happen to visit the b[k]
 // before visiting the memcpy block (which will contain the lifetime start
 // for "b" then it will appear that 'b' has a degenerate lifetime.
-//
-// Handle Windows Exception with LifetimeStartOnFirstUse:
-// -----------------
-//
-// There was a bug for using LifetimeStartOnFirstUse in win32.
-// class Type1 {
-// ...
-// ~Type1(){ write memory;}
-// }
-// ...
-// try{
-// Type1 V
-// ...
-// } catch (Type2 X){
-// ...
-// }
-// For variable X in catch(X), we put point pX=&(&X) into ConservativeSlots
-// to prevent using LifetimeStartOnFirstUse. Because pX may merged with
-// object V which may call destructor after implicitly writing pX. All these
-// are done in C++ EH runtime libs (through CxxThrowException), and can't
-// obviously check it in IR level.
-//
-// The loader of pX, without obvious writing IR, is usually the first LOAD MI
-// in EHPad, Some like:
-// bb.x.catch.i (landing-pad, ehfunclet-entry):
-// ; predecessors: %bb...
-//   successors: %bb...
-//  %n:gr32 = MOV32rm %stack.pX ...
-//  ...
-// The Type2** %stack.pX will only be written in EH runtime libs, so we
-// check the StoreSlots to screen it out.
 
 namespace {
 
@@ -462,9 +431,6 @@ class StackColoring : public MachineFunctionPass {
   /// slots lifetime-start-on-first-use is disabled).
   BitVector ConservativeSlots;
 
-  /// Record the FI slots referenced by a 'may write to memory'.
-  BitVector StoreSlots;
-
   /// Number of iterations taken during data flow analysis.
   unsigned NumIterations;
 
@@ -660,13 +626,10 @@ unsigned StackColoring::collectMarkers(unsigned NumSlot) {
   InterestingSlots.resize(NumSlot);
   ConservativeSlots.clear();
   ConservativeSlots.resize(NumSlot);
-  StoreSlots.clear();
-  StoreSlots.resize(NumSlot);
 
   // number of start and end lifetime ops for each slot
   SmallVector<int, 8> NumStartLifetimes(NumSlot, 0);
   SmallVector<int, 8> NumEndLifetimes(NumSlot, 0);
-  SmallVector<int, 8> NumLoadInCatchPad(NumSlot, 0);
 
   // Step 1: collect markers and populate the "InterestingSlots"
   // and "ConservativeSlots" sets.
@@ -722,13 +685,6 @@ unsigned StackColoring::collectMarkers(unsigned NumSlot) {
           if (! BetweenStartEnd.test(Slot)) {
             ConservativeSlots.set(Slot);
           }
-          // Here we check the StoreSlots to screen catch point out. For more
-          // information, please refer "Handle Windows Exception with
-          // LifetimeStartOnFirstUse" at the head of this file.
-          if (MI.mayStore())
-            StoreSlots.set(Slot);
-          if (MF->getWinEHFuncInfo() && MBB->isEHPad() && MI.mayLoad())
-            NumLoadInCatchPad[Slot] += 1;
         }
       }
     }
@@ -739,14 +695,23 @@ unsigned StackColoring::collectMarkers(unsigned NumSlot) {
     return 0;
   }
 
-  // 1) PR27903: slots with multiple start or end lifetime ops are not
+  // PR27903: slots with multiple start or end lifetime ops are not
   // safe to enable for "lifetime-start-on-first-use".
-  // 2) And also not safe for variable X in catch(X) in windows.
   for (unsigned slot = 0; slot < NumSlot; ++slot) {
-    if (NumStartLifetimes[slot] > 1 || NumEndLifetimes[slot] > 1 ||
-        (NumLoadInCatchPad[slot] > 1 && !StoreSlots.test(slot)))
+    if (NumStartLifetimes[slot] > 1 || NumEndLifetimes[slot] > 1)
       ConservativeSlots.set(slot);
   }
+
+  // The write to the catch object by the personality function is not propely
+  // modeled in IR: It happens before any cleanuppads are executed, even if the
+  // first mention of the catch object is in a catchpad. As such, mark catch
+  // object slots as conservative, so they are excluded from first-use analysis.
+  if (WinEHFuncInfo *EHInfo = MF->getWinEHFuncInfo())
+    for (WinEHTryBlockMapEntry &TBME : EHInfo->TryBlockMap)
+      for (WinEHHandlerType &H : TBME.HandlerArray)
+        if (H.CatchObj.FrameIndex != std::numeric_limits<int>::max())
+          ConservativeSlots.set(H.CatchObj.FrameIndex);
+
   LLVM_DEBUG(dumpBV("Conservative slots", ConservativeSlots));
 
   // Step 2: compute begin/end sets for each block

llvm/test/CodeGen/X86/stack-coloring-seh.ll renamed to llvm/test/CodeGen/X86/stack-coloring-wineh.ll

@@ -3,7 +3,7 @@
 
 @type_info = external global ptr
 
-; FIXME: This is a miscompile.
+; Make sure %a1 and %a2 don't share the same stack offset.
 define void @pr66984(ptr %arg) personality ptr @__CxxFrameHandler3 {
 ; CHECK-LABEL: pr66984:
 ; CHECK:       # %bb.0: # %bb
@@ -12,7 +12,7 @@ define void @pr66984(ptr %arg) personality ptr @__CxxFrameHandler3 {
 ; CHECK-NEXT:    pushl %ebx
 ; CHECK-NEXT:    pushl %edi
 ; CHECK-NEXT:    pushl %esi
-; CHECK-NEXT:    subl $20, %esp
+; CHECK-NEXT:    subl $24, %esp
 ; CHECK-NEXT:    movl %esp, -28(%ebp)
 ; CHECK-NEXT:    movl $-1, -16(%ebp)
 ; CHECK-NEXT:    leal -24(%ebp), %eax
@@ -31,7 +31,7 @@ define void @pr66984(ptr %arg) personality ptr @__CxxFrameHandler3 {
 ; CHECK-NEXT:  $ehgcr_0_4:
 ; CHECK-NEXT:    movl -24(%ebp), %eax
 ; CHECK-NEXT:    movl %eax, %fs:0
-; CHECK-NEXT:    addl $20, %esp
+; CHECK-NEXT:    addl $24, %esp
 ; CHECK-NEXT:    popl %esi
 ; CHECK-NEXT:    popl %edi
 ; CHECK-NEXT:    popl %ebx
@@ -47,7 +47,7 @@ define void @pr66984(ptr %arg) personality ptr @__CxxFrameHandler3 {
 ; CHECK-NEXT:    pushl %ebp
 ; CHECK-NEXT:    addl $12, %ebp
 ; CHECK-NEXT:    movl %esp, -28(%ebp)
-; CHECK-NEXT:    movl -32(%ebp), %ecx
+; CHECK-NEXT:    movl -36(%ebp), %ecx
 ; CHECK-NEXT:    movl $2, -16(%ebp)
 ; CHECK-NEXT:    calll _cleanup
 ; CHECK-NEXT:    movl $LBB0_3, %eax