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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits