[Windows SEH] Fix the frame-ptr of a nested-filter within a _finally

[tentzen]

Summary

This change fixed a SEH bug (exposed by test58 & test61 in MSVC test xcpt4u.c);
when an Except-filter is located inside a finally, the frame-pointer generated today via intrinsic @llvm.eh.recoverfp is the frame-pointer of the immediate parent _finally, not the frame-ptr of outermost host function.

The fix is to retrieve the Establisher's frame-pointer that was previously saved in parent's frame.
The prolog of a filter inside a _finally should be like:

%0 = call i8* @llvm.eh.recoverfp(i8* bitcast (@"?fin$0@0@main@@"), i8* %frame_pointer)
%1 = call i8* @llvm.localrecover(i8* bitcast (@"?fin$0@0@main@@"), i8* %0, i32 0)
%2 = bitcast i8* %1 to i8**
%3 = load i8*, i8** %2, align 8

[tentzen]

Fixed the format at comment lines

[efriedma]

Please upload patches with full context (-U1000000).

clang/lib/CodeGen/CGException.cpp

1798

Maybe worth expanding this comment a little, to explain that a "finally" should have at most one localescape'ed variable. (At least, that's my understanding.)

1800

nullptr

Do you actually use ParentCGF.ParentCGF anywhere? If not, do you really need to save it?

1805

Using the name isn't reliable. You should be using data stored somewhere in the CodeGenFunction or something like that.

1822

Is there some reason you can't reuse recoverAddrOfEscapedLocal here?

clang/lib/CodeGen/CodeGenFunction.h

370

This comment isn't really helpful.

[tentzen]

clang/lib/CodeGen/CGException.cpp

1798

ok, will add more explanation.

1800

Yes, I used it in other patches. I found it's very handy to have a way to access parent CGF & Function.

1805

good sense. will update it.

1822

because recoverAddrOfEscapedLocal() is used to get escaped locals of outermost frame. The escaped frame-pointer we are looking for here is in _finally frame, not outermost frame.

[tentzen]

Remove hard-code name "frame-pointer".
get the name from 2nd Arg of the _finally().

[efriedma]

Again, using the name isn't reliable. Among other things, in release builds, IR values don't have names at all.

[tentzen]

In D77982#1978105, @efriedma wrote:

Again, using the name isn't reliable. Among other things, in release builds, IR values don't have names at all.

ok, will fix it.

[tentzen]

Do not use name comparison to locate parent's alloca of frame-pointer-addr.
search parent's LocalDeclMap instead.

[efriedma]

Searching LocalDeclMap is less problematic, I guess... but still, it should be possible to something more straightforward. Maybe make startOutlinedSEHHelper store the actual ImplicitParamDecl, or something like that.

[tentzen]

In D77982#1981505, @efriedma wrote:

Searching LocalDeclMap is less problematic, I guess... but still, it should be possible to something more straightforward. Maybe make startOutlinedSEHHelper store the actual ImplicitParamDecl, or something like that.

I respectfully disagree. Two reasons:
(1) Nested filter within a _finally is a rare case. Scanning CGF.LocalDeclMap is not much different from retrieving it from CGF.FuncletFramePointerAddr. Why do we want to store a redundant information in CGF for a rare case of one specific target?
(2) The code a paremeter’s home address is allocated today is in EmitParmDecl() which (and two its callers in call-stack) are all target agnostic functions. See code and call-stack below. To store DeclPtr in CGF for SEH filter only would require some target-specific code in those functions. Do you really think it’s what you want? I thought one implementation philosophy is to avoid target-specific code in target-independent functions.

      // Otherwise, create a temporary to hold the value.
      DeclPtr = CreateMemTemp(Ty, getContext().getDeclAlign(&D), D.getName() + ".addr");

	clang-cl.exe!clang::CodeGen::CodeGenFunction::EmitParmDecl(const clang::VarDecl & D, clang::CodeGen::CodeGenFunction::ParamValue Arg, unsigned int ArgNo) Line 2434	C++
 	clang-cl.exe!clang::CodeGen::CodeGenFunction::EmitFunctionProlog(const clang::CodeGen::CGFunctionInfo & FI, llvm::Function * Fn, const clang::CodeGen::FunctionArgList & Args) Line 2631	C++
 	clang-cl.exe!clang::CodeGen::CodeGenFunction::StartFunction(clang::GlobalDecl GD, clang::QualType RetTy, llvm::Function * Fn, const clang::CodeGen::CGFunctionInfo & FnInfo, const clang::CodeGen::FunctionArgList & Args, clang::SourceLocation Loc, clang::SourceLocation StartLoc) Line 1065	C++
 	clang-cl.exe!clang::CodeGen::CodeGenFunction::startOutlinedSEHHelper(clang::CodeGen::CodeGenFunction & ParentCGF, bool IsFilter, const clang::Stmt * OutlinedStmt) Line 2157	C++
 	clang-cl.exe!clang::CodeGen::CodeGenFunction::GenerateSEHFinallyFunction(clang::CodeGen::CodeGenFunction & ParentCGF, const clang::SEHFinallyStmt & Finally) Line 2190	C++
 	clang-cl.exe!clang::CodeGen::CodeGenFunction::EnterSEHTryStmt(const clang::SEHTryStmt & S) Line 2315	C++
 	clang-cl.exe!clang::CodeGen::CodeGenFunction::EmitSEHTryStmt(const clang::SEHTryStmt & S) Line 1622	C++
 	clang-cl.exe!clang::CodeGen::CodeGenFunction::EmitStmt(const clang::Stmt * S, llvm::ArrayRef<clang::Attr const *> Attrs) Line 191	C++
 	clang-cl.exe!clang::CodeGen::CodeGenFunction::EmitCompoundStmtWithoutScope(const clang::CompoundStmt & S, bool GetLast, clang::CodeGen::AggValueSlot AggSlot) Line 446	C++
 	clang-cl.exe!clang::CodeGen::CodeGenFunction::EmitFunctionBody(const clang::Stmt * Body) Line 1159	C++
 	clang-cl.exe!clang::CodeGen::CodeGenFunction::GenerateCode(clang::GlobalDecl GD, llvm::Function * Fn, const clang::CodeGen::CGFunctionInfo & FnInfo) Line 1325	C++

[efriedma]

For (1), I can see your point that it's sort of a balancing act. But generally, I'm concerned about making fragile assumptions: here, that LocalDeclMap contains precisely the two ImplicitParmDecls for the arguments, and nothing else. If we are going to assume that, I'd prefer stronger assertions so we don't accidentally break that assumption in the future.

re: (2), I don't have a problem using the LocalDeclMap, just the part where you scan it using some fragile assumptions. I was thinking you would save the ImplicitParmDecl*, not the actual alloca.

[tentzen]

The fix there deals with SEH filter with SEH _finally parent where its prototype is FIXED (2 implicit parameters). It will never change.

For #2,

"...I was thinking you would save the ImplicitParmDecl*, not the actual alloca".

If we just savw ImplicitParmDecl, we will still need to search Prolog for alloca BY NAME. Is not it you are concerned with most?

[efriedma]

If you can assert(ParentCGF.LocalDeclMap.size() == 2);, I guess the current code is good enough.

[tentzen]

It can be greater than 2 because this Map includes Decls of User locals from parent.
see CodeGenFunction::EmitCapturedLocals() (the same place of this fix).

..
auto I = ParentCGF.LocalDeclMap.find(VD);
if (I == ParentCGF.LocalDeclMap.end())
  continue;

Address ParentVar = I->second;
setAddrOfLocalVar(
    VD, recoverAddrOfEscapedLocal(ParentCGF, ParentVar, ParentFP));

[tentzen]

any more concern or comment?
thanks,

[tentzen]

Hi, Anton,
I found you are the Code Owner of "Exception handling, Windows CodeGen, ARM EABI".
could you please provide a quick review here? thanks,

[tentzen]

Hi, Is there more concern?
To re-iterate the implementation strategy of this change:

This is a rare case that only manifests itself under Windows SEH. We don't want to pollut target agnostic codes.
The ABI of SEH _finally is fixed with two implicit parameters; one abnormal execution and one establisher Stack-pointer. This ABI will never change, or a huge problem will arise.
CGF.LocalDeclMap is the fundamental data structure in Clang-CodeGen phase with one primary purpose, storing alloca instructions. Retrieving alloca of spilling instruction for 2nd implicit-argument from that data structure is legitimate and robust.
thanks,

[tentzen]

Hi, does this look good? or is there any other concern?
thanks,

[tentzen]

this patch has lasted for a couple of months. a bug in this area is hard and time-consuming to diagnose.
it's better to get this fix in sooner than later. could someone review and approve it?
thanks,

[asmith]

Thanks for all the helpful feedback. Before we commit this just want to double check there are no final comments?

clang/lib/CodeGen/CGException.cpp

void CodeGenFunction::EmitCapturedLocals(CodeGenFunction &ParentCGF,
 
   llvm::Value *EntryFP = nullptr;
   CGBuilderTy Builder(CGM, AllocaInsertPt);
   if (IsFilter && CGM.getTarget().getTriple().getArch() == llvm::Triple::x86) {
     // 32-bit SEH filters need to be careful about FP recovery.  The end of the
     // EH registration is passed in as the EBP physical register.  We can
     // recover that with llvm.frameaddress(1).
     EntryFP = Builder.CreateCall(
         CGM.getIntrinsic(llvm::Intrinsic::frameaddress, AllocaInt8PtrTy),
         {Builder.getInt32(1)});
   } else {
     // Otherwise, for x64 and 32-bit finally functions, the parent FP is the
     // second parameter.
     auto AI = CurFn->arg_begin();
     ++AI;
     EntryFP = &*AI;
   }
 
   llvm::Value *ParentFP = EntryFP;
   if (IsFilter) {
     // Given whatever FP the runtime provided us in EntryFP, recover the true
     // frame pointer of the parent function. We only need to do this in filters,
     // since finally funclets recover the parent FP for us.
     llvm::Function *RecoverFPIntrin =
         CGM.getIntrinsic(llvm::Intrinsic::eh_recoverfp);
     llvm::Constant *ParentI8Fn =
         llvm::ConstantExpr::getBitCast(ParentCGF.CurFn, Int8PtrTy);
     ParentFP = Builder.CreateCall(RecoverFPIntrin, {ParentI8Fn, EntryFP});
+
+    // if the parent is a _finally, the passed-in ParentFP is the FP
+    // of parent _finally, not Establisher's FP (FP of outermost function).
+    // Establkisher FP is 2nd paramenter passed into parent _finally.
+    // Fortunately, it's always saved in parent's frame. The following
+    // code retrieves it, and escapes it so that spill instruction won't be
+    // optimized away.
+    if (ParentCGF.ParentCGF != nullptr) {
+      // Locate and escape Parent's frame_pointer.addr alloca
+      // Depending on target, should be 1st/2nd one in LocalDeclMap.
+      // Let's just scan for ImplicitParamDecl with VoidPtrTy.
+      llvm::AllocaInst *FramePtrAddrAlloca = nullptr;
+      for (auto &I : ParentCGF.LocalDeclMap) {
+        const VarDecl *D = cast<VarDecl>(I.first);
+        if (isa<ImplicitParamDecl>(D) &&
+            D->getType() == getContext().VoidPtrTy) {
+          assert(D->getName().startswith("frame_pointer"));
+          FramePtrAddrAlloca = cast<llvm::AllocaInst>(I.second.getPointer());
+          break;
+        }
+      }
+      assert(FramePtrAddrAlloca);
+      auto InsertPair = ParentCGF.EscapedLocals.insert(
+          std::make_pair(FramePtrAddrAlloca, ParentCGF.EscapedLocals.size()));
+      int FrameEscapeIdx = InsertPair.first->second;
+
+      // an example of a filter's prolog::
+      // %0 = call i8* @llvm.eh.recoverfp(bitcast(@"?fin$0@0@main@@"),..)
+      // %1 = call i8* @llvm.localrecover(bitcast(@"?fin$0@0@main@@"),..)
+      // %2 = bitcast i8* %1 to i8**
+      // %3 = load i8*, i8* *%2, align 8
+      //   ==> %3 is the frame-pointer of outermost host function
+      llvm::Function *FrameRecoverFn = llvm::Intrinsic::getDeclaration(
+          &CGM.getModule(), llvm::Intrinsic::localrecover);
+      llvm::Constant *ParentI8Fn =
+          llvm::ConstantExpr::getBitCast(ParentCGF.CurFn, Int8PtrTy);
+      ParentFP = Builder.CreateCall(
+          FrameRecoverFn, {ParentI8Fn, ParentFP,
+                           llvm::ConstantInt::get(Int32Ty, FrameEscapeIdx)});
+      ParentFP = Builder.CreateBitCast(ParentFP, CGM.VoidPtrPtrTy);
+      ParentFP = Builder.CreateLoad(Address(ParentFP, getPointerAlign()));
+    }
   }
 
   // Create llvm.localrecover calls for all captures.
   for (const VarDecl *VD : Finder.Captures) {
     if (isa<ImplicitParamDecl>(VD)) {
       CGM.ErrorUnsupported(VD, "'this' captured by SEH");
       CXXThisValue = llvm::UndefValue::get(ConvertTypeForMem(VD->getType()));
       continue;

 
 void CodeGenFunction::pushSEHCleanup(CleanupKind Kind,
                                      llvm::Function *FinallyFunc) {
   EHStack.pushCleanup<PerformSEHFinally>(Kind, FinallyFunc);
 }
 
 void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt &S) {
   CodeGenFunction HelperCGF(CGM, /*suppressNewContext=*/true);
+  HelperCGF.ParentCGF = this;
   if (const SEHFinallyStmt *Finally = S.getFinallyHandler()) {
     // Outline the finally block.
     llvm::Function *FinallyFunc =
         HelperCGF.GenerateSEHFinallyFunction(*this, *Finally);
 
     // Push a cleanup for __finally blocks.
     EHStack.pushCleanup<PerformSEHFinally>(NormalAndEHCleanup, FinallyFunc);
     return;

clang/lib/CodeGen/CodeGenFunction.h

private:
     llvm::BasicBlock *Block;
     EHScopeStack::stable_iterator ScopeDepth;
     unsigned Index;
   };
 
   CodeGenModule &CGM;  // Per-module state.
   const TargetInfo &Target;
 
+  // For EH/SEH outlined funclets, this field points to parent's CGF
+  CodeGenFunction *ParentCGF = nullptr;
+
   typedef std::pair<llvm::Value *, llvm::Value *> ComplexPairTy;
   LoopInfoStack LoopStack;
   CGBuilderTy Builder;
 
   // Stores variables for which we can't generate correct lifetime markers
   // because of jumps.
   VarBypassDetector Bypasses;
 
public:
     explicit CGCapturedStmtInfo(CapturedRegionKind K = CR_Default)
         : Kind(K), ThisValue(nullptr), CXXThisFieldDecl(nullptr) {}
     explicit CGCapturedStmtInfo(const CapturedStmt &S,
                                 CapturedRegionKind K = CR_Default)
       : Kind(K), ThisValue(nullptr), CXXThisFieldDecl(nullptr) {
 
       RecordDecl::field_iterator Field =
         S.getCapturedRecordDecl()->field_begin();
       for (CapturedStmt::const_capture_iterator I = S.capture_begin(),
                                                 E = S.capture_end();
            I != E; ++I, ++Field) {
         if (I->capturesThis())
           CXXThisFieldDecl = *Field;
         else if (I->capturesVariable())
           CaptureFields[I->getCapturedVar()->getCanonicalDecl()] = *Field;
         else if (I->capturesVariableByCopy())
           CaptureFields[I->getCapturedVar()->getCanonicalDecl()] = *Field;

clang/test/CodeGen/windows-seh-filter-inFinally.c

+// RUN: %clang_cc1 -triple x86_64-windows -fms-extensions -Wno-implicit-function-declaration -S -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: %[[dst:[0-9-]+]] = call i8* @llvm.eh.recoverfp(i8* bitcast (void (i8, i8*)* @"?fin$0@0@main@@" to i8*), i8* %frame_pointer)
+// CHECK-NEXT: %[[dst1:[0-9-]+]] = call i8* @llvm.localrecover(i8* bitcast (void (i8, i8*)* @"?fin$0@0@main@@" to i8*), i8* %[[dst]], i32 0)
+// CHECK-NEXT: %[[dst2:[0-9-]+]] = bitcast i8* %[[dst1]] to i8**
+// CHECK-NEXT: = load i8*, i8** %[[dst2]], align 8
+
+int
+main(int argc, char *argv[])
+{
+    int Counter = 0;
+    //
+    // Try/except within the finally clause of a try/finally.
+    //
+    __try {
+      Counter -= 1;
+    }
+    __finally {
+      __try {
+        Counter += 2;
+        // RtlRaiseStatus(STATUS_INTEGER_OVERFLOW);
+      } __except(Counter) {
+        __try {
+          Counter += 3;
+        }
+        __finally {
+          if (abnormal_termination() == 1) {
+            Counter += 5;
+          }
+        }
+      }
+    }
+    // expect Counter == 9
+    return 1;
+}
+