[PATCH] D63616: Implement `-fsanitize-coverage-whitelist` and `-fsanitize-coverage-blacklist` for clang

[Matt Morehouse via Phabricator via cfe-commits]

morehouse added a comment.

Thanks for the patch!  Seems like a useful feature for targeted fuzzing.



================
Comment at: clang/docs/SanitizerCoverage.rst:310
+
+In most cases, the whitelist will list the folders or source files for which you want
+instrumentation and allow all function names, while the blacklist will opt out some specific
----------------
The wording makes it sound like there may be exceptions to the expected whitelist/blacklist behavior.  But IIUC the paragraph is meant to explain the typical use case.  Can we make this more explicit?

e.g.,
```
A common use case is to have the whitelist list folders and source files ... while the blacklist ...
```

Or maybe we don't need this paragraph at all...


================
Comment at: clang/include/clang/Driver/Options.td:978
+def fsanitize_coverage_blacklist : Separate<["-"], "fsanitize-coverage-blacklist">,
+    Group<f_Group>, Flags<[CoreOption, DriverOption]>, Alias<fsanitize_coverage_blacklist_EQ>;
 def fsanitize_memory_track_origins_EQ : Joined<["-"], "fsanitize-memory-track-origins=">,
----------------
For `fsanitize_blacklist` we only support `-fsanitize-blacklist=`.  Let's do the same for these lists to keep things simple.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:218
   Opts.StackDepth = CGOpts.SanitizeCoverageStackDepth;
-  PM.add(createSanitizerCoverageModulePass(Opts));
+  PM.add(createSanitizerCoverageModulePass(Opts, CGOpts.SanitizeCoverageWhitelistFiles, CGOpts.SanitizeCoverageBlacklistFiles));
 }
----------------
Please run `clang-format --style=LLVM` on the patch.


================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:743
+      }
+    }
+  }
----------------
The two cases have lots of overlapping code.  Let's try to coalesce.


================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:759
+      D.Diag(clang::diag::err_drv_malformed_sanitizer_coverage_blacklist) << BLError;
+  }
+
----------------
Let's try to coalesce here too.  Maybe a helper function?  Then we could also use it for the sanitizer blacklist.


================
Comment at: compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_whitelist_blacklist.cc:60
+// RUN: %clangxx -O0 %s -S -o - -emit-llvm -fsanitize-coverage=trace-pc -fsanitize-coverage-whitelist=wl_bar.txt  -fsanitize-coverage-blacklist=bl_bar.txt   2>&1 | not grep "call void @__sanitizer_cov_trace_pc"
+
+// RUN: rm wl_*.txt
----------------
Can we also test with `-fsanitize=inline-8bit-counters`, since that is what libFuzzer uses by default?


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:192
+      Blacklist = SpecialCaseList::createOrDie(BlacklistFiles);
+    }
     initializeSanitizerCoverageModulePass(*PassRegistry::getPassRegistry());
----------------
Nit:  Preferred style is no curly braces for one-statement ifs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63616