[ubsan] Diagnose noreturn functions which return

[vsk]

Summary

Diagnose 'unreachable' UB when a noreturn function returns.

1. Insert a check at the end of functions marked noreturn.
2. A decl may be marked noreturn in the caller TU, but not marked in the TU where it's defined. To diagnose this scenario, strip away the noreturn attribute and insert a check after the call.

Testing: check-clang, check-ubsan, check-ubsan-minimal, D40700

rdar://33660464

[vsk]

Ah, I've found a problem while writing run-time tests. I'll need to take another cut at this.

[vsk]

• Emit the check in the noreturn function, so that the UB does not result in the optimizer throwing away the diagnostic instrumentation.

[vsk]

• Leave out an unrelated change in the handling of NakedAttr.

[efriedma]

lib/CodeGen/CGCall.cpp

2761

Unfortunately, this won't catch cases where the caller has a noreturn marking, but the callee doesn't. Maybe strip the noreturn attribute from the caller, then put the sanitizer check into both the caller and callee?

2769

This is likely a problem. Not because the code is supposed to be reachable, but because putting additional code into a naked function can have weird effects. Probably not worth messing with this case.

[vsk]

lib/CodeGen/CGCall.cpp

2761

If the caller sees a noreturn marking on the call target, inserting a 'reached-unreachable' check after the call is UB, and the optimizer throws it away. Perhaps you had a different case in mind?

2769

Sorry for the noisy change. I recognized it as a mistake a second after I uploaded the new diff :(.

[efriedma]

lib/CodeGen/CGCall.cpp

2761

I'm thinking of the case where the caller can't see the callee, like this:

a.c:
void a(void) __attribute((noreturn));
void b(void) { a(); }

b.c:
void a(void) {}

[vsk]

lib/CodeGen/CGCall.cpp

2761

Got it. So clang would then do:

a.c:
void a(void) /* strip the noreturn attribute */;
void b(void) { a(); /* insert a reached-unreachable check here */}

[vsk]

• Diagnose in the scenario Eli pointed out, by stripping the 'noreturn' attribute and emitting a check after the call.
• Test updates.

[vsk]

• Make the patch cleaner by introducing an overload of EmitCall() which doesn't require a SourceLocation argument.

[vsk]

• Tighten the IR test case.

[efriedma]

LGTM

docs/UndefinedBehaviorSanitizer.rst

-  ``-fsanitize=shift``: Shift operators where the amount shifted is
      or less than zero, or where the left hand side is negative. For a
      signed left shift, also checks for signed overflow in C, and for
      unsigned overflow in C++. You can use ``-fsanitize=shift-base`` or
      ``-fsanitize=shift-exponent`` to check only left-hand side or
      right-hand side of shift operation, respectively.
   -  ``-fsanitize=signed-integer-overflow``: Signed integer overflow,
      including all the checks added by ``-ftrapv``, and checking for
      overflow in signed division (``INT_MIN / -1``).
-  -  ``-fsanitize=unreachable``: If control flow reaches
-     ``__builtin_unreachable``.
+  -  ``-fsanitize=unreachable``: If control flow reaches an unreachable
+     program point.
   -  ``-fsanitize=unsigned-integer-overflow``: Unsigned integer
      overflows. Note that unlike signed integer overflow, unsigned integer
      is not undefined behavior. However, while it has well-defined semantics,
      it is often unintentional, so UBSan offers to catch it.
   -  ``-fsanitize=vla-bound``: A variable-length array whose bound
      does not evaluate to a positive value.
   -  ``-fsanitize=vptr``: Use of an object whose vptr indicates that it is of
      the wrong dynamic type, or that its lifetime has not begun or has ended.

lib/CodeGen/CGBuiltin.cpp

case Builtin::BI__builtin___clear_cache: {
     Value *F = CGM.getIntrinsic(Intrinsic::clear_cache);
     return RValue::get(Builder.CreateCall(F, {Begin, End}));
   }
   case Builtin::BI__builtin_trap:
     return RValue::get(EmitTrapCall(Intrinsic::trap));
   case Builtin::BI__debugbreak:
     return RValue::get(EmitTrapCall(Intrinsic::debugtrap));
   case Builtin::BI__builtin_unreachable: {
-    if (SanOpts.has(SanitizerKind::Unreachable)) {
-      SanitizerScope SanScope(this);
-      EmitCheck(std::make_pair(static_cast<llvm::Value *>(Builder.getFalse()),
-                               SanitizerKind::Unreachable),
-                SanitizerHandler::BuiltinUnreachable,
-                EmitCheckSourceLocation(E->getExprLoc()), None);
-    } else
-      Builder.CreateUnreachable();
+    EmitUnreachable(E->getExprLoc());
 
     // We do need to preserve an insertion point.
     EmitBlock(createBasicBlock("unreachable.cont"));
 
     return RValue::get(nullptr);
   }
 
   case Builtin::BI__builtin_powi:

lib/CodeGen/CGCall.cpp

static llvm::StoreInst *findDominatingStoreToReturnValue(CodeGenFunction &CGF) {
   // Okay, the store's basic block dominates the insertion point; we
   // can do our thing.
   return store;
 }
 
 void CodeGenFunction::EmitFunctionEpilog(const CGFunctionInfo &FI,
                                          bool EmitRetDbgLoc,
                                          SourceLocation EndLoc) {
+  if (FI.isNoReturn()) {
+    // Noreturn functions don't return.
+    EmitUnreachable(EndLoc);
+    return;
+  }
+
   if (CurCodeDecl && CurCodeDecl->hasAttr<NakedAttr>()) {
     // Naked functions don't have epilogues.
     Builder.CreateUnreachable();
     return;
   }
 
   // Functions with no result always return void.
   if (!ReturnValue.isValid()) {
     Builder.CreateRetVoid();
     return;
   }
void CodeGenFunction::deferPlaceholderReplacement(llvm::Instruction *Old,
                                                   llvm::Value *New) {
   DeferredReplacements.push_back(std::make_pair(Old, New));
 }
 
 RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
                                  const CGCallee &Callee,
                                  ReturnValueSlot ReturnValue,
                                  const CallArgList &CallArgs,
-                                 llvm::Instruction **callOrInvoke) {
+                                 llvm::Instruction **callOrInvoke,
+                                 SourceLocation Loc) {
   // FIXME: We no longer need the types from CallArgs; lift up and simplify.
 
   assert(Callee.isOrdinary());
 
   // Handle struct-return functions by passing a pointer to the
   // location that we would like to return into.
   QualType RetTy = CallInfo.getReturnType();
   const ABIArgInfo &RetAI = CallInfo.getReturnInfo();
#endif
   // If the call doesn't return, finish the basic block and clear the
   // insertion point; this allows the rest of IRGen to discard
   // unreachable code.
   if (CS.doesNotReturn()) {
     if (UnusedReturnSize)
       EmitLifetimeEnd(llvm::ConstantInt::get(Int64Ty, UnusedReturnSize),
                       SRetPtr.getPointer());
 
-    Builder.CreateUnreachable();
+    // Strip away the noreturn attribute to better diagnose unreachable UB.
+    if (SanOpts.has(SanitizerKind::Unreachable)) {
+      if (auto *F = CS.getCalledFunction())
+        F->removeFnAttr(llvm::Attribute::NoReturn);
+      CS.removeAttribute(llvm::AttributeList::FunctionIndex,
+                         llvm::Attribute::NoReturn);
+    }
+
+    EmitUnreachable(Loc);
     Builder.ClearInsertionPoint();
 
     // FIXME: For now, emit a dummy basic block because expr emitters in
     // generally are not ready to handle emitting expressions at unreachable
     // points.
     EnsureInsertPoint();
 
     // Return a reasonable RValue.

lib/CodeGen/CGExpr.cpp

void CodeGenFunction::EmitCfiCheckFail() {
   }
 
   FinishFunction();
   // The only reference to this function will be created during LTO link.
   // Make sure it survives until then.
   CGM.addUsedGlobal(F);
 }
 
+void CodeGenFunction::EmitUnreachable(SourceLocation Loc) {
+  if (SanOpts.has(SanitizerKind::Unreachable)) {
+    SanitizerScope SanScope(this);
+    EmitCheck(std::make_pair(static_cast<llvm::Value *>(Builder.getFalse()),
+                             SanitizerKind::Unreachable),
+              SanitizerHandler::BuiltinUnreachable,
+              EmitCheckSourceLocation(Loc), None);
+  }
+  Builder.CreateUnreachable();
+}
+
 void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked) {
   llvm::BasicBlock *Cont = createBasicBlock("cont");
 
   // If we're optimizing, collapse all calls to trap down to just one per
   // function to save on code size.
   if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
     TrapBB = createBasicBlock("trap");
     Builder.CreateCondBr(Checked, Cont, TrapBB);
if (isa<FunctionNoProtoType>(FnType) || Chain) {
     llvm::Type *CalleeTy = getTypes().GetFunctionType(FnInfo);
     CalleeTy = CalleeTy->getPointerTo();
 
     llvm::Value *CalleePtr = Callee.getFunctionPointer();
     CalleePtr = Builder.CreateBitCast(CalleePtr, CalleeTy, "callee.knr.cast");
     Callee.setFunctionPointer(CalleePtr);
   }
 
-  return EmitCall(FnInfo, Callee, ReturnValue, Args);
+  return EmitCall(FnInfo, Callee, ReturnValue, Args, nullptr, E->getExprLoc());
 }
 
 LValue CodeGenFunction::
 EmitPointerToDataMemberBinaryExpr(const BinaryOperator *E) {
   Address BaseAddr = Address::invalid();
   if (E->getOpcode() == BO_PtrMemI) {
     BaseAddr = EmitPointerWithAlignment(E->getLHS());
   } else {

lib/CodeGen/CGExprCXX.cpp

RValue CodeGenFunction::EmitCXXMemberOrOperatorCall(
     llvm::Value *This, llvm::Value *ImplicitParam, QualType ImplicitParamTy,
     const CallExpr *CE, CallArgList *RtlArgs) {
   const FunctionProtoType *FPT = MD->getType()->castAs<FunctionProtoType>();
   CallArgList Args;
   MemberCallInfo CallInfo = commonEmitCXXMemberOrOperatorCall(
       *this, MD, This, ImplicitParam, ImplicitParamTy, CE, Args, RtlArgs);
   auto &FnInfo = CGM.getTypes().arrangeCXXMethodCall(
       Args, FPT, CallInfo.ReqArgs, CallInfo.PrefixSize);
-  return EmitCall(FnInfo, Callee, ReturnValue, Args);
+  return EmitCall(FnInfo, Callee, ReturnValue, Args, nullptr,
+                  CE ? CE->getExprLoc() : SourceLocation());
 }
 
 RValue CodeGenFunction::EmitCXXDestructorCall(
     const CXXDestructorDecl *DD, const CGCallee &Callee, llvm::Value *This,
     llvm::Value *ImplicitParam, QualType ImplicitParamTy, const CallExpr *CE,
     StructorType Type) {
   CallArgList Args;
   commonEmitCXXMemberOrOperatorCall(*this, DD, This, ImplicitParam,
CodeGenFunction::EmitCXXMemberPointerCallExpr(const CXXMemberCallExpr *E,
 
   RequiredArgs required =
       RequiredArgs::forPrototypePlus(FPT, 1, /*FD=*/nullptr);
 
   // And the rest of the call args
   EmitCallArgs(Args, FPT, E->arguments());
   return EmitCall(CGM.getTypes().arrangeCXXMethodCall(Args, FPT, required,
                                                       /*PrefixSize=*/0),
-                  Callee, ReturnValue, Args);
+                  Callee, ReturnValue, Args, nullptr, E->getExprLoc());
 }
 
 RValue
 CodeGenFunction::EmitCXXOperatorMemberCallExpr(const CXXOperatorCallExpr *E,
                                                const CXXMethodDecl *MD,
                                                ReturnValueSlot ReturnValue) {
   assert(MD->isInstance() &&
          "Trying to emit a member call expr on a static method!");

lib/CodeGen/CodeGenFunction.h

public:
   //                         Scalar Expression Emission
   //===--------------------------------------------------------------------===//
 
   /// EmitCall - Generate a call of the given function, expecting the given
   /// result type, and using the given argument list which specifies both the
   /// LLVM arguments and the types they were derived from.
   RValue EmitCall(const CGFunctionInfo &CallInfo, const CGCallee &Callee,
                   ReturnValueSlot ReturnValue, const CallArgList &Args,
-                  llvm::Instruction **callOrInvoke = nullptr);
-
+                  llvm::Instruction **callOrInvoke, SourceLocation Loc);
+  RValue EmitCall(const CGFunctionInfo &CallInfo, const CGCallee &Callee,
+                  ReturnValueSlot ReturnValue, const CallArgList &Args,
+                  llvm::Instruction **callOrInvoke = nullptr) {
+    return EmitCall(CallInfo, Callee, ReturnValue, Args, callOrInvoke,
+                    SourceLocation());
+  }
   RValue EmitCall(QualType FnType, const CGCallee &Callee, const CallExpr *E,
-                  ReturnValueSlot ReturnValue,
-                  llvm::Value *Chain = nullptr);
+                  ReturnValueSlot ReturnValue, llvm::Value *Chain = nullptr);
   RValue EmitCallExpr(const CallExpr *E,
                       ReturnValueSlot ReturnValue = ReturnValueSlot());
   RValue EmitSimpleCallExpr(const CallExpr *E, ReturnValueSlot ReturnValue);
   CGCallee EmitCallee(const Expr *E);
 
   void checkTargetFeatures(const CallExpr *E, const FunctionDecl *TargetDecl);
 
   llvm::CallInst *EmitRuntimeCall(llvm::Value *callee,
void EmitCheck(ArrayRef<std::pair<llvm::Value *, SanitizerMask>> Checked,
                  ArrayRef<llvm::Value *> DynamicArgs);
 
   /// \brief Emit a slow path cross-DSO CFI check which calls __cfi_slowpath
   /// if Cond if false.
   void EmitCfiSlowPathCheck(SanitizerMask Kind, llvm::Value *Cond,
                             llvm::ConstantInt *TypeId, llvm::Value *Ptr,
                             ArrayRef<llvm::Constant *> StaticArgs);
 
+  /// Emit a reached-unreachable diagnostic if \p Loc is valid and runtime
+  /// checking is enabled. Otherwise, just emit an unreachable instruction.
+  void EmitUnreachable(SourceLocation Loc);
+
   /// \brief Create a basic block that will call the trap intrinsic, and emit a
   /// conditional branch to it, for the -ftrapv checks.
   void EmitTrapCheck(llvm::Value *Checked);
 
   /// \brief Emit a call to trap or debugtrap and attach function attribute
   /// "trap-func-name" if specified.
   llvm::CallInst *EmitTrapCall(llvm::Intrinsic::ID IntrID);
 

test/CodeGen/ubsan-noreturn.c

+// RUN: %clang_cc1 %s -emit-llvm -fsanitize=unreachable -o - | FileCheck %s
+
+// CHECK-LABEL: @f(
+void __attribute__((noreturn)) f() {
+  // CHECK: __ubsan_handle_builtin_unreachable
+  // CHECK: unreachable
+}

test/CodeGenCXX/ubsan-unreachable.cpp

+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=unreachable | FileCheck %s
+
+extern void __attribute__((noreturn)) abort();
+
+// CHECK-LABEL: define void @_Z14calls_noreturnv
+void calls_noreturn() {
+  abort();
+
+  // Check that there are no attributes on the call site.
+  // CHECK-NOT: call void @_Z5abortv{{.*}}#
+
+  // CHECK: __ubsan_handle_builtin_unreachable
+  // CHECK: unreachable
+}
+
+struct A {
+  // CHECK: declare void @_Z5abortv{{.*}} [[ABORT_ATTR:#[0-9]+]]
+
+  // CHECK-LABEL: define linkonce_odr void @_ZN1A5call1Ev
+  void call1() {
+    // CHECK-NOT: call void @_ZN1A16does_not_return2Ev{{.*}}#
+    does_not_return2();
+
+    // CHECK: __ubsan_handle_builtin_unreachable
+    // CHECK: unreachable
+  }
+
+  // Test static members.
+  static void __attribute__((noreturn)) does_not_return1() {
+    // CHECK-NOT: call void @_Z5abortv{{.*}}#
+    abort();
+  }
+
+  // CHECK-LABEL: define linkonce_odr void @_ZN1A5call2Ev
+  void call2() {
+    // CHECK-NOT: call void @_ZN1A16does_not_return1Ev{{.*}}#
+    does_not_return1();
+
+    // CHECK: __ubsan_handle_builtin_unreachable
+    // CHECK: unreachable
+  }
+
+  // Test calls through pointers to non-static member functions.
+  typedef void __attribute__((noreturn)) (A::*MemFn)();
+
+  // CHECK-LABEL: define linkonce_odr void @_ZN1A5call3Ev
+  void call3() {
+    MemFn MF = &A::does_not_return2;
+    (this->*MF)();
+
+    // CHECK-NOT: call void %{{.*}}#
+    // CHECK: __ubsan_handle_builtin_unreachable
+    // CHECK: unreachable
+  }
+
+  // Test regular members.
+  // CHECK-LABEL: define linkonce_odr void @_ZN1A16does_not_return2Ev({{.*}})
+  // CHECK-SAME: [[DOES_NOT_RETURN_ATTR:#[0-9]+]]
+  void __attribute__((noreturn)) does_not_return2() {
+    // CHECK-NOT: call void @_Z5abortv(){{.*}}#
+    abort();
+
+    // CHECK: call void @__ubsan_handle_builtin_unreachable
+    // CHECK: unreachable
+
+    // CHECK: call void @__ubsan_handle_builtin_unreachable
+    // CHECK: unreachable
+  }
+};
+
+// CHECK: define linkonce_odr void @_ZN1A16does_not_return1Ev() [[DOES_NOT_RETURN_ATTR]]
+
+void force_irgen() {
+  A a;
+  a.call1();
+  a.call2();
+  a.call3();
+}
+
+// CHECK-NOT: [[ABORT_ATTR]] = {{[^}]+}}noreturn
+// CHECK-NOT: [[DOES_NOT_RETURN_ATTR]] = {{[^}]+}}noreturn