[PATCH] D62888: [NewPM] Port Sancov

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

This was a lot easier for me to understand too, thanks. Somewhat minor code changes below.



================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1231-1232
+      MPM.addPass(ModuleSanitizerCoveragePass(SancovOpts));
+      MPM.addPass(
+          createModuleToFunctionPassAdaptor(SanitizerCoveragePass(SancovOpts)));
+    }
----------------
Is there an existing function pass pipeline we can put this in, maybe by using an extension point? That would make it much faster to run I suspect.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/SanitizerCoverage.h:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
----------------
Need to use the new file header.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/SanitizerCoverage.h:14
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_TRANSFORMS_INSTRUMENTATION_SANITIZERCOVERAGE_H
+#define LLVM_TRANSFORMS_INSTRUMENTATION_SANITIZERCOVERAGE_H
----------------
I feel like we usually have whitespace here too?


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/SanitizerCoverage.h:24
+
+/// This is a wrapper for SanitizerCoverage usage in the new pass manager. The
+/// pass instruments functions for coverage.
----------------
Not really a wrapper, just the pass itself...

`SanitizerCoverage` is an implementation detail that the reader here doesn't really know about.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/SanitizerCoverage.h:37
+
+/// This is a wrapper for the ModuleSanitizerCoverage. This adds initialization
+/// calls to the module for trace PC guards and 8bit counters if they are
----------------
Same comment as above.


================
Comment at: llvm/lib/Passes/PassRegistry.def:248
 FUNCTION_PASS("tsan", ThreadSanitizerPass())
+FUNCTION_PASS("sancov-func", SanitizerCoveragePass())
 #undef FUNCTION_PASS
----------------
Consistency would suggest just `sancov` here? I don't feel strongly though.


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:228-229
+      : Options(OverrideFromCL(Options)) {
+    initializeModuleSanitizerCoverageLegacyPassPass(
+        *PassRegistry::getPassRegistry());
   }
----------------
This should be in the legacy pass below? (actually, I tihnk it already is, so this can likely be removed)


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:262-269
+    bool ShouldEmitInitCalls = false;
+    for (const Function &F : M) {
+      if (canInstrumentWithSancov(F)) {
+        ShouldEmitInitCalls = true;
+        break;
+      }
+    }
----------------
```
if (!llvm::any_of(M, [](const Function &F) { return can InstrumentWithSancov(F); }))
  return false;
```


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:442
+  const PostDominatorTree *PDT = &AM.getResult<PostDominatorTreeAnalysis>(F);
+  SanitizerCoverage Sancov(*F.getParent(), Options);
+  if (Sancov.instrumentFunction(F, DT, PDT))
----------------
I completely misread this the first time through thinking you used a common object for state....

Maybe change the constructor to accept a function to make it more obvious that this is intended to be built per-function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62888