[MSAN] Reintroduce libatomic load/store instrumentation

[guiand]

Summary

Have the front-end use the `nounwind` attribute on atomic libcalls.
This prevents us from seeing `invoke __atomic_load` in MSAN, which
is problematic as it has no successor for instrumentation to be added.

[guiand]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

3518

TODO: Remove

[eugenis]

__libatomic_load might come at the end of the function, with no succeeding BB

Not exactly. It may come at the end of a BB.

We should probably fix it in clang by adding nounwind to libatomic calls, and ignore invokes here.

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

3508

Check if the function already exists.

3510

addVisibility(GlobalValue::HiddenVisibility);

[guiand]

Simplified by returning to the old implementation, but having libatomic calls made nounwind (so we never see them as invokes).

[guiand]

Rebased on master

[guiand]

Rebased on master (again)

[eugenis]

clang/lib/CodeGen/CGAtomic.cpp

315

This needs a clang test, and better move it to a separate change.

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

3509

Better explicitly check that this an invoke and not call. Call always has a next instruction.

And move this check to the parent function to apply regular call instrumentation to the unrecognized call.

3511

It has a successor. Two successors, actually.
I'd rather say something like "invoke of __atomic_load".

[guiand]

Separated out the frontend change. Addressed other comments.

[guiand]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

3513

TODO: Remove this redudant call

[eugenis]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

3513

you've already checked it with assert(isa<CallInst>) - which could be changed to isTerminator(), and this could be simplified to NextIRB(CB.getNextNode()).

[guiand]

Addressed comments

[eugenis]

LGTM with 1 comment

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

3576

Probably need the same check for atomic_store.

[guiand]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

3576

Although it would be good for consistency I don't think it's strictly needed since with atomic_store we don't need to use getNextNode()

[eugenis]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

3576

ah, good point. LGTM

[guiand]

Committed as 97de0188dd5d845ff90c8ac779a2ea09688b17df

clang/lib/CodeGen/CGAtomic.cpp

 
 static RValue emitAtomicLibcall(CodeGenFunction &CGF,
                                 StringRef fnName,
                                 QualType resultType,
                                 CallArgList &args) {
   const CGFunctionInfo &fnInfo =
     CGF.CGM.getTypes().arrangeBuiltinFunctionCall(resultType, args);
   llvm::FunctionType *fnTy = CGF.CGM.getTypes().GetFunctionType(fnInfo);
-  llvm::FunctionCallee fn = CGF.CGM.CreateRuntimeFunction(fnTy, fnName);
+  llvm::AttributeList fnAttrs;
+  fnAttrs = fnAttrs.addAttribute(CGF.getLLVMContext(),
+                                 llvm::AttributeList::FunctionIndex,
+                                 llvm::Attribute::NoUnwind);
+  llvm::FunctionCallee fn =
+      CGF.CGM.CreateRuntimeFunction(fnTy, fnName, fnAttrs);
   auto callee = CGCallee::forDirect(fn);
   return CGF.EmitCall(fnInfo, callee, ReturnValueSlot(), args);
 }
 
 /// Does a store of the given IR type modify the full expected width?
 static bool isFullSizeType(CodeGenModule &CGM, llvm::Type *type,
                            uint64_t expectedSize) {
   return (CGM.getDataLayout().getTypeStoreSize(type) * 8 == expectedSize);

compiler-rt/test/msan/libatomic_load_exceptions.cpp

+// RUN: %clangxx_msan -fexceptions -fsanitize-memory-track-origins=2 -latomic -O0 %s -o %t && not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-SHADOW
+
+// PPC has no libatomic
+// UNSUPPORTED: powerpc64-target-arch
+// UNSUPPORTED: powerpc64le-target-arch
+
+#include <sanitizer/msan_interface.h>
+#include <stdatomic.h>
+
+typedef struct __attribute((packed)) {
+  uint8_t val[3];
+} i24;
+
+void copy(i24 *dst, i24 *src);
+
+int main() {
+  i24 uninit;
+  i24 init = {0};
+
+  __msan_check_mem_is_initialized(&init, 3);
+  copy(&init, &uninit);
+  __msan_check_mem_is_initialized(&init, 3);
+}
+
+void copy(i24 *dst, i24 *src) {
+  try {
+    __atomic_load(src, dst, __ATOMIC_RELAXED);
+  } catch (...) {
+  }
+}
+
+// CHECK: MemorySanitizer: use-of-uninitialized-value
+// CHECK: #0 {{0x[a-f0-9]+}} in main{{.*}}libatomic_load_exceptions.cpp:[[@LINE-10]]
+
+// CHECK-SHADOW: Uninitialized value was stored to memory at
+// CHECK-SHADOW: #0 {{0x[a-f0-9]+}} in copy{{.*}}libatomic_load_exceptions.cpp:[[@LINE-8]]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

void visitLibAtomicLoad(CallBase &CB) {
     Value *DstPtr = CB.getArgOperand(2);
     Value *Ordering = CB.getArgOperand(3);
     // Convert the call to have at least Acquire ordering to make sure
     // the shadow operations aren't reordered before it.
     Value *NewOrdering =
         IRB.CreateExtractElement(makeAddAcquireOrderingTable(IRB), Ordering);
     CB.setArgOperand(3, NewOrdering);
 
-    IRBuilder<> NextIRB(CB.getNextNode());
+    Instruction *InsPoint = CB.getNextNode();
+    if (!InsPoint) {
+      llvm::errs() << "MSAN -- cannot instrument libatomic call with no "
+                      "successor. Ignoring!\n";
+      return;
+    }
+    IRBuilder<> NextIRB(InsPoint);
     NextIRB.SetCurrentDebugLocation(CB.getDebugLoc());
 
     Value *SrcShadowPtr, *SrcOriginPtr;
     std::tie(SrcShadowPtr, SrcOriginPtr) =
         getShadowOriginPtr(SrcPtr, NextIRB, NextIRB.getInt8Ty(), Align(1),
                            /*isStore*/ false);
     Value *DstShadowPtr =
         getShadowOriginPtr(DstPtr, NextIRB, NextIRB.getInt8Ty(), Align(1),
                            /*isStore*/ true)
             .first;
 
     NextIRB.CreateMemCpy(DstShadowPtr, Align(1), SrcShadowPtr, Align(1), Size);
void visitCallBase(CallBase &CB) {
     }
     LibFunc LF;
     if (TLI->getLibFunc(CB, LF)) {
       // libatomic.a functions need to have special handling because there isn't
       // a good way to intercept them or compile the library with
       // instrumentation.
       switch (LF) {
       case LibFunc_atomic_load:
         visitLibAtomicLoad(CB);
         return;
       case LibFunc_atomic_store:
         visitLibAtomicStore(CB);
         return;
       default:
         break;
       }
     }