[PATCH] D16374: [sancov] generating html report on coverage dump

[Alexey Samsonov via llvm-commits]

samsonov added inline comments.

================
Comment at: lib/sanitizer_common/sanitizer_coverage_libcdep.cc:787
@@ +786,3 @@
+static void GenerateHtmlReport(const InternalMmapVector<char *> &sancov_argv) {
+  if (!common_flags()->html_cov_report) {
+    return;
----------------
Consider adding an early return here instead
  if (!common_flags()->html_cov_report || sancov_args[0] == nullptr)


================
Comment at: lib/sanitizer_common/sanitizer_coverage_libcdep.cc:800
@@ +799,3 @@
+      if (result == 0) {
+        VReport(1, " CovDump: html report generated to %s (%d)\n",
+                report_path.data(), result);
----------------
Don't you want to log unsuccessful attempt to generate html?

================
Comment at: lib/sanitizer_common/sanitizer_flags.inc:206
@@ +205,2 @@
+COMMON_FLAG(bool, html_cov_report, false, "Generate html coverage report.")
+COMMON_FLAG(const char *, sancov, "sancov", "Sancov tool location.")
----------------
Is sancov_path more appropriate flag name?

================
Comment at: test/asan/lit.cfg:144
@@ +143,3 @@
+sancovcc_path = os.path.join(
+  get_required_attr(config, "llvm_obj_root"), "bin/sancov") 
+if os.path.exists(sancovcc_path):
----------------
I think you should use `llvm_tools_dir` instead. See the way it's handled in `test/lit.common.cfg`: probably you want to add this functionality there. Also, as LLVM tools directory is in PATH, auto-discovery of "sancov" binary must work in the lit test (in the same way we auto-discover llvm-symbolizer).

================
Comment at: test/asan/lit.cfg:147
@@ -142,1 +146,3 @@
+  config.available_features.add("has_sancovcc")
+  config.substitutions.append( ("%sancovcc ", sancovcc_path) )
 
----------------
You have to explicitly add dependency on "sancov" binary from sanitizer test suites: see `test/CMakeLists.txt`


http://reviews.llvm.org/D16374