[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

2756

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?

2764

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

2756

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?

2764

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

[efriedma]

lib/CodeGen/CGCall.cpp

2756

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

2756

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

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

lib/CodeGen/CodeGenFunction.h

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