[exceptions] Upgrade exception handlers when stack protector is used

[etienneb]

Summary

MSVC provide exception handlers with enhanced information to deal with security buffer feature (/GS).

To be more secure, the security cookies (GS and SEH) are validated when unwinding the stack.

The following code:

void f() {}

void foo() {
  __try {
    f();
  } __except(1) {
    f();
  }
}

[etienneb]

This is a draft on the exception handlers upgrade.
For now, only 32-bits is implemented.

It's producing this code:

	.text
	.def	 @feat.00;
	.scl	3;
	.type	0;
	.endef
	.globl	@feat.00
@feat.00 = 1
	.def	 "?example@@YAHHH@Z";
	.scl	2;
	.type	32;
	.endef
	.globl	"?example@@YAHHH@Z"
	.p2align	4, 0x90
"?example@@YAHHH@Z":                    # @"\01?example@@YAHHH@Z"
Lfunc_begin0:
# BB#0:
	pushl	%ebp
	movl	%esp, %ebp
	pushl	%ebx
	pushl	%edi
	pushl	%esi
	subl	$48, %esp
	movl	$"L__ehtable$?example@@YAHHH@Z", %edx
	movl	%esp, -36(%ebp)
	movl	$-2, -16(%ebp)
	movl	12(%ebp), %ecx
	movl	8(%ebp), %eax
"L?example@@YAHHH@Z$frame_escape_0" = -44
	xorl	___security_cookie, %edx
	movl	%edx, -20(%ebp)
	movl	$__except_handler4, -24(%ebp)
	leal	-28(%ebp), %edx
	movl	%fs:0, %esi
	movl	%esi, -28(%ebp)
	movl	%edx, %fs:0
	movl	%ecx, -40(%ebp)
	movl	%eax, -48(%ebp)
	movl	$0, -16(%ebp)
	leal	-58(%ebp), %ecx
	movl	-40(%ebp), %eax
	pushl	%eax
	pushl	$204
	pushl	%ecx
	calll	"?memset@@YAPAXPAXHH@Z"
	addl	$12, %esp
LBB0_2:
	movl	-40(%ebp), %eax
	movl	-28(%ebp), %ecx
	movsbl	-58(%ebp,%eax), %eax
	movl	%ecx, %fs:0
	addl	$48, %esp
	popl	%esi
	popl	%edi
	popl	%ebx
	popl	%ebp
	retl
LBB0_1:
	movl	-24(%ebp), %esp
	addl	$12, %ebp
	jmp	LBB0_2

With MSVC compiler, every exception handlers in the same compilation unit are upgraded (with /GS).
With this approach, only functions with EH and GS are upgraded.

[rnk]

Test? You should be able to add an IR-only test that uses opt -winehprepare -stack-protector like test/CodeGen/WinEH/wineh-cloning.ll.

include/llvm/Analysis/EHPersonalities.h

74 ↗

(On Diff #59945)

Code in lib/Analysis isn't supposed to modify IR, but inserting a declaration into the module should be fine. Maybe a better name would be getStackGuardEHPersonality, though, since it doesn't actually modify the IR.

lib/Analysis/EHPersonalities.cpp

51 ↗

(On Diff #59945)

Since we will extend this soon, here's a cute thing you can do:

StringRef NewName = llvm::StringSwitch<StringRef>(F->getName())
              .Case("_except_handler3", "_except_handler4")
              .Case("__C_specific_handler", "__GSHandlerCheck_SEH")
              .Default("");
if (NewName.empty())
  return nullptr;
return M->getOrInsertFunction(NewName, ...);

[etienneb]

add test

[majnemer]

include/llvm/Analysis/EHPersonalities.h

72–74 ↗

(On Diff #60204)

I'd sink this into WinEHPrepare unless you expect to use it elsewhere.

lib/Analysis/EHPersonalities.cpp

44 ↗

(On Diff #60204)

Please change the return type to Value *.

lib/CodeGen/WinEHPrepare.cpp

681–683 ↗

(On Diff #60204)

Ditto.

683–684 ↗

(On Diff #60204)

You can Fold the assignment into the if.

testCodeGenWinEHwineh-promote-eh.ll

1 ↗

(On Diff #60204)

-mtriple=-mtriple= looks funky

[etienneb]

comments

[majnemer]

LGTM

lib/CodeGen/WinEHPrepare.cpp

107–110 ↗

(On Diff #60403)

I think it would make sense to sink this call into prepareExplicitEH.

[etienneb]

nits

[rnk]

lgtm

[thakis]

Did this ever land? If not, is it blocked on anything?

[etienneb]

In D21101#471334, @thakis wrote:

Did this ever land? If not, is it blocked on anything?

It's not blocked by anything.
It was the next thing to land on the Exception/GS issue.

We decided to land step by step the patches related to the feature to see when it will explode.

For now, this patch is a noop since we do not produce stack-guard + exception at the same time.

Should we land it now?

[rnk]

Let's go ahead and land this. Even though it's not functional, it documents our future plans in code.

[etienneb]

rebase (minor fixes)

[etienneb]

temporary reverted.

SEH4 needs to be fixed first.

[Diffusion] rL274251: revert http://reviews.llvm.org/D21101

llvm/lib/CodeGen/WinEHPrepare.cpp

 // personality requires catch handler code to be outlined.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringSwitch.h"
 #include "llvm/Analysis/CFG.h"
 #include "llvm/Analysis/EHPersonalities.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/WinEHFuncInfo.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Debug.h"
public:
 
   void getAnalysisUsage(AnalysisUsage &AU) const override;
 
   const char *getPassName() const override {
     return "Windows exception handling preparation";
   }
 
 private:
+  void promoteEHPersonality(Function &F);
   void insertPHIStores(PHINode *OriginalPHI, AllocaInst *SpillSlot);
   void
   insertPHIStore(BasicBlock *PredBlock, Value *PredVal, AllocaInst *SpillSlot,
                  SmallVectorImpl<std::pair<BasicBlock *, Value *>> &Worklist);
   AllocaInst *insertPHILoads(PHINode *PN, Function &F);
   void replaceUseWithLoad(Value *V, Use &U, AllocaInst *&SpillSlot,
                           DenseMap<BasicBlock *, Value *> &Loads, Function &F);
   bool prepareExplicitEH(Function &F);
static int addClrEHHandler(WinEHFuncInfo &FuncInfo, int HandlerParentState,
   Entry.TryParentState = TryParentState;
   Entry.Handler = Handler;
   Entry.HandlerType = HandlerType;
   Entry.TypeToken = TypeToken;
   FuncInfo.ClrEHUnwindMap.push_back(Entry);
   return FuncInfo.ClrEHUnwindMap.size() - 1;
 }
 
+static Value *getStackGuardEHPersonality(Value *Pers) {
+  Function *F =
+      Pers ? dyn_cast<Function>(Pers->stripPointerCasts()) : nullptr;
+  if (!F)
+    return nullptr;
+
+  // TODO(etienneb): Upgrade exception handlers when they are working.
+  StringRef NewName = llvm::StringSwitch<StringRef>(F->getName())  
+      .Case("_except_handler3", "_except_handler4")
+      .Default("");
+  if (NewName.empty())
+    return nullptr;
+
+  Module *M = F->getParent();
+  return M->getOrInsertFunction("_except_handler4", F->getFunctionType(),
+                                F->getAttributes());
+}
+
+void WinEHPrepare::promoteEHPersonality(Function &F) {
+  // Promote the exception handler when stack protection is activated.
+  if (!F.hasFnAttribute(Attribute::StackProtect) &&
+      !F.hasFnAttribute(Attribute::StackProtectReq) &&
+      !F.hasFnAttribute(Attribute::StackProtectStrong))
+    return;
+
+  if (Value *PersonalityFn = F.getPersonalityFn()) {
+    if (Value *Personality = getStackGuardEHPersonality(PersonalityFn)) {
+      Function* PromotedFn = cast<Function>(Personality);
+      F.setPersonalityFn(PromotedFn);
+    }
+  }
+}    
+
 void llvm::calculateClrEHStateNumbers(const Function *Fn,
                                       WinEHFuncInfo &FuncInfo) {
   // Return if it's already been done.
   if (!FuncInfo.EHPadStateMap.empty())
     return;
 
   // This numbering assigns one state number to each catchpad and cleanuppad.
   // It also computes two tree-like relations over states:
for (BasicBlock &BB : F) {
     if (NumColors > 1)
       report_fatal_error("Multicolor BB!");
     assert((DisableDemotion || !(BB.isEHPad() && isa<PHINode>(BB.begin()))) &&
            "EH Pad still has a PHI!");
   }
 }
 
 bool WinEHPrepare::prepareExplicitEH(Function &F) {
+  // When stack-protector is present, some exception handlers need to be
+  // promoted to a compatible handlers.
+  promoteEHPersonality(F);
+
   // Remove unreachable blocks.  It is not valuable to assign them a color and
   // their existence can trick us into thinking values are alive when they are
   // not.
   removeUnreachableBlocks(F);
 
   // Determine which blocks are reachable from which funclet entries.
   colorFunclets(F);
 

llvm/test/CodeGen/WinEH/wineh-promote-eh.ll

+; RUN: opt -mtriple=i686-windows-msvc -S -winehprepare %s | FileCheck %s
+
+declare i32 @_except_handler3(...)
+
+define void @test1a() personality i32 (...)* @_except_handler3 {
+; CHECK: define void @test1a() personality i32 (...)* @_except_handler3
+entry:
+  ret void
+}
+
+define void @test1b() ssp personality i32 (...)* @_except_handler3 {
+; CHECK: define void @test1b() [[attr:.*]] personality i32 (...)* @_except_handler4
+entry:
+  ret void
+}
+