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

[Kodé Williams via Phabricator via llvm-commits]

kodewilliams marked 4 inline comments as done.
kodewilliams added inline comments.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:606
+    if (Stats[i] == 0) MutationWeights[i] = 1;
+    // 1000 due to 3 decimal places
+    else
----------------
metzman wrote:
> metzman wrote:
> > I think the placement of this comment is awkward. Maybe put on the same line as `else`.
> I'm also unsure what three decimal places this comment is referring to. We don't round anywhere here do we?
We don't round anything, just when printing but even then that's just the format specifier rounding.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:608
+    else
+      MutationWeights[i] = static_cast<size_t>((Stats[i] * 1000) / SumOfStats);
+  }
----------------
Dor1s wrote:
> metzman wrote:
> > I'm somewhat unsure why we divide by `SumOfStats`, can you explain this to me please?
> +1
I divide by SumOfStats in order to normalize the numbers, since the usefulness percentages are ratios of themselves and not a total, the only way to determine which one is better than the other is by finding how much of the total usefulness each mutation accounts for.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:610
+  }
+  for (const size_t Weight : MutationWeights) SumOfWeights += Weight;
+  UseMutationWeights = true;
----------------
metzman wrote:
> metzman wrote:
> > Think this loop can be eliminated and that SumOfWeights can be calculated in the previous loop.
> > Actually, won't it always be `1000`?
> Doesn't it always end up as `1000` (now `1000+Mutators.size()*kDefaultMutationWeight`) at the end of the loop?
> 
> Also, if a reviewer leaves a question/comment that looks like it needs a response, (ie: not a request to change something) please reply to it instead of marking it `Done`.
> It's impossible to properly review code if the author is unresponsive.
> Even if the question was discussed offline (in this particular case it wasn't), it is good to summarized the answer when for reviewers who weren't in the offline conversation.
> Do this for every question I have asked in this review.
Using discrete_distribution, I no longer need the SumOfWeights. And will do.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:598
+    double UsefulPercentage = M.TotalCount ? (M.UsefulCount) / M.TotalCount : 0;
+    Stats.push_back(UsefulPercentage);
+  }
----------------
metzman wrote:
> 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.
Oversight on my end, not sure how I missed this so many times reading it. Stats should be cleared before these push_back instructions are executed.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:606
+  SetMutationStats(Stats);
+  for (const double &Stat : Stats) SumOfStats += Stat;
+  // Convert stats into weights.
----------------
metzman wrote:
> SumOfStats can be calculated in the loop in `SetMutationStats` right?
It was suggested that SumOfStats be declared locally in AssignMutationWeights.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:611
+    if (Stats[i] == 0)
+      MutationWeights[i] = kDefaultMutationWeight;
+    else
----------------
metzman wrote:
> How about instead of having a default usefulness stat of 0 and a default MutationWeight of 1 we just use 1/(100 * 1000) as the default stat.
I feel like that works, it's close enough to zero to show basically minimal usefulness but isn't actually zero so it eliminates the case of me setting the default weight to 1.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:621
+// lowest weighted mutations.
+size_t MutationDispatcher::WeightedIndex() {
+  double RandomMark = Rand(0, SumOfWeights);
----------------
metzman wrote:
> Does this need to return a `size_t`?
> Also, was this inspired by https://stackoverflow.com/a/1761646 ?
I had the original thought to decrement each mutation stat until it got to zero (we had discussed it in person). Then I realized it would not work in a case where the number of runs was not specified, so I got to searching and someone had the same problem I had (https://medium.com/@peterkellyonline/weighted-random-selection-3ff222917eb6). Inspired by this Medium post.

In MutateImpl, by default it uses Rand(Mutators.size()) to choose a Mutator index and Rand returns a size_t, hence my choice to also return size_t.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D49621