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

[Max Moroz via Phabricator via llvm-commits]

Dor1s added a comment.

Kode, you also need to write a test for the functionality your're adding. Please check out the section regarding the tests in LLVM Dev Starting Kit. Then, take a look at the existing tests: https://github.com/llvm-mirror/compiler-rt/tree/master/test/fuzzer, especially at "fuzzer-finalstats.test".

While thinking about the test, I've realized the following. Since we eventually plan to remove the detailed statistics output (with mutation names), we will have to update the tests after that. The fact that we have that detailed output right now would also slightly complicate writing a test for you.

So, my proposal would be to remove the detailed output at all, just print those `useful / total` float numbers in a single line, as we discussed today. We still be using `MutationType` enum type for tracking mutation stats, but we can get rid of `kMutationNames` and the code that uses it. Struct `Mutator` would have both name and identifier. That would simplify the code and also automatically address some of Jonathan's comments.



================
Comment at: lib/fuzzer/FuzzerMutate.h:112
     size_t (MutationDispatcher::*Fn)(uint8_t *Data, size_t Size, size_t Max);
-    const char *Name;
+    MutationType Identifier;
   };
----------------
Please see my main comment to this CL, and let's have both `const char *Name;` and `MutationType Identifier;` in this struct.


================
Comment at: lib/fuzzer/FuzzerMutate.h:170
 
+const std::unordered_map<MutationType, std::string> kMutationNames = {
+  {ManualDict, "AddWordFromManualDictionaryCount"},
----------------
I've just realized that this lookup thing is unnecessary complicated which we'll eventually have to remove anyway. Let's do this now, please see my main comment to the CL.


================
Comment at: lib/fuzzer/FuzzerMutationStats.cpp:38
+void MutationStats::IncTotalMutationCount(MutationType MType) {
+  if ((MType < 0) || MType >= MaxNumberOfMutationTypes)
+    return;
----------------
metzman wrote:
> Please remove the paren around the first condition, i don't think it helps readability and is inconsistent with the second condition.
Actually, inverse the condition and put it inside an assert:

assert(MType >= 0 && MType < MaxNumberOfMutationTypes);


================
Comment at: lib/fuzzer/FuzzerMutationStats.cpp:45
+void MutationStats::IncUsefulMutationCount(MutationType MType) {
+  if ((MType < 0) || MType >= MaxNumberOfMutationTypes)
+    return;
----------------
metzman wrote:
> Please remove the paren around the first condition, i don't think it helps readability and is inconsistent with the second condition.
Actually, inverse the condition and put it inside an assert:

```
  assert(MType >= 0 && MType < MaxNumberOfMutationTypes);
```




================
Comment at: lib/fuzzer/FuzzerMutationStats.h:25
+  // A total count of each mutation used in the fuzzing process.
+  std::array<size_t, MaxNumberOfMutationTypes> TotalMutations;
+  // The number of each mutation that resulted in new coverage.
----------------
metzman wrote:
> I'm a bit conflicted about using size_t here. On the one hand it is a nice way of saying the largest int type. On the other, this isn't really a size.
yeah, `uint64_t` would be a better choice


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D48054