[ubsan] Improve diagnostics for return value checks (compiler-rt)

[vsk]

Summary

Clang part: https://reviews.llvm.org/D34299

[arphaman]

lib/ubsan/ubsan_interface.inc

31

Just to confirm: We don't care about backwards compatibility, and the old version can be dropped safely?

[filcab]

lib/ubsan/ubsan_handlers.cc

479

This looks fishy. Can you add a comment to explain why this might happen?

lib/ubsan/ubsan_interface.inc

31

The usual reasoning is that we, in open source, always want to have matching clang+compiler-rt. But we don't want mysterious problems when mismatches happen, so I introduced the _v* mechanism to force the linker to "yell loudly" when there's a mismatch.

In the past, the first change to check data I remember (after the check has been in an llvm release) was made by me and we managed to come up with a heuristic to avoid revving the checks. The second change introduced the _v* mechanism (and also removed the older symbol).

test/ubsan/TestCases/Misc/nonnull.cpp

15

Shouldn't you do a similar transformation for the nullability test?

[vsk]

lib/ubsan/ubsan_interface.inc

31

@arphaman Yes, we're intentionally dropping backwards compatibility here. Xcode's build system knows to package up sanitizer runtimes into app bundles, so back/future-deployment isn't a concern. That is also not the common/supported use case -- runtime tools are generally meant for testing.

test/ubsan/TestCases/Misc/nonnull.cpp

15

I did update the nullability.c test. If you mean that we should have a function that looks like "bar", but with a _Nonnull return value annotation, I can add that in.

[filcab]

test/ubsan/TestCases/Misc/nonnull.cpp

15

yeah, I guess there's no need for an additional one here, since we can tell it's emitting at the new place (expression for return), and you should already have the call emission covered in the clang part.

[vsk]

Make the null check in the non-null return value handler an unexpected case.

lib/ubsan/ubsan_handlers.h

struct FunctionTypeMismatchData {
   const TypeDescriptor &Type;
 };
 
 RECOVERABLE(function_type_mismatch,
             FunctionTypeMismatchData *Data,
             ValueHandle Val)
 
 struct NonNullReturnData {
-  SourceLocation Loc;
   SourceLocation AttrLoc;
 };
 
 /// \brief Handle returning null from function with the returns_nonnull
 /// attribute, or a return type annotated with _Nonnull.
-RECOVERABLE(nonnull_return, NonNullReturnData *Data)
-RECOVERABLE(nullability_return, NonNullReturnData *Data)
+RECOVERABLE(nonnull_return_v1, NonNullReturnData *Data, SourceLocation *Loc)
+RECOVERABLE(nullability_return_v1, NonNullReturnData *Data, SourceLocation *Loc)
 
 struct NonNullArgData {
   SourceLocation Loc;
   SourceLocation AttrLoc;
   int ArgIndex;
 };
 
 /// \brief Handle passing null pointer to a function parameter with the nonnull

lib/ubsan/ubsan_handlers.cc

 
 void __ubsan::__ubsan_handle_function_type_mismatch_abort(
     FunctionTypeMismatchData *Data, ValueHandle Function) {
   GET_REPORT_OPTIONS(true);
   handleFunctionTypeMismatch(Data, Function, Opts);
   Die();
 }
 
-static void handleNonNullReturn(NonNullReturnData *Data, ReportOptions Opts,
-                                bool IsAttr) {
-  SourceLocation Loc = Data->Loc.acquire();
+static void handleNonNullReturn(NonNullReturnData *Data, SourceLocation *LocPtr,
+                                ReportOptions Opts, bool IsAttr) {
+  if (!LocPtr)
+    return;
+
+  SourceLocation Loc = LocPtr->acquire();
   ErrorType ET = ErrorType::InvalidNullReturn;
 
   if (ignoreReport(Loc, Opts, ET))
     return;
 
   ScopedReport R(Opts, Loc, ET);
 
   Diag(Loc, DL_Error, "null pointer returned from function declared to never "
                       "return null");
   if (!Data->AttrLoc.isInvalid())
     Diag(Data->AttrLoc, DL_Note, "%0 specified here")
         << (IsAttr ? "returns_nonnull attribute"
                    : "_Nonnull return type annotation");
 }
 
-void __ubsan::__ubsan_handle_nonnull_return(NonNullReturnData *Data) {
+void __ubsan::__ubsan_handle_nonnull_return_v1(NonNullReturnData *Data,
+                                               SourceLocation *LocPtr) {
   GET_REPORT_OPTIONS(false);
-  handleNonNullReturn(Data, Opts, true);
+  handleNonNullReturn(Data, LocPtr, Opts, true);
 }
 
-void __ubsan::__ubsan_handle_nonnull_return_abort(NonNullReturnData *Data) {
+void __ubsan::__ubsan_handle_nonnull_return_v1_abort(NonNullReturnData *Data,
+                                                     SourceLocation *LocPtr) {
   GET_REPORT_OPTIONS(true);
-  handleNonNullReturn(Data, Opts, true);
+  handleNonNullReturn(Data, LocPtr, Opts, true);
   Die();
 }
 
-void __ubsan::__ubsan_handle_nullability_return(NonNullReturnData *Data) {
+void __ubsan::__ubsan_handle_nullability_return_v1(NonNullReturnData *Data,
+                                                   SourceLocation *LocPtr) {
   GET_REPORT_OPTIONS(false);
-  handleNonNullReturn(Data, Opts, false);
+  handleNonNullReturn(Data, LocPtr, Opts, false);
 }
 
-void __ubsan::__ubsan_handle_nullability_return_abort(NonNullReturnData *Data) {
+void __ubsan::__ubsan_handle_nullability_return_v1_abort(
+    NonNullReturnData *Data, SourceLocation *LocPtr) {
   GET_REPORT_OPTIONS(true);
-  handleNonNullReturn(Data, Opts, false);
+  handleNonNullReturn(Data, LocPtr, Opts, false);
   Die();
 }
 
 static void handleNonNullArg(NonNullArgData *Data, ReportOptions Opts,
                              bool IsAttr) {
   SourceLocation Loc = Data->Loc.acquire();
   ErrorType ET = ErrorType::InvalidNullArgument;
 

lib/ubsan/ubsan_interface.inc

 INTERFACE_FUNCTION(__ubsan_handle_load_invalid_value_abort)
 INTERFACE_FUNCTION(__ubsan_handle_missing_return)
 INTERFACE_FUNCTION(__ubsan_handle_mul_overflow)
 INTERFACE_FUNCTION(__ubsan_handle_mul_overflow_abort)
 INTERFACE_FUNCTION(__ubsan_handle_negate_overflow)
 INTERFACE_FUNCTION(__ubsan_handle_negate_overflow_abort)
 INTERFACE_FUNCTION(__ubsan_handle_nonnull_arg)
 INTERFACE_FUNCTION(__ubsan_handle_nonnull_arg_abort)
-INTERFACE_FUNCTION(__ubsan_handle_nonnull_return)
-INTERFACE_FUNCTION(__ubsan_handle_nonnull_return_abort)
+INTERFACE_FUNCTION(__ubsan_handle_nonnull_return_v1)
+INTERFACE_FUNCTION(__ubsan_handle_nonnull_return_v1_abort)
 INTERFACE_FUNCTION(__ubsan_handle_nullability_arg)
 INTERFACE_FUNCTION(__ubsan_handle_nullability_arg_abort)
-INTERFACE_FUNCTION(__ubsan_handle_nullability_return)
-INTERFACE_FUNCTION(__ubsan_handle_nullability_return_abort)
+INTERFACE_FUNCTION(__ubsan_handle_nullability_return_v1)
+INTERFACE_FUNCTION(__ubsan_handle_nullability_return_v1_abort)
 INTERFACE_FUNCTION(__ubsan_handle_out_of_bounds)
 INTERFACE_FUNCTION(__ubsan_handle_out_of_bounds_abort)
 INTERFACE_FUNCTION(__ubsan_handle_pointer_overflow)
 INTERFACE_FUNCTION(__ubsan_handle_pointer_overflow_abort)
 INTERFACE_FUNCTION(__ubsan_handle_shift_out_of_bounds)
 INTERFACE_FUNCTION(__ubsan_handle_shift_out_of_bounds_abort)
 INTERFACE_FUNCTION(__ubsan_handle_sub_overflow)
 INTERFACE_FUNCTION(__ubsan_handle_sub_overflow_abort)
 INTERFACE_FUNCTION(__ubsan_handle_type_mismatch_v1)
 INTERFACE_FUNCTION(__ubsan_handle_type_mismatch_v1_abort)
 INTERFACE_FUNCTION(__ubsan_handle_vla_bound_not_positive)
 INTERFACE_FUNCTION(__ubsan_handle_vla_bound_not_positive_abort)
 INTERFACE_WEAK_FUNCTION(__ubsan_default_options)

test/ubsan/TestCases/Misc/nonnull.cpp

 // RUN: %clangxx -fsanitize=returns-nonnull-attribute %s -O3 -o %t
-// RUN: %run %t foo
+// RUN: %run %t foo 2>&1 | count 0
 // RUN: %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=returns-nonnull-attribute -fno-sanitize-recover=returns-nonnull-attribute %s -O3 -o %t.abort
+// RUN: not %run %t.abort &> /dev/null
 
 __attribute__((returns_nonnull)) char *foo(char *a);
 
 char *foo(char *a) {
+  // CHECK: nonnull.cpp:[[@LINE+2]]:3: runtime error: null pointer returned from function declared to never return null
+  // CHECK-NEXT: nonnull.cpp:[[@LINE-4]]:16: note: returns_nonnull attribute specified here
   return a;
-  // CHECK: nonnull.cpp:[[@LINE+2]]:1: runtime error: null pointer returned from function declared to never return null
-  // CHECK-NEXT: nonnull.cpp:[[@LINE-5]]:16: note: returns_nonnull attribute specified here
+}
+
+__attribute__((returns_nonnull)) char *bar(int x, char *a) {
+  if (x > 10) {
+    // CHECK: nonnull.cpp:[[@LINE+2]]:5: runtime error: null pointer returned from function declared to never return null
+    // CHECK-NEXT: nonnull.cpp:[[@LINE-3]]:16: note: returns_nonnull attribute specified here
+    return a;
+  } else {
+    // CHECK: nonnull.cpp:[[@LINE+2]]:5: runtime error: null pointer returned from function declared to never return null
+    // CHECK-NEXT: nonnull.cpp:[[@LINE-7]]:16: note: returns_nonnull attribute specified here
+    return a;
+  }
 }
 
 int main(int argc, char **argv) {
-  return foo(argv[1]) == 0;
+  char *a = argv[1];
+
+  foo(a);
+
+  bar(20, a);
+
+  // We expect to see a runtime error the first time we cover the "else"...
+  bar(5, a);
+
+  // ... but not a second time.
+  // CHECK-NOT: runtime error
+  bar(5, a);
+
+  return 0;
 }

test/ubsan/TestCases/Misc/nullability.c

 // RUN: %clang -w -fsanitize=nullability-arg,nullability-assign,nullability-return %s -O3 -o %t
 // RUN: %run %t foo 2>&1 | count 0
 // RUN: %run %t 2>&1 | FileCheck %s
 
-// CHECK: nullability.c:[[@LINE+2]]:51: runtime error: null pointer returned from function declared to never return null
+// CHECK: nullability.c:[[@LINE+2]]:41: runtime error: null pointer returned from function declared to never return null
 // CHECK-NEXT: nullability.c:[[@LINE+1]]:6: note: _Nonnull return type annotation specified here
 int *_Nonnull nonnull_retval1(int *p) { return p; }
 
 // CHECK: nullability.c:1001:22: runtime error: null pointer passed as argument 2, which is declared to never be null
 // CHECK-NEXT: nullability.c:[[@LINE+1]]:56: note: _Nonnull type annotation specified here
 int *_Nonnull nonnull_retval2(int *_Nonnull arg1, int *_Nonnull arg2,
                               int *_Nullable arg3, int *arg4, int arg5, ...) {
   return arg1;