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

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added a comment.

Please fix lints.



================
Comment at: compiler-rt/test/dfsan/atomic.cpp:37
+    pthread_join(t[i], 0);
+  }
+  return 0;
----------------
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.


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


================
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 =
----------------
Any reason not to use the load app, load shadow ordering always?


================
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.
+}
----------------
Should we call this function `storeZeroPrimitiveShadow` then?


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2174
+  I.setSuccessOrdering(AddReleaseOrdering(I.getSuccessOrdering()));
+}
+
----------------
Please add comments to `visitAtomicRMWInst` and `visitAtomicCmpXchgInst` explaining why we modify the atomic orderings.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97310