[PATCH] D103176: [dfsan] Add a flag about whether to propagate offset labels at gep

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

stephan.yichao.zhao added inline comments.


================
Comment at: compiler-rt/test/dfsan/gep.c:15
+  dfsan_set_label(1, &i, sizeof(i));
+  dfsan_set_label(0, &p, sizeof(p));
+  p = p + i;
----------------
gbalats wrote:
> Why is this needed?
removed.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:190
+enum PropagationPolicy {
+  DontCombinePointerLabelsOnLoad,
+  CombinePointerLabelsOnStore,
----------------
gbalats wrote:
> Why do we need the negated condition and not have CombinePointerLabelsOnLoad instead? Same for other options.
> We should be able to set a default value that doesn't have to be the 0 bitmask.
This is because options set do not support default values. I changed back to using individual options.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:196
+
+cl::bits<PropagationPolicy> PropagationPolicyBits(
+    cl::desc("Label Propagation Policies:"),
----------------
gbalats wrote:
> Out of curiosity, is there any benefit other in logically grouping these together when using cl::bits vs having individual options?
A set of options mainly make the code more readable. 
But it does not have a way to set default values. so I switched back to use individual options. To make them readable, we may add an argument group later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103176