[Windows EH] Fix the order of Nested try-catches in $tryMap$ table

[asmith]

Summary

This bug is exposed by Test7 of ehthrow.cxx in MSVC EH suite where
a rethrow occurs in a try-catch inside a catch (i.e., a nested Catch
handlers). See the test code in
https://github.com/microsoft/compiler-tests/blob/master/eh/ehthrow.cxx#L346

When an object is rethrown in a Catch handler, the copy-ctor of this
object must be executed after the destructions of live objects, but
BEFORE the dtors of live objects in parent handlers.

Today Windows runtime (__CxxFrameHandler3 & 4) expects nested Catch handers
are stored in pre-order (outer first, inner next) in $tryMap$ table, so
that given a State, its Catch's beginning State can be properly
retrieved. The Catch beginning state (which is also the ending State) is
the State where rethrown object's copy-ctor must take place.

LLVM currently stores nested catch handlers in post-ordering because
it's the natural way to compute the highest State in Catch.
The fix is to simply store TryCatch handler in pre-order, but update
Catch's highest State after child Catches are all processed.

[asmith]

Nit - update comment

[rnk]

Sounds good, and the fix looks simple, but please add a test.

I'm surprised this change doesn't break an existing test, I'd expect us to have a test for the LSDA of a try-in-catch:

try { }
catch () {
  try { }
  catch () { }
}

As I understand it, the outer TBME must appear before the inner one, and the current behavior puts them in the other way around, innermost first.

[tentzen]

In D77920#1984956, @rnk wrote:

Sounds good, and the fix looks simple, but please add a test.

I'm surprised this change doesn't break an existing test, I'd expect us to have a test for the LSDA of a try-in-catch:

try { }
catch () {
  try { }
  catch () { }
}

As I understand it, the outer TBME must appear before the inner one, and the current behavior puts them in the other way around, innermost first.

Are you questioning the throw in outer _try or inner _try? Windows runtime expects outer-first and inner-next ordering. It searches TBME table in REVERSED ordering. So for throw in inner _try, the inner catch is located. For the throw in outer _try, the State of it is lower then inner one. So the inner catch will be skipped and outer catch will be located. Hope this answer your question.

[rnk]

In D77920#1985315, @tentzen wrote:

Are you questioning the throw in outer _try or inner _try? Windows runtime expects outer-first and inner-next ordering. It searches TBME table in REVERSED ordering. So for throw in inner _try, the inner catch is located. For the throw in outer _try, the State of it is lower then inner one. So the inner catch will be skipped and outer catch will be located. Hope this answer your question.

Yep, this makes sense, I understand how we want to rearrange the table.

My concern is that I thought we already have in-tree tests for this case, and I can see from the Harbormaster link on this patch that they will fail:
https://reviews.llvm.org/harbormaster/unit/view/57903/
LLVM.CodeGen/X86::win-catchpad-nested-cxx.ll

So, those test expectations need to be updated to land this patch.

[tentzen]

right, thank you Reid. I see it now :

; CHECK: $tryMap$try_in_catch:
; CHECK-NEXT: .long 2
; CHECK-NEXT: .long 2
; CHECK-NEXT: .long 3
; CHECK-NEXT: .long 1
; CHECK-NEXT: .long ($handlerMap$0$try_in_catch)
; CHECK-NEXT: .long 0
; CHECK-NEXT: .long 0
; CHECK-NEXT: .long 3
; CHECK-NEXT: .long 1
; CHECK-NEXT: .long ($handlerMap$1$try_in_catch)

the order must be reversed.

[asmith]

Thanks for all the comments! Does this look good? Or other considerations?

[asmith]

LGTM. Anyone else have comments?

[rnk]

The code change looks correct, but the test still needs to be updated. I'll go ahead and mark it accepted, but please remember to do that when landing this.

[asmith]

Duplicated by https://reviews.llvm.org/D79474

llvm/lib/CodeGen/WinEHPrepare.cpp

       if ((PredBlock = getEHPadFromPredecessor(PredBlock,
                                                CatchSwitch->getParentPad())))
         calculateCXXStateNumbers(FuncInfo, PredBlock->getFirstNonPHI(),
                                  TryLow);
     int CatchLow = addUnwindMapEntry(FuncInfo, ParentState, nullptr);
 
     // catchpads are separate funclets in C++ EH due to the way rethrow works.
     int TryHigh = CatchLow - 1;
+
+    // MSVC FrameHandler3/4 expect that Catch Handlers in $tryMap$ are
+    // stored in pre-order (outer first, inner next), not post-order.
+    // Add a map here to fix CatchHigh after the children are processed.
+    addTryBlockMapEntry(FuncInfo, TryLow, TryHigh, CatchLow, Handlers);
+    unsigned TBMEIdx = FuncInfo.TryBlockMap.size() - 1;
+
     for (const auto *CatchPad : Handlers) {
       FuncInfo.FuncletBaseStateMap[CatchPad] = CatchLow;
       for (const User *U : CatchPad->users()) {
         const auto *UserI = cast<Instruction>(U);
         if (auto *InnerCatchSwitch = dyn_cast<CatchSwitchInst>(UserI)) {
           BasicBlock *UnwindDest = InnerCatchSwitch->getUnwindDest();
           if (!UnwindDest || UnwindDest == CatchSwitch->getUnwindDest())
             calculateCXXStateNumbers(FuncInfo, UserI, CatchLow);
         }
         if (auto *InnerCleanupPad = dyn_cast<CleanupPadInst>(UserI)) {
           BasicBlock *UnwindDest = getCleanupRetUnwindDest(InnerCleanupPad);
           // If a nested cleanup pad reports a null unwind destination and the
           // enclosing catch pad doesn't it must be post-dominated by an
           // unreachable instruction.
           if (!UnwindDest || UnwindDest == CatchSwitch->getUnwindDest())
             calculateCXXStateNumbers(FuncInfo, UserI, CatchLow);
         }
       }
     }
     int CatchHigh = FuncInfo.getLastStateNumber();
-    addTryBlockMapEntry(FuncInfo, TryLow, TryHigh, CatchHigh, Handlers);
+    // Now child Catches are processed, update CatchHigh
+    FuncInfo.TryBlockMap[TBMEIdx].CatchHigh = CatchHigh;
     LLVM_DEBUG(dbgs() << "TryLow[" << BB->getName() << "]: " << TryLow << '\n');
     LLVM_DEBUG(dbgs() << "TryHigh[" << BB->getName() << "]: " << TryHigh
                       << '\n');
     LLVM_DEBUG(dbgs() << "CatchHigh[" << BB->getName() << "]: " << CatchHigh
                       << '\n');
   } else {
     auto *CleanupPad = cast<CleanupPadInst>(FirstNonPHI);