Page MenuHomePhabricator

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

Authored by tentzen on Apr 12 2020, 3:53 PM.

Details

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

Diff Detail

Event Timeline

tentzen created this revision.Apr 12 2020, 3:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2020, 3:53 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tentzen updated this revision to Diff 256896.Apr 12 2020, 5:24 PM

Fixed the format at comment lines

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 marked 4 inline comments as done.Apr 13 2020, 1:08 AM
tentzen added inline comments.
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 updated this revision to Diff 256942.Apr 13 2020, 1:47 AM

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

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

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 updated this revision to Diff 257227.Apr 14 2020, 1:31 AM

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

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.

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

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.

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?

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

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

any more concern or comment?
thanks,

tentzen added a reviewer: asl.May 1 2020, 6:20 PM

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,

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,

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

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,

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

asmith accepted this revision.Fri, Jul 10, 11:38 AM
This revision is now accepted and ready to land.Fri, Jul 10, 11:38 AM