[PATCH] D49621: [libFuzzer] Initial implementation of weighted mutation leveraging during runtime.

[Max Moroz via Phabricator via llvm-commits]

Dor1s added inline comments.


================
Comment at: lib/fuzzer/FuzzerFlags.def:164
 FUZZER_FLAG_INT(print_mutation_stats, 0, "Experimental")
+FUZZER_FLAG_INT(use_weighted_mutations, 0, "Experimental: If 1, fuzzing will favor mutations that perform better during runtime.")
----------------
please format the line to be consistent with style of the rest of the file


================
Comment at: lib/fuzzer/FuzzerLoop.cpp:41
 static const size_t kMaxUnitSizeToPrint = 256;
+const size_t kRunsBeforeWeightedMutations = 10000;
 
----------------
why not static?


================
Comment at: lib/fuzzer/FuzzerLoop.cpp:41
 static const size_t kMaxUnitSizeToPrint = 256;
+const size_t kRunsBeforeWeightedMutations = 10000;
 
----------------
Dor1s wrote:
> why not static?
I think it may have a better name, maybe something like `kUpdateMutationWeightRuns`


================
Comment at: lib/fuzzer/FuzzerLoop.cpp:558
+  if (TotalNumberOfRuns % kRunsBeforeWeightedMutations == 0 &&
+      Options.UseWeightedMutations)
+    MD.AssignMutationWeights();
----------------
Check the bool flag first, then check number of runs


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:608
+    else
+      MutationWeights[i] = static_cast<size_t>((Stats[i] * 1000) / SumOfStats);
+  }
----------------
metzman wrote:
> I'm somewhat unsure why we divide by `SumOfStats`, can you explain this to me please?
+1


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:23
+double SumOfWeights;
+double SumOfStats;
+Vector<double> Stats;
----------------
Do we use this anywhere outside of `MutationDispatcher::AssignMutationWeights`? Looks like we don't, please use a local variable inside the function for that.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:69
+  // For weighted mutation selection
+  MutationWeights.resize(DefaultMutators.size(), 0);
 }
----------------
This won't use `Mutate_CustomCrossOver`, will it? We should probably use `Mutators.size()` here and debug what was the root cause of the test that was failing for you.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:595
+void MutationDispatcher::SetMutationStats(Vector<double> &Stats) {
+  Stats.resize(Mutators.size(), 0);
+  for (auto &M : Mutators) {
----------------
Mismatch with line 69


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:623
+  double RandomMark = Rand(0, SumOfWeights);
+  for (size_t i = 0; i < MutationWeights.size(); i++) {
+    if (RandomMark <= MutationWeights[i])
----------------
That's not good, we should not loop through the array and implement our own weighted random selector, there are STL classes for that. See how corpus distribution is implemented, for an example: https://github.com/llvm-mirror/compiler-rt/blob/master/lib/fuzzer/FuzzerCorpus.h#L294

I think you can try `std::discrete_distribution` instead, it looks sufficient for our purpose.


================
Comment at: test/fuzzer/fuzzer-weightedmutations.test:4
+# Weighted mutations only trigger after first 10,000 runs, hence flag.
+RUN: not %run %t-WeightedMutationsTest -use_weighted_mutations=1 -runs=100000 2>&1 | FileCheck %s
+
----------------
metzman wrote:
> How long does it take to do 100k runs? Does this UnitTest take long?
> 
> Also, what behavior does this test verify? The only thing I can tell is that weighted mutations doesn't crash anything. 
> I think it would be nice to have a test of something more substantial if possible.
+1


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D49621