[PATCH] D48054: [libFuzzer] Mutation tracking and logging implemented

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: lib/fuzzer/FuzzerDefs.h:15
 
+#include <array>
 #include <cassert>
----------------
Can we include this only in the files that need it?


================
Comment at: lib/fuzzer/FuzzerFlags.def:156
 FUZZER_FLAG_STRING(data_flow_trace, "Experimental: use the data flow trace")
+FUZZER_FLAG_INT(print_mutation_usefulness, 0, "Experimental")
----------------
Maybe `print_mutation_stats` would be a more accurate name?


================
Comment at: lib/fuzzer/FuzzerLoop.cpp:369
+  if (Options.PrintMutationUsefulness)
+    MStats->PrintMutationUsefulness();
 }
----------------
Should this go before the short-circuit above?  Otherwise these stats won't be printed if `PrintFinalStats` is false.


================
Comment at: lib/fuzzer/FuzzerMutate.h:38
+  SHUFFLE_BYTES,
+  NUMBER_OF_MUTATION_TYPES
+};
----------------
Maybe call this `kNumMutationTypes` or similar to distinguish it from the other enum values.  That might help prevent future contributors from adding new values after it.


================
Comment at: lib/fuzzer/FuzzerMutationStats.cpp:18
+
+MutationStats *MStats = new MutationStats();
+
----------------
Can we just do `MutationStats MStats` instead of making this on the heap?


================
Comment at: lib/fuzzer/FuzzerMutationStats.cpp:25
+    double UsefulPercentage = (100.0 * UsefulMutations.at(i)) /
+     (1 + TotalMutations.at(i));
+    Printf("%.3f", UsefulPercentage);
----------------
Can we just do:
```
double UsefulPercentage = TotalMutations.at(i) ? ... : 0
```


================
Comment at: lib/fuzzer/FuzzerMutationStats.cpp:26
+     (1 + TotalMutations.at(i));
+    Printf("%.3f", UsefulPercentage);
+    if (i < NUMBER_OF_MUTATION_TYPES - 1) Printf(",");
----------------
The output will be easier to understand if you also print the mutation names.


================
Comment at: lib/fuzzer/FuzzerMutationStats.h:18
+public:
+  ~MutationStats() {}
+  void IncTotalMutationCount(MutationType MType);
----------------
Can we just omit the destructor since it has an empty implementation anyway?


================
Comment at: test/fuzzer/fuzzer-mutationstats.test:5
+# Ensures there are some non-zero values in the usefulness percentages printed.
+CHECK: stat::mutation_usefulness: {{.*[0-9]+\.[0-9]+[1-9]+.*}}
----------------
I think this regex can be simplified to `{{[0-9]+\.[0-9]+}}`


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D48054