[StackProtector] attribute __stack_chk_fail as NoReturn

[nickdesaulniers]

Summary

When GCC added support for stack smashing protections, it was defined
that:

This hook returns a CALL_EXPR that alerts the runtime that the stack
protect guard variable has been modified. This expression should
involve a call to a noreturn function.
The default version of this hook invokes a function called
‘__stack_chk_fail’, taking no arguments.

Do so as well for __stack_smash_handler for OpenBSD.

Every libc implementation I could find has __stack_chk_fail marked
noreturn, or the implementation calls abort, exit, or panic (which
themselves are noreturn).

Glibc: https://sourceware.org/git/?p=glibc.git;a=blob;f=debug/stack_chk_fail.c
Musl: https://git.musl-libc.org/cgit/musl/tree/src/env/__stack_chk_fail.c
Bionic: https://android.googlesource.com/platform/bionic/+/refs/heads/master/libc/bionic/__stack_chk_fail.cpp
FreeBSD: https://cgit.freebsd.org/src/tree/lib/libc/secure/stack_protector.c
OpenBSD: https://github.com/openbsd/src/blob/master/lib/libc/sys/stack_protector.c
NetBSD: https://github.com/NetBSD/src/blob/trunk/lib/libc/misc/stack_protector.c
Linux Kernel: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/panic.c
Apple: https://opensource.apple.com/source/Libc/Libc-1439.40.11/sys/OpenBSD/stack_protector.c.auto.html

Link: https://gcc.gnu.org/onlinedocs/gccint/Stack-Smashing-Protection.html#Stack-Smashing-Protection

This will later help us diagnose functions that fall through to other
functions vs end in calls to functions that are noreturn.

[nickdesaulniers]

• refactor

[nickdesaulniers]

• set SmallVector size to 1

[efriedma]

LGTM

[nickdesaulniers]

llvm/lib/CodeGen/StackProtector.cpp

637

@efriedma I wonder if we can avoid this unreachable? (maybe as a follow up commit to this)

I had tried it and a bunch of stack check related tests crashed; I didn't investigate, but suspect maybe some other code expects there to be an instruction after the call.

Or can you think of any reason why an unreachable needs to be added here? Is there something implicit about noreturn functions needing to be followed by unreachable in IR?

(even if the child patches don't land, D146339 alone is still probably worthwhile to land)

[efriedma]

llvm/lib/CodeGen/StackProtector.cpp

637

Every basic block must end in a terminator, and call isn't a terminator, so we have to put something after the call. "unreachable" is the terminator we use when we know control can't actually flow anywhere else in the function. Not sure what you'd want to replace it with.

Yes, this patch makes sense even without the other changes.

llvm/lib/CodeGen/StackProtector.cpp

 // with a random value in it is stored onto the stack before the local variables
 // are allocated. Upon exiting the block, the stored value is checked. If it's
 // changed, then there was some sort of violation and the program aborts.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/StackProtector.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/Analysis/MemoryLocation.h"
 #include "llvm/Analysis/OptimizationRemarkEmitter.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/TargetLowering.h"
 #include "llvm/CodeGen/TargetPassConfig.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"

 /// check fails.
 BasicBlock *StackProtector::CreateFailBB() {
   LLVMContext &Context = F->getContext();
   BasicBlock *FailBB = BasicBlock::Create(Context, "CallStackCheckFailBlk", F);
   IRBuilder<> B(FailBB);
   if (F->getSubprogram())
     B.SetCurrentDebugLocation(
         DILocation::get(Context, 0, 0, F->getSubprogram()));
+  FunctionCallee StackChkFail;
+  SmallVector<Value *, 1> Args;
   if (Trip.isOSOpenBSD()) {
-    FunctionCallee StackChkFail = M->getOrInsertFunction(
-        "__stack_smash_handler", Type::getVoidTy(Context),
+    StackChkFail = M->getOrInsertFunction("__stack_smash_handler",
+                                          Type::getVoidTy(Context),
                                           Type::getInt8PtrTy(Context));
-
-    B.CreateCall(StackChkFail, B.CreateGlobalStringPtr(F->getName(), "SSH"));
+    Args.push_back(B.CreateGlobalStringPtr(F->getName(), "SSH"));
   } else {
-    FunctionCallee StackChkFail =
+    StackChkFail =
         M->getOrInsertFunction("__stack_chk_fail", Type::getVoidTy(Context));
-
-    B.CreateCall(StackChkFail, {});
   }
+  cast<Function>(StackChkFail.getCallee())->addFnAttr(Attribute::NoReturn);
+  B.CreateCall(StackChkFail, Args);
   B.CreateUnreachable();
   return FailBB;
 }
 
 bool StackProtector::shouldEmitSDCheck(const BasicBlock &BB) const {
   return HasPrologue && !HasIRCheck && isa<ReturnInst>(BB.getTerminator());
 }
 
 void StackProtector::copyToMachineFrameInfo(MachineFrameInfo &MFI) const {

llvm/test/CodeGen/X86/2009-04-14-IllegalRegs.ll

 ; CHECK-NEXT:  ## %bb.2: ## %SP_return
 ; CHECK-NEXT:    movl {{[-0-9]+}}(%e{{[sb]}}p), %eax ## 4-byte Reload
 ; CHECK-NEXT:    addl $148, %esp
 ; CHECK-NEXT:    popl %esi
 ; CHECK-NEXT:    popl %edi
 ; CHECK-NEXT:    retl
 ; CHECK-NEXT:  LBB0_3: ## %CallStackCheckFailBlk
 ; CHECK-NEXT:    calll ___stack_chk_fail
-; CHECK-NEXT:    ud2
 entry:
   %retval = alloca i32    ; <ptr> [#uses=2]
   %xxx = alloca %struct.X   ; <ptr> [#uses=6]
   %0 = alloca i32   ; <ptr> [#uses=2]
   %"alloca point" = bitcast i32 0 to i32    ; <i32> [#uses=0]
   %1 = getelementptr %struct.X, ptr %xxx, i32 0, i32 1    ; <ptr> [#uses=1]
   %2 = getelementptr [32 x i8], ptr %1, i32 0, i32 31    ; <ptr> [#uses=1]
   store i8 48, ptr %2, align 1

llvm/test/CodeGen/X86/stack-protector-weight.ll

 ; RUN: llc -mtriple=x86_64-apple-darwin -print-after=finalize-isel -enable-selectiondag-sp=true %s -o /dev/null 2>&1 | FileCheck %s --check-prefix=DARWIN-SELDAG
 ; RUN: llc -mtriple=x86_64-apple-darwin -print-after=finalize-isel -enable-selectiondag-sp=false %s -o /dev/null 2>&1 | FileCheck %s --check-prefix=DARWIN-IR
 ; RUN: llc -mtriple=i386-pc-windows-msvc -print-after=finalize-isel -enable-selectiondag-sp=true %s -o /dev/null 2>&1 | FileCheck %s -check-prefix=MSVC-SELDAG
 ; RUN: llc -mtriple=i386-pc-windows-msvc -print-after=finalize-isel -enable-selectiondag-sp=false %s -o /dev/null 2>&1 | FileCheck %s -check-prefix=MSVC-IR
 
 ; DARWIN-SELDAG: # Machine code for function test_branch_weights:
 ; DARWIN-SELDAG: successors: %bb.[[SUCCESS:[0-9]+]](0x7ffff800), %bb.[[FAILURE:[0-9]+]]
 ; DARWIN-SELDAG: bb.[[FAILURE]]{{[0-9a-zA-Z_.]+}}:
 ; DARWIN-SELDAG: CALL64pcrel32 &__stack_chk_fail
 ; DARWIN-SELDAG: bb.[[SUCCESS]]{{[0-9a-zA-Z_.]+}}:
 
 ; DARWIN-IR: # Machine code for function test_branch_weights:
-; DARWIN-IR: successors: %bb.[[SUCCESS:[0-9]+]](0x7fffffff), %bb.[[FAILURE:[0-9]+]]
+; DARWIN-IR: successors: %bb.[[SUCCESS:[0-9]+]](0x7ffff800), %bb.[[FAILURE:[0-9]+]]
 ; DARWIN-IR: bb.[[SUCCESS]]{{[0-9a-zA-Z_.]+}}:
 ; DARWIN-IR: bb.[[FAILURE]]{{[0-9a-zA-Z_.]+}}:
 ; DARWIN-IR: CALL64pcrel32 @__stack_chk_fail
 
 ; MSVC-SELDAG: # Machine code for function test_branch_weights:
 ; MSVC-SELDAG: :: (volatile load (s32) from @__security_cookie)
 ; MSVC-SELDAG: (store (s32) into stack)
 ; MSVC-SELDAG: (volatile load (s32) from %stack.0.StackGuardSlot)