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

[Jonathan Metzman via Phabricator via llvm-commits]

metzman added inline comments.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:587
 
+void MutationDispatcher::GetMutationStats(Vector<double> &Stats) {
+  Stats.resize(Mutators.size(), 0);
----------------
metzman wrote:
> I think this should be `SetMutationStats` since it sets something rather than returns something.
> I would also add a comment explaining what this method does since it changes the vector it accepts.
Please make this comment say explicitly that it modifies the argument it is passed.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:591
+    double UsefulPercentage =
+        M.TotalCount ? (100.0 * M.UsefulCount) / M.TotalCount : 0;
+    Stats.push_back(UsefulPercentage);
----------------
metzman wrote:
> Although we use percentages in when printing stats, I'm not sure we need to do it here (and in other parts of this CL), since the only other thing that will see this number is your own code.
Since this isn't a percentage anymore, the name `UsefulPercentage` needs to be updated.
Some general advice: After making suggested changes in  a code review, you should go back through your code to see if it still makes sense, and then update it where the suggested changes have amde it necessary to do so.
Also, the parentheses don't do anything here.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:610
+  }
+  for (const size_t Weight : MutationWeights) SumOfWeights += Weight;
+  UseMutationWeights = true;
----------------
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.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:606
+  SetMutationStats(Stats);
+  for (const double &Stat : Stats) SumOfStats += Stat;
+  // Convert stats into weights.
----------------
SumOfStats can be calculated in the loop in `SetMutationStats` right?


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:611
+    if (Stats[i] == 0)
+      MutationWeights[i] = kDefaultMutationWeight;
+    else
----------------
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.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:614
+      MutationWeights[i] =
+          ((Stats[i] * 1000) / SumOfStats) + kDefaultMutationWeight;
+    SumOfWeights += MutationWeights[i];
----------------
Please leave a comment explaining why we are adding `kDefaultMutationWeight` here.
Also, since this else body is multiple lines, please put curly braces around it.
Another thing: let's use a const instead of magic number `1000`.
Some general advice about code reviews: If a reviewer comments that a change should be made, you should go through your code and see if that same change needs to be made elsewhere.

nit: inner parentheses not actually needed.


================
Comment at: lib/fuzzer/FuzzerMutate.cpp:621
+// lowest weighted mutations.
+size_t MutationDispatcher::WeightedIndex() {
+  double RandomMark = Rand(0, SumOfWeights);
----------------
Does this need to return a `size_t`?
Also, was this inspired by https://stackoverflow.com/a/1761646 ?


================
Comment at: lib/fuzzer/FuzzerMutate.h:109
+
+  // Has a higher probability of returning a mutation with a greater weight.
+  size_t WeightedIndex();
----------------
This comment is phrased somewhat differently from the others, maybe phrase it like so:
`Returns the Index of a mutation...`


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D49621