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

[Ten Tzen via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG66f1dcd872db: [Windows SEH] Fix the frame-ptr of a 
nested-filter within a _finally (authored by tentzen).

Changed prior to commit:
  https://reviews.llvm.org/D77982?vs=257227=277279#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/windows-seh-filter-inFinally.c

Index: clang/test/CodeGen/windows-seh-filter-inFinally.c
===
--- /dev/null
+++ clang/test/CodeGen/windows-seh-filter-inFinally.c
@@ -0,0 +1,36 @@
+// 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;
+}
+
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -264,6 +264,9 @@
   CodeGenModule   // Per-module state.
   const TargetInfo 
 
+  // For EH/SEH outlined funclets, this field points to parent's CGF
+  CodeGenFunction *ParentCGF = nullptr;
+
   typedef std::pair ComplexPairTy;
   LoopInfoStack LoopStack;
   CGBuilderTy Builder;
Index: clang/lib/CodeGen/CGException.cpp
===
--- clang/lib/CodeGen/CGException.cpp
+++ clang/lib/CodeGen/CGException.cpp
@@ -1815,6 +1815,48 @@
 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  : ParentCGF.LocalDeclMap) {
+const VarDecl *D = cast(I.first);
+if (isa(D) &&
+D->getType() == getContext().VoidPtrTy) {
+  assert(D->getName().startswith("frame_pointer"));
+  FramePtrAddrAlloca = cast(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(
+  (), 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.
@@ -2013,6 +2055,7 @@
 
 void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt ) {
   

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

[Aaron Smith via Phabricator via cfe-commits]

asmith added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen added a comment.

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,


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen added a comment.

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,


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen added a reviewer: asl.
tentzen added a comment.

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,


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen added a comment.

any more concern or comment?
thanks,


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen added a comment.

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));


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen added a comment.

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.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 2631C++

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 2157C++

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 2315C++
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 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++




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen updated this revision to Diff 257227.
tentzen added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/windows-seh-filter-inFinally.c

Index: clang/test/CodeGen/windows-seh-filter-inFinally.c
===
--- /dev/null
+++ clang/test/CodeGen/windows-seh-filter-inFinally.c
@@ -0,0 +1,36 @@
+// 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;
+}
+
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -367,6 +367,9 @@
   CodeGenModule   // Per-module state.
   const TargetInfo 
 
+  // For EH/SEH outlined funclets, this field points to parent's CGF
+  CodeGenFunction *ParentCGF = nullptr;
+
   typedef std::pair ComplexPairTy;
   LoopInfoStack LoopStack;
   CGBuilderTy Builder;
Index: clang/lib/CodeGen/CGException.cpp
===
--- clang/lib/CodeGen/CGException.cpp
+++ clang/lib/CodeGen/CGException.cpp
@@ -1794,6 +1794,48 @@
 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 ImplicitParamDecl with VoidPtrTy.
+  llvm::AllocaInst *FramePtrAddrAlloca = nullptr;
+  for (auto  : ParentCGF.LocalDeclMap) {
+const VarDecl *D = cast(I.first);
+if (isa(D) &&
+D->getType() == getContext().VoidPtrTy) {
+  assert(D->getName().startswith("frame_pointer"));
+  FramePtrAddrAlloca = cast(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(
+  (), 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.
@@ -1992,6 +2034,7 @@
 
 void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt ) {
   CodeGenFunction HelperCGF(CGM, /*suppressNewContext=*/true);
+  HelperCGF.ParentCGF = this;
   if (const 

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen updated this revision to Diff 256942.
tentzen added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/windows-seh-filter-inFinally.c

Index: clang/test/CodeGen/windows-seh-filter-inFinally.c
===
--- /dev/null
+++ clang/test/CodeGen/windows-seh-filter-inFinally.c
@@ -0,0 +1,36 @@
+// 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;
+}
+
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -367,6 +367,9 @@
   CodeGenModule   // Per-module state.
   const TargetInfo 
 
+  // For EH/SEH outlined funclets, this field points to parent's CGF
+  CodeGenFunction *ParentCGF = nullptr;
+
   typedef std::pair ComplexPairTy;
   LoopInfoStack LoopStack;
   CGBuilderTy Builder;
Index: clang/lib/CodeGen/CGException.cpp
===
--- clang/lib/CodeGen/CGException.cpp
+++ clang/lib/CodeGen/CGException.cpp
@@ -1794,6 +1794,46 @@
 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
+  llvm::Function *Fn = ParentCGF.CurFn;
+  StringRef Name = Fn->getArg(1)->getName();
+  llvm::AllocaInst *FramePtrAddrAlloca = nullptr;
+  for (llvm::Instruction  : ParentCGF.CurFn->getEntryBlock()) {
+llvm::AllocaInst *II = dyn_cast();
+if (II && II->getName().startswith(Name)) {
+  FramePtrAddrAlloca = II;
+  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(
+  (), 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.
@@ -1992,6 +2032,7 @@
 
 void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt ) {
   CodeGenFunction HelperCGF(CGM, /*suppressNewContext=*/true);
+  HelperCGF.ParentCGF = this;
   if (const SEHFinallyStmt *Finally = S.getFinallyHandler()) {
 // Outline the finally block.
 llvm::Function *FinallyFunc =

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen marked 4 inline comments as done.
tentzen added inline comments.



Comment at: clang/lib/CodeGen/CGException.cpp:1798
+
+// if the parent is a _finally, need to retrive Establisher's FP,
+//  2nd paramenter, saved & named frame_pointer in parent's frame

efriedma wrote:
> 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.)
ok, will add more explanation.



Comment at: clang/lib/CodeGen/CGException.cpp:1800
+//  2nd paramenter, saved & named frame_pointer in parent's frame
+if (ParentCGF.ParentCGF != NULL) {
+  // Locate and escape Parent's frame_pointer.addr alloca

efriedma wrote:
> nullptr
> 
> Do you actually use ParentCGF.ParentCGF anywhere?  If not, do you really need 
> to save it?
Yes, I used it in other patches.  I found it's very handy to have a way to 
access parent CGF & Function.



Comment at: clang/lib/CodeGen/CGException.cpp:1805
+llvm::AllocaInst *II = dyn_cast();
+if (II && II->getName().startswith("frame_pointer")) {
+  FramePtrAddrAlloca = II;

efriedma wrote:
> Using the name isn't reliable. You should be using data stored somewhere in 
> the CodeGenFunction or something like that.
good sense.  will update it.



Comment at: clang/lib/CodeGen/CGException.cpp:1822
+  llvm::Function *FrameRecoverFn = llvm::Intrinsic::getDeclaration(
+  (), llvm::Intrinsic::localrecover);
+  llvm::Constant *ParentI8Fn =

efriedma wrote:
> Is there some reason you can't reuse recoverAddrOfEscapedLocal here?
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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Please upload patches with full context (-U100).




Comment at: clang/lib/CodeGen/CGException.cpp:1798
+
+// if the parent is a _finally, need to retrive Establisher's FP,
+//  2nd paramenter, saved & named frame_pointer in parent's frame

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.)



Comment at: clang/lib/CodeGen/CGException.cpp:1800
+//  2nd paramenter, saved & named frame_pointer in parent's frame
+if (ParentCGF.ParentCGF != NULL) {
+  // Locate and escape Parent's frame_pointer.addr alloca

nullptr

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



Comment at: clang/lib/CodeGen/CGException.cpp:1805
+llvm::AllocaInst *II = dyn_cast();
+if (II && II->getName().startswith("frame_pointer")) {
+  FramePtrAddrAlloca = II;

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



Comment at: clang/lib/CodeGen/CGException.cpp:1822
+  llvm::Function *FrameRecoverFn = llvm::Intrinsic::getDeclaration(
+  (), llvm::Intrinsic::localrecover);
+  llvm::Constant *ParentI8Fn =

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



Comment at: clang/lib/CodeGen/CodeGenFunction.h:370
 
+  // For outliner helper only
+  CodeGenFunction *ParentCGF = nullptr;

This comment isn't really helpful.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen updated this revision to Diff 256896.
tentzen added a comment.

Fixed the format at comment lines


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77982/new/

https://reviews.llvm.org/D77982

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/windows-seh-filter-inFinally.c

Index: clang/test/CodeGen/windows-seh-filter-inFinally.c
===
--- /dev/null
+++ clang/test/CodeGen/windows-seh-filter-inFinally.c
@@ -0,0 +1,36 @@
+// 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;
+}
+
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -367,6 +367,9 @@
   CodeGenModule   // Per-module state.
   const TargetInfo 
 
+  // For outliner helper only
+  CodeGenFunction *ParentCGF = nullptr;
+
   typedef std::pair ComplexPairTy;
   LoopInfoStack LoopStack;
   CGBuilderTy Builder;
Index: clang/lib/CodeGen/CGException.cpp
===
--- clang/lib/CodeGen/CGException.cpp
+++ clang/lib/CodeGen/CGException.cpp
@@ -1794,6 +1794,40 @@
 llvm::Constant *ParentI8Fn =
 llvm::ConstantExpr::getBitCast(ParentCGF.CurFn, Int8PtrTy);
 ParentFP = Builder.CreateCall(RecoverFPIntrin, {ParentI8Fn, EntryFP});
+
+// if the parent is a _finally, need to retrive Establisher's FP,
+//  2nd paramenter, saved & named frame_pointer in parent's frame
+if (ParentCGF.ParentCGF != NULL) {
+  // Locate and escape Parent's frame_pointer.addr alloca
+  llvm::AllocaInst *FramePtrAddrAlloca = nullptr;
+  for (llvm::Instruction  : ParentCGF.CurFn->getEntryBlock()) {
+llvm::AllocaInst *II = dyn_cast();
+if (II && II->getName().startswith("frame_pointer")) {
+  FramePtrAddrAlloca = II;
+  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(
+  (), 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.
@@ -1992,6 +2026,7 @@
 
 void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt ) {
   CodeGenFunction HelperCGF(CGM, /*suppressNewContext=*/true);
+  HelperCGF.ParentCGF = this;
   if (const SEHFinallyStmt *Finally = S.getFinallyHandler()) {
 // Outline the finally block.
 llvm::Function *FinallyFunc =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ten Tzen via Phabricator via cfe-commits]

tentzen created this revision.
tentzen added reviewers: rnk, eli.friedman, JosephTremoulet, asmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77982

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/windows-seh-filter-inFinally.c

Index: clang/test/CodeGen/windows-seh-filter-inFinally.c
===
--- /dev/null
+++ clang/test/CodeGen/windows-seh-filter-inFinally.c
@@ -0,0 +1,36 @@
+// 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;
+}
+
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -367,6 +367,9 @@
   CodeGenModule   // Per-module state.
   const TargetInfo 
 
+  // For outliner helper only
+  CodeGenFunction *ParentCGF = nullptr;
+
   typedef std::pair ComplexPairTy;
   LoopInfoStack LoopStack;
   CGBuilderTy Builder;
Index: clang/lib/CodeGen/CGException.cpp
===
--- clang/lib/CodeGen/CGException.cpp
+++ clang/lib/CodeGen/CGException.cpp
@@ -1794,6 +1794,40 @@
 llvm::Constant *ParentI8Fn =
 llvm::ConstantExpr::getBitCast(ParentCGF.CurFn, Int8PtrTy);
 ParentFP = Builder.CreateCall(RecoverFPIntrin, {ParentI8Fn, EntryFP});
+
+// if the parent is a _finally, need to retrive Establisher's FP,
+//  2nd paramenter, saved & named frame_pointer in parent's frame
+if (ParentCGF.ParentCGF != NULL) {
+  // Locate and escape Parent's frame_pointer.addr alloca
+  llvm::AllocaInst *FramePtrAddrAlloca = nullptr;
+  for (llvm::Instruction  : ParentCGF.CurFn->getEntryBlock()) {
+llvm::AllocaInst *II = dyn_cast();
+if (II && II->getName().startswith("frame_pointer")) {
+  FramePtrAddrAlloca = II;
+  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(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
+  //   ==> %3 is the frame-pointer of outermost host function
+  llvm::Function *FrameRecoverFn = llvm::Intrinsic::getDeclaration(
+  (), 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.
@@ -1992,6 +2026,7 @@