[PATCH] D84371: [DFSan] Add efficient fast16labels instrumentation mode.

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added a subscriber: Dor1s.
morehouse added a comment.

In D84371#2168367 <https://reviews.llvm.org/D84371#2168367>, @kcc wrote:

> In what cases do we still call __dfsan_union?


We still call `__dfsan_union_load` when we load sizes greater than 2 bytes but not divisible by 4.  I actually am not sure what instruction sequence would hit this case, but it was already in DFSan so I left it there.

`__dfsan_union_load` then calls `__dfsan_union`.



================
Comment at: compiler-rt/lib/dfsan/dfsan.cpp:173
+// Configures the DFSan runtime to use fast16labels mode.
+extern "C" SANITIZER_INTERFACE_ATTRIBUTE void dfsan_use_fast16labels() {
+  use_fast16labels = true;
----------------
kcc wrote:
> I wonder if we can get away w/o these flags.
> Just assume that fast16mode works only when the fast16mode instrumentation is enabled. 
> (remember, fast16mode is currently experimental, we are free to change its behavior) 
SGTM.  I only know of @Dor1s using it at the moment.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1079
+                           cast<Constant>(DFSanUseFast16LabelsFn.getCallee()),
+                           "__dfsan_fast16labels_preinit");
+    UseFast16LabelsInit->setSection(".preinit_array");
----------------
vitalybuka wrote:
> what we are going to do in __dfsan_fast16labels_preinit?
This global is simply a pointer to `dfsan_use_fast16labels()` so that it runs during preinit.  `dfsan_use_fast16labels` just enables fast16labels mode in the runtime.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1339
+        IRB.CreateAlignedLoad(IRB.getInt64Ty(), WideAddr, ShadowAlign);
+    for (uint64_t Ofs = 64 / DFS.ShadowWidthBits; Ofs != Size;
+         Ofs += 64 / DFS.ShadowWidthBits) {
----------------
vitalybuka wrote:
> have you considered a loop?
> 
I think we should avoid loop overhead at least for the common case here  (loading 8-16 bytes of shadow).

I think it's also possible to hit this case with vector instructions, but vanilla DFSan currently doesn't use a loop in that case either.  If we want to use a loop for vector loads, I think we should do it in a different patch.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84371