[PATCH] D85928: [libFuzzer] Added -print_full_coverage flag.

[Max Moroz via Phabricator via llvm-commits]

Dor1s added a comment.

Matt, I'm trying to finish this on behalf of Ryan. Marty has been enabling the use of that feature on ClusterFuzz already, so it makes sense to move forward with this. Please take another look when you can :)



================
Comment at: compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327
+  } else {
+    F->TryDetectingAMemoryLeak(U.data(), U.size(), true);
+  }
----------------
morehouse wrote:
> Why does the flag disable leak detection?  We should either leave it enabled, or explain why it is disabled in the flag description.
added a comment


================
Comment at: compiler-rt/lib/fuzzer/FuzzerLoop.cpp:357-360
+  if (Options.PrintFullCoverage)
+    TPC.PrintCoverage(true);
   if (Options.PrintCoverage)
+    TPC.PrintCoverage(false);
----------------
morehouse wrote:
> 
done!


================
Comment at: compiler-rt/lib/fuzzer/FuzzerTracePC.cpp:300
+      } else {
+        CoveredPCs.push_back(TE->PC);
+      }
----------------
morehouse wrote:
> Nit: please remove curly braces
done


================
Comment at: compiler-rt/lib/fuzzer/FuzzerTracePC.cpp:304
+    if (PrintAllCounters) {
+      if (UncoveredPCs.size() > 0) {
+        Printf("U");
----------------
morehouse wrote:
> Do we need to exclude the U and C when the vectors are empty?  It's slightly cleaner to exclude these if-statements.
I think it depends on the consuming side. Marty, what do you think?


================
Comment at: compiler-rt/lib/fuzzer/FuzzerTracePC.cpp:272
 
-void TracePC::PrintCoverage() {
+void TracePC::PrintCoverage(const bool full) {
   if (!EF->__sanitizer_symbolize_pc ||
----------------
Dor1s wrote:
> let's rename `full` to something like `PrintAllCounters`
here as well


================
Comment at: compiler-rt/lib/fuzzer/FuzzerTracePC.h:97
 
-  void PrintCoverage();
+  void PrintCoverage(const bool PrintAllCounters);
 
----------------
morehouse wrote:
> Remove const, it is pointless for non-pointers.
done


================
Comment at: compiler-rt/test/fuzzer/full-coverage.test:4
+XFAIL: s390x
+RUN: %cpp_compiler -mllvm -use-unknown-locations=Disable %S/DSO1.cpp -fPIC %ld_flags_rpath_so1 -O0 -shared -o %dynamiclib1
+RUN: %cpp_compiler -mllvm -use-unknown-locations=Disable %S/DSO2.cpp -fPIC %ld_flags_rpath_so2 -O0 -shared -o %dynamiclib2
----------------
morehouse wrote:
> What does `-mllvm -use-unknown-locations=Disable` do, and why do we need it?
Does seem necessary!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85928