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

[Max Moroz via Phabricator via llvm-commits]

Dor1s accepted this revision.
Dor1s added a comment.
This revision is now accepted and ready to land.

LGTM! Left some questions though, mostly for my own education, I guess 😛



================
Comment at: lib/fuzzer/dataflow/DataFlow.cpp:73
+extern "C" {
+extern int LLVMFuzzerTestOneInput(const unsigned char *data, size_t size);
+__attribute__((weak)) extern int LLVMFuzzerInitialize(int *argc, char ***argv);
----------------
nit: in other places these use CamelCase: `Data`, `Size`


================
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.
----------------
nit: do we need trailing `\n` as on line 158?


================
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++)
----------------
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?


================
Comment at: lib/fuzzer/dataflow/DataFlow.cpp:179
+  assert(FuncNum < NumFuncs);
+  CurrentFunc = FuncNum;
+}
----------------
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?


================
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);                                            \
----------------
just to confirm: the hooks defined below will get called by the runtime, when a particular comparison type is executed?


================
Comment at: test/fuzzer/dataflow.test:27
+IN_ABC: LABELS: 4
+IN_ABC: F{{[012]}} 4
+IN_ABC-NO: F
----------------
I guess order of functions can differ, this is why we use a regexp rather than a particular function number?


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D46666