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

[Jonathan Metzman via Phabricator via llvm-commits]

metzman requested changes to this revision.
metzman added a comment.
This revision now requires changes to proceed.

This CL is still broken, it doesn't actually do what it is intended to do.
I recommend testing it more thoroughly (manually, or preferably using written tests) to ensure that it accomplishes it what is intended to, in at least a few cases, before starting the next review. 
I personally would build one or two fuzzers with this change, and use print statements or GDB to verify that this change is doing what is intended (using `sanitizer_keep_symbols=true` as a gn arg will let you print variable values in gdb), and then write some tests.
This is good advice for most code reviews, really they should only contain broken code if you are asking the reviewer why certain code is broken.



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


================
Comment at: lib/fuzzer/FuzzerLoop.cpp:558
+  if (TotalNumberOfRuns % kRunsBeforeWeightedMutations == 0 &&
+      Options.UseWeightedMutations)
+    MD.AssignMutationWeights();
----------------
Dor1s wrote:
> Check the bool flag first, then check number of runs
+1.
When you have a condition composed of two or more conditions, you can take advantage of short circuiting by putting the more efficient (especially if likely to be false) conditions first.
You can read more on this [[ https://en.wikipedia.org/wiki/Short-circuit_evaluation#Common_use | here ]].


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:594
+// Refreshes current mutation stats recalculated based on most previous runs.
+void MutationDispatcher::SetMutationStats(Vector<double> &Stats) {
+  Stats.resize(Mutators.size(), 0);
----------------
This method is kind of weird. It is only called once with an argument called `Stats` which is a file-scope variable that it then shadows.
Please change the method so it no longer shadows the variable (ie: change the name of the argument or less preferably, change the method to not take any argument at all).
I'm also unsure it's worth keeping as its own function.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:598
+    double UsefulPercentage = M.TotalCount ? (M.UsefulCount) / M.TotalCount : 0;
+    Stats.push_back(UsefulPercentage);
+  }
----------------
This is a serious bug, it pretty much breaks the CL. 
The size of `Stats` on the first iteration is `Mutators.size()` not `0`. Pushing to the back simply doubles the size of the array so every mutator ends up with a default weight.


================
Comment at: lib/fuzzer/FuzzerMutate.h:96
 
+  // Returns a line of output containing usefulness of each mutation.
   void PrintMutationStats();
----------------
Please use [[ https://llvm.org/docs/CodingStandards.html#id14 | Doxygen style comments ]] (ie: `///`) like the other methods in this file.


================
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
+
----------------
Dor1s wrote:
> 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
I think it will be hard to verify some of the behavior that I have observed is broken in this CL with an integration test. You may want to use a unittest for this purpose.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D49621