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

[Max Moroz via Phabricator via llvm-commits]

Dor1s added a comment.

I don't like the test as it only tests that we do not completely break libFuzzer, but doesn't test the feature itself. I'll play with some ideas locally, will share those if anything works out. Otherwise, I guess we'll proceed with this test.



================
Comment at: lib/fuzzer/FuzzerMutate.cpp:35
       {
-          {&MutationDispatcher::Mutate_EraseBytes, "EraseBytes", 0, 0},
-          {&MutationDispatcher::Mutate_InsertByte, "InsertByte", 0, 0},
+          // Initialize total count as 1 to prevent zero division.
+          {&MutationDispatcher::Mutate_EraseBytes, "EraseBytes", 0, 1},
----------------
"zero division" -> "division by zero"


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:67
+
+  // For weighted mutation selection.
+  MutationWeights.resize(Mutators.size(), kDefaultMutationWeight);
----------------
add ", init with uniform weights distribution." to the comment please


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:68
+  // For weighted mutation selection.
+  MutationWeights.resize(Mutators.size(), kDefaultMutationWeight);
+  Stats.resize(Mutators.size(), kDefaultMutationStat);
----------------
Let's remove `kDefaultMutationWeight` variable, just use 1.0 here.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:69
+  MutationWeights.resize(Mutators.size(), kDefaultMutationWeight);
+  Stats.resize(Mutators.size(), kDefaultMutationStat);
 }
----------------
We should start with 0 values in `Stats`, remove `kDefaultMutationStat` and leave `Stats.resize(Mutators.size()` here.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:524
   // Try several times before returning un-mutated data.
+  Mutator *M;
   for (int Iter = 0; Iter < 100; Iter++) {
----------------
Please do `Mutator *M = nullptr;` just to be safe in case there ever will be any bug and the pointer won't be initialized.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:611
+size_t MutationDispatcher::WeightedIndex() {
+  std::discrete_distribution<size_t> SelectIndex(MutationWeights.begin(),
+                                                 MutationWeights.end());
----------------
We should keep the distribution object inside of `MutationDispatcher` class and update it only every `kUpdateMutationWeightRuns`, not for every weighted index retrieval.


================
Comment at: lib/fuzzer/FuzzerMutate.h:104
+  /// Refreshes current mutation stats recalculated based on most previous run.
+  void SetMutationStats();
+
----------------
Let's rename this method to `UpdateMutationStats`, that name would make more sense.


================
Comment at: lib/fuzzer/FuzzerMutate.h:174
+  // Used to weight mutations based on usefulness.
+  Vector<double> MutationWeights;
+  Vector<double> Stats;
----------------
This line and line 175 feel slightly inconsistent, both should be `MutationWeights` and `MutationStats` or just `Weights` and `Stats`. I'd prefer the latter, as we're already inside `MutationDispatcher` class and clearly deal with mutations here.


================
Comment at: lib/fuzzer/FuzzerMutate.h:174
+  // Used to weight mutations based on usefulness.
+  Vector<double> MutationWeights;
+  Vector<double> Stats;
----------------
Dor1s wrote:
> This line and line 175 feel slightly inconsistent, both should be `MutationWeights` and `MutationStats` or just `Weights` and `Stats`. I'd prefer the latter, as we're already inside `MutationDispatcher` class and clearly deal with mutations here.
Actually, we don't need `MutationWeights` here, we need only the distribution. Could you please do the following:

1) Remove this `MutationWeights` member

2) Add `Distribution` member (also see my comment for `WeightedIndex()` function)

3) Rename `MutationDispatcher::AssignMutationWeights()` to `MutationDispatcher::UpdateDistribution()`

4) In `MutationDispatcher::UpdateDistribution()` create a local vector `Weights` and update it using `Stats` value as you do now

5) Update the `Distribution` using the `Weights`, e.g. see how it's done for the corpus distribution (FuzzerCorpus.h:294)


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D49621