This is an archive of the discontinued LLVM Phabricator instance.

[MSAN] Instrument libatomic load/store calls
ClosedPublic

Authored by guiand on Jul 7 2020, 12:36 PM.

Details

Summary

These calls are neither intercepted by compiler-rt nor is libatomic.a
naturally instrumented.

This patch uses the existing libcall mechanism to detect a call
to atomic_load or atomic_store, and instruments them much like
the preexisting instrumentation for atomics.

Calls to _load are modified to have at least Acquire ordering, and
calls to _store at least Release ordering. Because this needs to be
converted at runtime, msan injects a LUT (implemented as a vector
with extractelement).

Diff Detail

Event Timeline

guiand created this revision.Jul 7 2020, 12:36 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 7 2020, 12:36 PM
Herald added subscribers: llvm-commits, Restricted Project, jfb, hiraditya. · View Herald Transcript
guiand marked 3 inline comments as done.Jul 7 2020, 12:49 PM
guiand added inline comments.
compiler-rt/test/msan/libatomic.c
38

One thing that turned out a little strange is that because I have to insert instructions *after* the atomic load, including the origin update, the msan reporter decides that the origin is one line below the call. Is there anything I can do about this?

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
1872

Is there a more elegant way to do this?

3544

Is it valid to assume any alignment other than 1 here? I can't think of a way, but I'd like to make sure.

guiand marked 3 inline comments as not done.Jul 7 2020, 12:49 PM
guiand updated this revision to Diff 276186.Jul 7 2020, 12:55 PM

Update TargetLibraryInfoTest to include new LibFunc declarations.

vitalybuka added inline comments.Jul 7 2020, 2:30 PM
llvm/include/llvm/Analysis/TargetLibraryInfo.def
266

Can you move TargetLibraryInfo extension into a separate patch

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
3544

Can we do MemCpy of shadow here at all? not all bits of shadow bytes correspond to the src/dst

3546

I'd use ", Align(1), " without temp var

3553

DstShadowPtr.first for consistency

eugenis added inline comments.Jul 7 2020, 2:55 PM
compiler-rt/test/msan/libatomic.c
38

use SetCurrentDebugLocation on the builder

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
3550

I think isStore needs to be true here.

3554

How about we do the same thing that happens to memcpy() calls in the user code, i.e. emit a call to __msan_memcpy?
It will take care of everything.

guiand updated this revision to Diff 276271.Jul 7 2020, 5:16 PM
guiand marked 6 inline comments as done.

Addressed comments

guiand updated this revision to Diff 276272.Jul 7 2020, 5:20 PM

isStore=true on atomic load destination

eugenis added inline comments.Jul 17 2020, 11:15 AM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
3531

Check that the arguments are initialized (see ClCheckAccessAddress).

3559

Why not storeOrigin? It even has an option to use a runtime helper, a better one (conditional).

3585

Origin is meaningless when the shadow is 0.

guiand updated this revision to Diff 278966.Jul 17 2020, 8:42 PM
guiand marked an inline comment as done.

Remove origin store from atomic_store

guiand marked 5 inline comments as done.Jul 17 2020, 8:42 PM
guiand added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
3559

Since this function isn't necessarily called with a constant size, I needed some way to set the origin for a variable-sized region too. It seems like storeOrigin and the maybe-store functions use constant sizes?

eugenis accepted this revision.Jul 20 2020, 1:46 PM

LGTM

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
3559

Oh, right. OK, this should be good enough.

This revision is now accepted and ready to land.Jul 20 2020, 1:46 PM
This revision was automatically updated to reflect the committed changes.
eugenis added inline comments.Aug 7 2020, 11:51 AM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
3543

CB can be a terminator and not have a next node.
Example:

invoke void @__atomic_load(i64 48, i8* %4, i8* %5, i32 %6)
        to label %invoke.cont unwind label %terminate.lpad

This will crash in

#0  llvm::Value::getContext (this=0x0) at /code/llvm-project/llvm/lib/IR/Value.cpp:839
#1  0x0000000003e55767 in llvm::IRBuilder<llvm::ConstantFolder, llvm::IRBuilderDefaultInserter>::IRBuilder (
    this=0x7fffffffb4b0, IP=0x0, FPMathTag=0x0, OpBundles=...) at /code/llvm-project/llvm/include/llvm/IR/IRBuilder.h:2598
#2  0x00000000067e1233 in (anonymous namespace)::MemorySanitizerVisitor::visitLibAtomicLoad (this=0x7fffffffbfc0, CB=...)
    at /code/llvm-project/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:3508
#3  0x00000000067dfd8d in (anonymous namespace)::MemorySanitizerVisitor::visitCallBase (this=0x7fffffffbfc0, CB=...)
    at /code/llvm-project/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:3570
#4  0x00000000067dfc13 in llvm::InstVisitor<(anonymous namespace)::MemorySanitizerVisitor, void>::visitInvokeInst (
    this=0x7fffffffbfc0, I=...) at /code/llvm-project/llvm/include/llvm/IR/InstVisitor.h:220
#5  0x00000000067ddb40 in llvm::InstVisitor<(anonymous namespace)::MemorySanitizerVisitor, void>::visitInvoke (
    this=0x7fffffffbfc0, I=...) at /code/llvm-project/llvm/include/llvm/IR/Instruction.def:131