[PATCH] D73776: Entropic: Boosting LibFuzzer Performance

[marcel via Phabricator via llvm-commits]

marcel marked 19 inline comments as done.
marcel added a comment.

Preparing the next patch.



================
Comment at: compiler-rt/lib/fuzzer/FuzzerCorpus.h:278
+      // Remove most abundant rare feature.
+      RareFeatures.erase(remove(RareFeatures.begin(), RareFeatures.end(),
+                                ST_mostAbundantRareFeatureIdx),
----------------
Dor1s wrote:
> assuming this code gets executed quite often, and the order inside `RareFeatures` isn't important, we can avoid erase-remove and do something like:
> 
> ```
> RareFeatures[index_from_the_loop] = RareFeatures.back();
> RareFeatures.resize(RareFeatures.size() - 1);
> ```
> 
> but the loop on line 269 would have to use index in the vector (from 1 to `< RareFeatures.size()`) instead of the iterator
> 
> feel free to ignore though, it's just a suggestion which may or may not be a good one :)
With the subsequent push_back (Line 292), do you mean a swap and pop_back here?


================
Comment at: compiler-rt/lib/fuzzer/FuzzerCorpus.h:357
+      if (II->FeatureFreqs.empty()) {
+        II->FeatureFreqs.push_back(std::pair<uint32_t, uint16_t>(Idx32, 1));
+      } else {
----------------
Dor1s wrote:
> Dor1s wrote:
> > can do return after this line and avoid `else` with extra indentation below
> what is `1` here? would it make sense to have a static const variable with a descriptive name, e.g. `kDefaultSomething`?
We are setting the frequency of Idx32 to 1. Adding a comment.


================
Comment at: compiler-rt/lib/fuzzer/FuzzerCorpus.h:362
+            std::lower_bound(II->FeatureFreqs.begin(), II->FeatureFreqs.end(),
+                             std::pair<uint32_t, uint16_t>(Idx32, 0));
+
----------------
Dor1s wrote:
> same point, what is `0`? a constant with a descriptive name or (less preferred) comment would be really helpful for others who come to read the code in future
Zero (0) is the default value for lower_bound as binary search over a vector of pairs. In this case, any value would work.


================
Comment at: compiler-rt/lib/fuzzer/FuzzerCorpus.h:380
+  // of the seed. Since we do not know the entropy of a seed that has
+  // never been executed we assign fresh seeds maximum entropy and
+  // let II->Energy approach the true entropy from above.
----------------
Dor1s wrote:
> From the code below it seems like `Energy` represents entropy and the max value is 0, which we reduce depending on the actual feature frequencies. Is that correct understanding?
Yes, we estimate the entropy over the probabilities of the features in the neighborhood of the seed. Entropy is positive. The maximum entropy is `logl(GlobalNumberOfFeatures)`.


================
Comment at: compiler-rt/lib/fuzzer/FuzzerCorpus.h:445
+    // i.e., randomly do not skip.
+    if (!DistributionNeedsUpdate && Rand(kSparseEnergyUpdates))
+      return;
----------------
Dor1s wrote:
> it seems like the `Rand(kSparseEnergyUpdates)` clause is applicable to the `Entropic` case only, is that correct? Do we really need it in the vanilla case?
Yes. `kSparseEnergyUpdates` should apply only for Entropic.


================
Comment at: compiler-rt/lib/fuzzer/FuzzerLoop.cpp:712
   }
+  II.NeedsUpdate = true;
 }
----------------
Dor1s wrote:
> does it always need update, even when new coverage wasn't observed?
For II, the local feature frequencies have changed. So we schedule an update. However, it will only be updated when the distribution needs an update, and we do not set `DistributionNeedsUpdate` here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73776/new/

https://reviews.llvm.org/D73776