[PATCH] D97310: [dfsan] Conservative solution to atomic load/store

[stephan.yichao.zhao via Phabricator via llvm-commits]

stephan.yichao.zhao added a subscriber: evghenii.
stephan.yichao.zhao added inline comments.


================
Comment at: compiler-rt/test/dfsan/atomic.cpp:37
+    pthread_join(t[i], 0);
+  }
+  return 0;
----------------
morehouse wrote:
> This test seems to miss the mark.  All it checks is that atomic stores set shadow to 0, and all the thread stuff is unnecessary to check this.
> 
> But where multiple threads *would* be helpful is to write a test where overtainting would occur before this change, but not after.
Updated the test case. Set atomic_i with an initial label to ensure load can get only initial labels or zeros.


================
Comment at: compiler-rt/test/dfsan/atomic.cpp:39
+  return 0;
+}
----------------
morehouse wrote:
> Should we also test builtin atomics?  https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
Yes. 

DFSan needs some extension to work with Builtins. MSan uses a function pass TargetLibraryInfoWrapperPass to identify builtin calls, while DFSan is a module pass.
And DFSan's ABI does not work with builtins because builtins do not have C libcalls.

Our current internal use cases do not support Builtins. But we need to support them in the next step.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2009
   Align Alignment = ClPreserveAlignment ? LI.getAlign() : Align(1);
+  Instruction *Pos = LI.isAtomic() ? LI.getNextNode() : &LI;
   Value *PrimitiveShadow =
----------------
morehouse wrote:
> Any reason not to use the load app, load shadow ordering always?
This code could seem confusing. Actually it does not skip a line. It sets the code insertion point after app atomic load. None-atomic load still inserts instrumented code before them. Actually they can be the same but that would require updating a lot of tests...


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2041
+  // Do not write origins for 0 shadows because we do not trace origins for
+  // untainted sinks.
+}
----------------
morehouse wrote:
> Should we call this function `storeZeroPrimitiveShadow` then?
done. Thank you.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2174
+  I.setSuccessOrdering(AddReleaseOrdering(I.getSuccessOrdering()));
+}
+
----------------
morehouse wrote:
> Please add comments to `visitAtomicRMWInst` and `visitAtomicCmpXchgInst` explaining why we modify the atomic orderings.
This is a good point. Since we always use 0 shadows, what their orderings are does not matter.

@evghenii, this followed MSan. Do you think it is fine not to change their orderings?


================
Comment at: llvm/test/Instrumentation/DataFlowSanitizer/atomics.ll:54
+
+; cmpxchg: the same as above, but also check %a shadow
+
----------------
morehouse wrote:
> I don't see `%a`'s shadow being checked anywhere.
updated comments. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97310/new/

https://reviews.llvm.org/D97310