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

[Jonathan Metzman via Phabricator via llvm-commits]

metzman added a comment.

@Dor1s I still think using a map that contains string -> count makes most sense. 
What is the advantage of using an array? I don't consider the performance benefits worth it since that cost is paid instead when printing the name of a mutation (ie: when this option is not being used).
Additionally this approach is simpler, we only need 2 data structures instead of 3.



================
Comment at: lib/fuzzer/FuzzerMutate.cpp:41
+          {&MutationDispatcher::Mutate_ShuffleBytes, ShuffleBytes},
+          {&MutationDispatcher::Mutate_ChangeASCIIInteger, ChangeAsciiInt},
+          {&MutationDispatcher::Mutate_ChangeBinaryInteger, ChangeBinInt},
----------------
I think the names here should match the mutations exactly (minus the `Mutate_` prefix I guess). So please change this to  `ChangeAsciiInteger`.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:44
+          {&MutationDispatcher::Mutate_CopyPart, CopyPart},
+          {&MutationDispatcher::Mutate_CrossOver, CrossOverData},
           {&MutationDispatcher::Mutate_AddWordFromManualDictionary,
----------------
Same as above, please change to `CrossOver`


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:52
     DefaultMutators.push_back(
-        {&MutationDispatcher::Mutate_AddWordFromTORC, "CMP"});
+        {&MutationDispatcher::Mutate_AddWordFromTORC, CMP});
 
----------------
Same as above, please change to `AddWordFromTORC`


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:55
   if (EF->LLVMFuzzerCustomMutator)
-    Mutators.push_back({&MutationDispatcher::Mutate_Custom, "Custom"});
+    Mutators.push_back({&MutationDispatcher::Mutate_Custom, CustomMutation});
   else
----------------
Same as above, please change to `Custom` (I don't love the fact that its less descriptive than the name you used)


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:490
+  for (auto M : CurrentMutatorSequence) {
+    auto currentMutation = kMutationNames.find(M.Identifier);
+    Printf("%s-", currentMutation->second.c_str());
----------------
Even though the lookuptime is constant, I don't like that we are slowing down fuzzing even when this option is not used.
We wouldn't need to do this if the map was string -> enum Value.


================
Comment at: lib/fuzzer/FuzzerMutate.h:115
 
+
+private:
----------------
Please remove this blank line so that there is just one blank line between the end of `Muatator` and `private`


================
Comment at: lib/fuzzer/FuzzerMutationStats.cpp:23
+  Printf("\nTotal Mutations ----------\n");
+  for (int i = 0; i < MaxNumberOfMutationTypes; i++) {
+    auto current = kMutationNames.find((MutationType) i);
----------------
Probably nicer to use the iterator as done in FuzzerMutate.cpp:540


================
Comment at: lib/fuzzer/FuzzerMutationStats.cpp:29
+  Printf("\nUseful Mutations ----------\n");
+  for (int i = 0; i < MaxNumberOfMutationTypes; i++) {
+    auto current = kMutationNames.find((MutationType) i);
----------------
Same as before: Probably nicer to use the iterator as done in FuzzerMutate.cpp:540


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


================
Comment at: lib/fuzzer/FuzzerMutationStats.cpp:40
+    return;
+  else
+    TotalMutations[MType]++;
----------------
We can get rid of the else here.


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


================
Comment at: lib/fuzzer/FuzzerMutationStats.cpp:47
+    return;
+  else
+    UsefulMutations[MType]++;
----------------
We can get rid of the else here.


================
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.
----------------
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.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D48054