[PATCH] D46666: [libFuzzer] Experimental data flow tracer for fuzz targets.

[Kostya Serebryany via Phabricator via llvm-commits]

kcc marked an inline comment as done.
kcc added inline comments.


================
Comment at: lib/fuzzer/dataflow/DataFlow.cpp:159
+  assert(NumFuncs == 0 && "This tool does not support DSOs\n");
+  assert(start < stop && "The code is not instrumented for coverage");
+  if (start == stop || *start) return;  // Initialize only once.
----------------
Dor1s wrote:
> nit: do we need trailing `\n` as on line 158?
doesn't matter, but I unified these. 


================
Comment at: lib/fuzzer/dataflow/DataFlow.cpp:160
+  assert(start < stop && "The code is not instrumented for coverage");
+  if (start == stop || *start) return;  // Initialize only once.
+  for (uint32_t *x = start; x < stop; x++)
----------------
Dor1s wrote:
> I don't understand `*start` condition here. As per lines 161-162, `*start` would be 0, i.e. false when already initialized? Do we need `!*start` here?
Good point. This is a copy-pasto from the docs where the indexes start from 1, not from 0. 
And I think They actually should start from 1 to avoid double-initializtion. 
Good catch! Fixed. 


================
Comment at: lib/fuzzer/dataflow/DataFlow.cpp:179
+  assert(FuncNum < NumFuncs);
+  CurrentFunc = FuncNum;
+}
----------------
Dor1s wrote:
> probably a stupid question, but are we sure than `__sanitizer_cov_trace_pc_guard` gets called before any other hook? Otherwise, how can we be sure that `CurrentFunc` has a correct value when e.g. `__dfsw___sanitizer_cov_trace_switch` or `__dfsw___sanitizer_cov_trace_*` is executed?
Yes. __sanitizer_cov_trace_pc_guard is inserted into the beginning of a function entry block. 


================
Comment at: lib/fuzzer/dataflow/DataFlow.cpp:189
+#define HOOK(Name, Type)                                                       \
+  void Name(Type Arg1, Type Arg2, dfsan_label L1, dfsan_label L2) {            \
+    assert(CurrentFunc < NumFuncs);                                            \
----------------
Dor1s wrote:
> just to confirm: the hooks defined below will get called by the runtime, when a particular comparison type is executed?
The hooks are inserted by the sanitizer coverage (and then modified by dfsan) -- they will be called before every instrumented comparison insn. 


================
Comment at: test/fuzzer/dataflow.test:27
+IN_ABC: LABELS: 4
+IN_ABC: F{{[012]}} 4
+IN_ABC-NO: F
----------------
Dor1s wrote:
> I guess order of functions can differ, this is why we use a regexp rather than a particular function number?
Yes, the order of functions is undefined. 
Note that the output doesn't contain the function names (it would be too expensive) -- it's just 'F' followed by a number. 
See the comment at the top of DataFlow.cpp


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D46666