[TSan] Don't instrument inout accesses on known-empty types

Empty types (such as structs without stored properties) have a
meaningless value for their address. To avoid crashes in the Thread
Sanitizer runtime, rather than passing this unspecified value as
the address of the inout access, skip emission of the runtime call.

The bug allowing unspecified behavior here has been present since we
first added TSan support for checking Swift access races -- but codegen
changes on arm64 have recently made crashes due to the bug much more
likely.

rdar://problem/47686212

Author: devincoughlin

lib/IRGen/GenBuiltin.cpp

@@ -120,6 +120,7 @@ getLoweredTypeAndTypeInfo(IRGenModule &IGM, Type unloweredType) {
 /// emitBuiltinCall - Emit a call to a builtin function.
 void irgen::emitBuiltinCall(IRGenFunction &IGF, const BuiltinInfo &Builtin,
                             Identifier FnId, SILType resultType,
+                            ArrayRef<SILType> argTypes,
                             Explosion &args, Explosion &out,
                             SubstitutionMap substitutions) {
   if (Builtin.ID == BuiltinValueKind::COWBufferForReading) {
@@ -1074,7 +1075,18 @@ if (Builtin.ID == BuiltinValueKind::id) { \
 
   if (Builtin.ID == BuiltinValueKind::TSanInoutAccess) {
     auto address = args.claimNext();
-    IGF.emitTSanInoutAccessCall(address);
+
+    // The tsanInoutAccess builtin takes a single argument, the address
+    // of the accessed storage
+    SILType accessedType = argTypes[0];
+
+    // Empty types (such as structs without stored properties) have a
+    // meaningless value for their address. We not should call into the
+    // TSan runtime to check for data races on accesses on such addresses.
+    if (!IGF.IGM.getTypeInfo(accessedType)
+        .isKnownEmpty(ResilienceExpansion::Maximal)) {
+      IGF.emitTSanInoutAccessCall(address);
+    }
     return;
   }
 

lib/IRGen/GenBuiltin.h

@@ -33,6 +33,7 @@ namespace irgen {
   /// Emit a call to a builtin function.
   void emitBuiltinCall(IRGenFunction &IGF, const BuiltinInfo &builtin,
                        Identifier fnId, SILType resultType,
+                       ArrayRef<SILType> argTypes,
                        Explosion &args, Explosion &result,
                        SubstitutionMap substitutions);
 

lib/IRGen/IRGenSIL.cpp

@@ -2682,18 +2682,21 @@ void IRGenSILFunction::visitBuiltinInst(swift::BuiltinInst *i) {
 
   auto argValues = i->getArguments();
   Explosion args;
+  SmallVector<SILType, 4> argTypes;
 
   for (auto idx : indices(argValues)) {
     auto argValue = argValues[idx];
 
     // Builtin arguments should never be substituted, so use the value's type
     // as the parameter type.
     emitApplyArgument(*this, argValue, argValue->getType(), args);
+
+    argTypes.push_back(argValue->getType());
   }
 
   Explosion result;
   emitBuiltinCall(*this, builtin, i->getName(), i->getType(),
-                  args, result, i->getSubstitutions());
+                  argTypes, args, result, i->getSubstitutions());
 
   setLoweredExplosion(i, result);
 }

test/SILGen/tsan_instrumentation.swift

@@ -1,5 +1,6 @@
 // REQUIRES: tsan_runtime
 // RUN: %target-swift-emit-silgen -sanitize=thread %s | %FileCheck %s
+// RUN: %target-swift-frontend -sanitize=thread -emit-ir -primary-file %s | %FileCheck --check-prefix=CHECK-LLVM-IR %s
 
 // TSan is only supported on 64 bit.
 // REQUIRES: PTRSIZE=64
@@ -63,3 +64,53 @@ func inoutGlobalClassStoredProperty() {
   // temporary buffer passed to takesInout().
   takesInout(&gClass.storedProperty)
 }
+
+// Known-empty types don't have storage, so there is no address
+// to pass to the TSan runtime to check for data races on inout accesses.
+// In this case, the instrumentation should skip the call to
+// __tsan_external_write()
+
+struct ZeroSizedStruct {
+  mutating
+  func mutate() { }
+}
+
+struct NonEmptyStruct {
+  var f: Int = 5
+  mutating
+  func mutate() { }
+}
+
+// CHECK-LLVM-IR-LABEL: testNoInstrumentZeroSizedStruct
+func testNoInstrumentZeroSizedStruct() {
+  var s = ZeroSizedStruct()
+
+// CHECK-LLVM-IR-NOT: tsan_external_write
+  s.mutate()
+}
+
+func takesInout<T>(_ p: inout T) { }
+
+// CHECK-LLVM-IR-LABEL: testNoInstrumentEmptyTuple
+func testNoInstrumentEmptyTuple() {
+  var t: Void = ()
+
+// CHECK-LLVM-IR-NOT: tsan_external_write
+  takesInout(&t)
+}
+
+// CHECK-LLVM-IR-LABEL: testNoInstrumentMutateInoutZeroSizedStruct
+func testNoInstrumentMutateInoutZeroSizedStruct(p: inout ZeroSizedStruct) {
+
+// CHECK-LLVM-IR-NOT: tsan_external_write
+  p.mutate()
+}
+
+// CHECK-LLVM-IR-LABEL: testInstrumentNonEmptyStruct
+func testInstrumentNonEmptyStruct() {
+  // Make sure we actually instrument accesses to non-empty structs.
+  var s = NonEmptyStruct()
+
+// CHECK-LLVM-IR: tsan_external_write
+  s.mutate()
+}