[GCOV] Add options to filter files which must be instrumented.

[calixte]

Summary

When making code coverage, a lot of files (like the ones coming from /usr/include) are removed when post-processing gcno/gcda so finally they doen't need to be instrumented nor to appear in gcno/gcda.
The goal of the patch is to be able to filter the files we want to instrument, there are several advantages to do that:

• improve speed (no overhead due to instrumentation on files we don't care)
• reduce gcno/gcda size
• it gives the possibility to easily instrument only few files (e.g. ones modified in a patch) without changing the build system
• need to accept this patch to be enabled in clang: https://reviews.llvm.org/D52034

[vsk]

lib/Transforms/Instrumentation/GCOVProfiling.cpp

102 ↗

(On Diff #165278)

Just reading the signature here, it's not clear what getRegex does. Perhaps a more specific name would help? Maybe 'createRegexesFromFilterString'?

Also, it's an llvm convention to rely on NRVO instead of having a single out-parameter (so, simply returning the std::vector here is fine).

103 ↗

(On Diff #165278)

StringRef is cheap to copy, no need for a const reference here.

135 ↗

(On Diff #165278)

StringMap<bool> sounds like it's just StringSet?

451 ↗

(On Diff #165278)

Nit, maybe 'doesFilenameMatchARegex' would be a clearer name.

476 ↗

(On Diff #165278)

This might not be portable. Try llvm::sys::fs::real_path.

479 ↗

(On Diff #165278)

Use a more specific name, perhaps 'bool ShouldInstrument'.

487 ↗

(On Diff #165278)

Does this do the right thing if both sets of filters are empty?

Stepping back a bit, what is the motivation for having both inclusion & exclusion filters? Why not just have one?

Is it so that you can, say, exclude "/usr/*", but also include "/usr/local/*"? In that case the user needs to know the order in which you apply filters, because they're not commutative.

[calixte]

lib/Transforms/Instrumentation/GCOVProfiling.cpp

135 ↗

(On Diff #165278)

I need to know if the filename has been already processed to avoid to do everything again.

476 ↗

(On Diff #165278)

Thx for that, I looked after it and didn't find it.

487 ↗

(On Diff #165278)

If both are empty then we returned at the beginning of the function.

Having only one is quite difficult to use.
The 2 common cases I thought about:

• exclude /usr/include/ stuff
• include only foo.cpp, bar.cpp (to make code coverage on modified files in a patch)

Having only one will induce that it will be more difficult to write regex.

When we've both then a file is included when it matches one of the inc regex and when it doesn't match all the exc regex.
So exc /usr/* and inc /usr/local/* means that all files in /usr/ are rejected, if you want only the /usr/local/ just don't put exc stuff.
And exc /usr/local/* and inc /usr/* means that we take all files in /usr except the ones in /usr/local.
But definitely, I must make a clearer doc.

[calixte]

Fix the nits

[calixte]

Fix stupid error when checking the returned value of real_path

[marco-c]

include/llvm/Transforms/Instrumentation.h

69 ↗

(On Diff #165648)

Nit: maybe you could give a better name to these options, e.g. something like FilterFiles and ExcludeFiles.

lib/Transforms/Instrumentation/GCOVProfiling.cpp

434 ↗

(On Diff #165648)

Nit: the name is a little confusing as it makes it look like it only applied to Filter and not to Exclude

Maybe you could rename it simply createRegexFromString?

[vsk]

lib/Transforms/Instrumentation/GCOVProfiling.cpp

474 ↗

(On Diff #165648)

If it's safe to use Filename when there's an error getting the real_path, I'm not sure why it's necessary to use the real_path at all. Why not fall back to the default behavior when there's an error?

487 ↗

(On Diff #165278)

I see, what I was looking for is the rule you specified for when inclusion + exclusion lists exist. Sounds good.

[calixte]

lib/Transforms/Instrumentation/GCOVProfiling.cpp

474 ↗

(On Diff #165648)

When compiling Firefox, most of the paths are already absolute paths with two exceptions:

• /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/***: we need to have real_path here
• hb-ot-shape-complex-indic-machine.rl: real_path is failing but if the exclude regex is hb.*\.rl we can exclude them

[calixte]

include/llvm/Transforms/Instrumentation.h

69 ↗

(On Diff #165648)

The two variables here have the same name as the options (ie coverage-filter & coverage-exclude)

[calixte]

Fix nits

[calixte]

Regexs separator is ':' for Unix and ';' for Windows

[calixte]

Fix plural form of regex and just use ';' as separator.

llvm/trunk/include/llvm/Transforms/Instrumentation.h

struct GCOVOptions {
 
   // Emit the name of the function in the .gcda files. This is redundant, as
   // the function identifier can be used to find the name from the .gcno file.
   bool FunctionNamesInData;
 
   // Emit the exit block immediately after the start block, rather than after
   // all of the function body's blocks.
   bool ExitBlockBeforeBody;
+
+  // Regexes separated by a semi-colon to filter the files to instrument.
+  std::string Filter;
+
+  // Regexes separated by a semi-colon to filter the files to not instrument.
+  std::string Exclude;
 };
 
 ModulePass *createGCOVProfilerPass(const GCOVOptions &Options =
                                    GCOVOptions::getDefault());
 
 // PGO Instrumention
 ModulePass *createPGOInstrumentationGenLegacyPass();
 ModulePass *

llvm/trunk/lib/Transforms/Instrumentation/GCOVProfiling.cpp

 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Regex.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/Instrumentation.h"
 #include "llvm/Transforms/Instrumentation/GCOVProfiler.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 #include <algorithm>
 #include <memory>
 #include <string>
 #include <utility>

 private:
   // Create the .gcno files for the Module based on DebugInfo.
   void emitProfileNotes();
 
   // Modify the program to track transitions along edges and call into the
   // profiling runtime to emit .gcda files when run.
   bool emitProfileArcs();
 
+  bool isFunctionInstrumented(const Function &F);
+  std::vector<Regex> createRegexesFromString(StringRef RegexesStr);
+  static bool doesFilenameMatchARegex(StringRef Filename,
+                                      std::vector<Regex> &Regexes);
+
   // Get pointers to the functions in the runtime library.
   Constant *getStartFileFunc();
   Constant *getEmitFunctionFunc();
   Constant *getEmitArcsFunc();
   Constant *getSummaryInfoFunc();
   Constant *getEndFileFunc();
 
   // Add the function to write out all our counters to the global destructor
private:
   char ReversedVersion[5];
   // Checksum, produced by hash of EdgeDestinations
   SmallVector<uint32_t, 4> FileChecksums;
 
   Module *M;
   const TargetLibraryInfo *TLI;
   LLVMContext *Ctx;
   SmallVector<std::unique_ptr<GCOVFunction>, 16> Funcs;
+  std::vector<Regex> FilterRe;
+  std::vector<Regex> ExcludeRe;
+  StringMap<bool> InstrumentedFiles;
 };
 
 class GCOVProfilerLegacyPass : public ModulePass {
 public:
   static char ID;
   GCOVProfilerLegacyPass()
       : GCOVProfilerLegacyPass(GCOVOptions::getDefault()) {}
   GCOVProfilerLegacyPass(const GCOVOptions &Opts)
private:
     uint32_t FuncChecksum;
     bool UseCfgChecksum;
     uint32_t CfgChecksum;
     DenseMap<BasicBlock *, GCOVBlock> Blocks;
     GCOVBlock ReturnBlock;
   };
 }
 
+// RegexesStr is a string containing differents regex separated by a semi-colon.
+// For example "foo\..*$;bar\..*$".
+std::vector<Regex> GCOVProfiler::createRegexesFromString(StringRef RegexesStr) {
+  std::vector<Regex> Regexes;
+  while (!RegexesStr.empty()) {
+    std::pair<StringRef, StringRef> HeadTail = RegexesStr.split(';');
+    if (!HeadTail.first.empty()) {
+      Regex Re(HeadTail.first);
+      std::string Err;
+      if (!Re.isValid(Err)) {
+        Ctx->emitError(Twine("Regex ") + HeadTail.first +
+                       " is not valid: " + Err);
+      }
+      Regexes.emplace_back(std::move(Re));
+    }
+    RegexesStr = HeadTail.second;
+  }
+  return Regexes;
+}
+
+bool GCOVProfiler::doesFilenameMatchARegex(StringRef Filename,
+                                           std::vector<Regex> &Regexes) {
+  for (Regex &Re : Regexes) {
+    if (Re.match(Filename)) {
+      return true;
+    }
+  }
+  return false;
+}
+
+bool GCOVProfiler::isFunctionInstrumented(const Function &F) {
+  if (FilterRe.empty() && ExcludeRe.empty()) {
+    return true;
+  }
+  const StringRef Filename = F.getSubprogram()->getFilename();
+  auto It = InstrumentedFiles.find(Filename);
+  if (It != InstrumentedFiles.end()) {
+    return It->second;
+  }
+
+  SmallString<256> RealPath;
+  StringRef RealFilename;
+
+  // Path can be
+  // /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/*.h so for
+  // such a case we must get the real_path.
+  if (sys::fs::real_path(Filename, RealPath)) {
+    // real_path can fail with path like "foo.c".
+    RealFilename = Filename;
+  } else {
+    RealFilename = RealPath;
+  }
+
+  bool ShouldInstrument;
+  if (FilterRe.empty()) {
+    ShouldInstrument = !doesFilenameMatchARegex(RealFilename, ExcludeRe);
+  } else if (ExcludeRe.empty()) {
+    ShouldInstrument = doesFilenameMatchARegex(RealFilename, FilterRe);
+  } else {
+    ShouldInstrument = doesFilenameMatchARegex(RealFilename, FilterRe) &&
+                       !doesFilenameMatchARegex(RealFilename, ExcludeRe);
+  }
+  InstrumentedFiles[Filename] = ShouldInstrument;
+  return ShouldInstrument;
+}
+
 std::string GCOVProfiler::mangleName(const DICompileUnit *CU,
                                      GCovFileType OutputType) {
   bool Notes = OutputType == GCovFileType::GCNO;
 
   if (NamedMDNode *GCov = M->getNamedMetadata("llvm.gcov")) {
     for (int i = 0, e = GCov->getNumOperands(); i != e; ++i) {
       MDNode *N = GCov->getOperand(i);
       bool ThreeElement = N->getNumOperands() == 3;

 
 bool GCOVProfiler::runOnModule(Module &M, const TargetLibraryInfo &TLI) {
   this->M = &M;
   this->TLI = &TLI;
   Ctx = &M.getContext();
 
   AddFlushBeforeForkAndExec();
 
+  FilterRe = createRegexesFromString(Options.Filter);
+  ExcludeRe = createRegexesFromString(Options.Exclude);
+
   if (Options.EmitNotes) emitProfileNotes();
   if (Options.EmitData) return emitProfileArcs();
   return false;
 }
 
 PreservedAnalyses GCOVProfilerPass::run(Module &M,
                                         ModuleAnalysisManager &AM) {
 
for (unsigned i = 0, e = CU_Nodes->getNumOperands(); i != e; ++i) {
     }
 
     std::string EdgeDestinations;
 
     unsigned FunctionIdent = 0;
     for (auto &F : M->functions()) {
       DISubprogram *SP = F.getSubprogram();
       if (!SP) continue;
-      if (!functionHasLines(F)) continue;
+      if (!functionHasLines(F) || !isFunctionInstrumented(F))
+        continue;
       // TODO: Functions using scope-based EH are currently not supported.
       if (isUsingScopeBasedEH(F)) continue;
 
       // gcov expects every function to start with an entry block that has a
       // single successor, so split the entry block to make sure of that.
       BasicBlock &EntryBlock = F.getEntryBlock();
       BasicBlock::iterator It = EntryBlock.begin();
       while (shouldKeepInEntry(It))
bool GCOVProfiler::emitProfileArcs() {
   if (!CU_Nodes) return false;
 
   bool Result = false;
   for (unsigned i = 0, e = CU_Nodes->getNumOperands(); i != e; ++i) {
     SmallVector<std::pair<GlobalVariable *, MDNode *>, 8> CountersBySP;
     for (auto &F : M->functions()) {
       DISubprogram *SP = F.getSubprogram();
       if (!SP) continue;
-      if (!functionHasLines(F)) continue;
+      if (!functionHasLines(F) || !isFunctionInstrumented(F))
+        continue;
       // TODO: Functions using scope-based EH are currently not supported.
       if (isUsingScopeBasedEH(F)) continue;
       if (!Result) Result = true;
 
       DenseMap<std::pair<BasicBlock *, BasicBlock *>, unsigned> EdgeToCounter;
       unsigned Edges = 0;
       for (auto &BB : F) {
         Instruction *TI = BB.getTerminator();