Displaying coverage information for all source files present in a directory

[chakshugrover]

Summary

The llvm-cov tool currently reads code coverage data files and displays the coverage information for a specified source file. This patch will enable us to provide it with a directory and it will display the information for all the source files inside that directory by iterating over all of them using directory_iterator.
It still works if you provide a single source file.

[vsk]

Thank you very much for doing this!

Logistics note: in the future, please upload a diff with more context (git diff -U1000 ...).

tools/llvm-cov/CodeCoverage.cpp

38

I don't think you need any of these #includes.

76

Please add a short, doxygen-style comment describing what walk does. Also: walk does not seem like a sufficiently descriptive name. Wdyt of e.g collectSourceFiles?

76

I think a StringRef would work better than a SmallString here.

248

Please use llvm::sys::fs::status.

249

nit, I prefer checking for the simplest/base case first. Start by handling llvm::sys::fs::is_regular_file(FS), followed by llvm::sys::fs::is_directory(FS), followed by the error case.

250

Please follow the existing error-reporting idiom in this file (e.g colored_ostream(), "error: Failed to ....").

257

Why not just call walk on File->path() directly? It should handle files, directories, and access errors correctly.

[vsk]

One last note: you'll need to add a small test to test/tools/llvm-cov along with this patch.

[davidxl]

tools/llvm-cov/CodeCoverage.cpp

255

Can you just use recursive_directory_iterator?

[chakshugrover]

I have tried to incorporate all the comments.
The code is still not what i would like it to be!

I started with this

void CodeCoverageTool::collectSourceFiles(StringRef Path) {

llvm::sys::fs::file_status Status;
llvm::sys::fs::status(Path, Status);
if (llvm::sys::fs::exists(Status)) {
  // If it's a source file, collect it.
  if (llvm::sys::fs::is_regular_file(Status)) {
    SourceFiles.push_back(Path.str());
    return;
  }
  if (llvm::sys::fs::is_directory(Status)) {
    std::error_code EC;
    // Collect all source files within this directory by iterating
    // recursively.
    for (llvm::sys::fs::recursive_directory_iterator File(Path, EC), FileEnd;
         File != FileEnd && !EC; File.increment(EC)) {
      if (llvm::sys::fs::is_regular_file(File->path())) {
        SourceFiles.push_back(File->path());
      }
    }
    return;
  }
} else
  fprintf(stderr, "Error: Can't access: %s\n", Path.str().c_str());
return;

}

But the code above breaks the test case test/tools/llvm-cov/report.cpp. Please suggest how to go about it.
The script that it emitted works without any error if run from test/tools/llvm-cov/ directory but the test fails.

[chakshugrover]

tools/llvm-cov/CodeCoverage.cpp

35

Removed

260

Please check the comment above.

[vsk]

There shouldn't be a need to check in a binary profdata file.

test/tools/llvm-cov/cov_dir.cpp

2

Instead of checking in an executable, use llvm-cov convert-for-testing cov_dir_test -o cov_dir.covmapping and check in the covmapping.

tools/llvm-cov/CodeCoverage.cpp

73

nit, Please add a period.

269

You aren't checking whether this is a regular file, or if it exists. Could you extend the patch in your penultimate comment with extra error checking and report back on the exact failures you're seeing?

[chakshugrover]

Sorry for the delay. Please review the updated collectSourceFiles function. I had to explicitly handle the case when -filename-equivalence option is passed.

I am facing issue in creating the test case. As absolute path of files being compiled gets hardcoded inside the covmapping, I had to use -filename-equivalence while passing file names. I couldn't find a way to pass a directory name while using this option such that my code actually gets tested.
Please suggest how should i proceed with the test case.

[chakshugrover]

Had forgotten to add period.

[chakshugrover]

Ping
Thanks

[vsk]

Meta: Please wait one week before pinging patches, see http://llvm.org/docs/DeveloperPolicy.html#code-reviews.

Your code isn't being tested because -filename-equivalence sets CompareFilenamesOnly: collectSourceFiles pushes back the directory path and returns immediately when this happens.

Please don't check in a profdata file or any source files. Add a hidden -dump-input-file-list cl::opt to CodeCoverageTool::run and write a test like this:

RUN: rm -rf %t
RUN: mkdir -p %t/a/b/
RUN: echo "" > %t/a/b/c.tmp
RUN: echo "" > %t/a/d.tmp
RUN: llvm-cov show %S/Inputs/directory-search.covmapping -instr-profile=/dev/null -dump-input-file-list | FileCheck %s
CHECK-DAG: {{.*}}c.tmp
CHECK-DAG: {{.*}}d.tmp

This tests the functionality you're adding, instead of testing e.g the coverage report format, which is unrelated.

[chakshugrover]

Added the requested -dump-input-file-list cl::opt to CodeCoverageTool::run and completed test case.
Sorry for the delay. This is the first time I am trying to write a test case.

[vsk]

test/tools/llvm-cov/cov_dir.cpp

6

It might make more sense to pass "/dev/null" for the covmapping file, since it ultimately isn't used. Also, please add another RUN line to test your code with -filename-equivalence turned on.

tools/llvm-cov/CodeCoverage.cpp

96

Does this really need to live here? Why not just deal with it in parseCommandLineArgs?

257

Why are we skipping the recursive directory traversal if -filename-equivalence is turned on?

279

Drop the redundant return statement here.

282

Use the error method instead of fprintf.

356

You're only handling this argument when running the "show" sub-command. It would be better to handle it in commandLineParser.

480

In llvm, this is typically written using a range-based for, e.g: for (StringRef SF : SourceFiles).

481

I think this file uses the llvm namespace, so you should be able to drop the llvm::.

[chakshugrover]

DumpInputFileList is handled in commandLineParser itself. I make an early exit from show and report subcommands if this option is enabled ( hence the need of bool DumpFileNames in class CodeCoverageTool ). I skip the recursive directory traversal if -filename-equivalence is turned on as I believe that option is meant for a completely different purpose ( Treat source files (not directories) as equivalent to paths in the coverage data even when only the file names match )

[chakshugrover]

test/tools/llvm-cov/cov_dir.cpp

6

As the code is being tested already in multiple test cases such as report.cpp with -filename-equivalence turned on, I think writing another RUN line to check the same may be redundant.

[vsk]

In D20803#489656, @chakshugrover wrote:

I skip the recursive directory traversal if -filename-equivalence is turned on as I believe that option is meant for a completely different purpose ( Treat source files (not directories) as equivalent to paths in the coverage data even when only the file names match )

I disagree. It should retain its original meaning while applying to all files within the directory.

test/tools/llvm-cov/cov_dir.cpp

6

I pointed this out in a comment further below, but: I think there is a bug in your patch which occurs when you turn on -filename-equivalence with recursive directory search. So, in this case, I don't think the extra test I suggested is redundant.

tools/llvm-cov/CodeCoverage.cpp

96

Instead of adding this to CodeCoverageTool, you can simply call exit(0) after you're finished printing the input file list in the command line arg parser.

test/tools/llvm-cov/cov_dir.cpp

+//RUN: rm -rf %t
+//RUN: mkdir -p %t/a/b/
+//RUN: echo "" > %t/a/b/c.tmp
+//RUN: echo "" > %t/a/d.tmp
+//RUN: llvm-cov show /dev/null -instr-profile=/dev/null -dump-input-file-list %t | FileCheck %s
+//CHECK-DAG: {{.*}}c.tmp
+//CHECK-DAG: {{.*}}d.tmp

tools/llvm-cov/CodeCoverage.cpp

 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include <functional>
 #include <system_error>
 
 using namespace llvm;
 using namespace coverage;
 
 namespace {
 /// \brief The implementation of the coverage tool.
 class CodeCoverageTool {
 public:
   enum Command {
     /// \brief The show command.
     Show,
     /// \brief The report command.
     Report
   };
public:
 
   /// \brief Create the main source view of a particular source file.
   std::unique_ptr<SourceCoverageView>
   createSourceFileView(StringRef SourceFile, CoverageMapping &Coverage);
 
   /// \brief Load the coverage mapping data. Return true if an error occured.
   std::unique_ptr<CoverageMapping> load();
 
+  /// \brief Collects all source files recursively inside the directory.
+  void collectSourceFiles(StringRef Path);
+
   int run(Command Cmd, int argc, const char **argv);
 
   typedef llvm::function_ref<int(int, const char **)> CommandLineParserType;
 
   int show(int argc, const char **argv,
            CommandLineParserType commandLineParser);
 
   int report(int argc, const char **argv,
              CommandLineParserType commandLineParser);
 
   std::string ObjectFilename;
   CoverageViewOptions ViewOpts;
   std::string PGOFilename;
   CoverageFiltersMatchAll Filters;
   std::vector<StringRef> SourceFiles;
   std::vector<std::pair<std::string, std::unique_ptr<MemoryBuffer>>>
       LoadedSourceFiles;
   bool CompareFilenamesOnly;
   StringMap<std::string> RemappedFilenames;
   std::string CoverageArch;
+  bool DumpFileNames;
 
 private:
   std::vector<std::string> CollectedPaths;
 };
 }
 
 void CodeCoverageTool::error(const Twine &Message, StringRef Whence) {
   errs() << "error: ";
std::unique_ptr<CoverageMapping> CodeCoverageTool::load() {
   if (CompareFilenamesOnly) {
     auto CoveredFiles = Coverage.get()->getUniqueSourceFiles();
     for (auto &SF : SourceFiles) {
       StringRef SFBase = sys::path::filename(SF);
       for (const auto &CF : CoveredFiles)
         if (SFBase == sys::path::filename(CF)) {
           RemappedFilenames[CF] = SF;
           SF = CF;
           break;
         }
     }
   }
 
   return Coverage;
 }
 
+void CodeCoverageTool::collectSourceFiles(StringRef Path) {
+  if (CompareFilenamesOnly) {
+    addCollectedPath(Path.str());
+    return;
+  }
+  llvm::sys::fs::file_status Status;
+  llvm::sys::fs::status(Path, Status);
+  if (llvm::sys::fs::exists(Status)) {
+    // If it's a source file, collect it.
+    if (llvm::sys::fs::is_regular_file(Status)) {
+      addCollectedPath(Path.str());
+      return;
+    }
+    if (llvm::sys::fs::is_directory(Status)) {
+      std::error_code EC;
+      // Collect all source files within this directory by iterating
+      // recursively.
+      for (llvm::sys::fs::recursive_directory_iterator File(Path, EC), FileEnd;
+           File != FileEnd && !EC; File.increment(EC)) {
+        if (llvm::sys::fs::is_regular_file(File->path())) {
+          addCollectedPath(File->path());
+        }
+      }
+    }
+  } else
+    errs() << "Error: Can't access: " << Path.str().c_str() << "\n";
+}
+
 int CodeCoverageTool::run(Command Cmd, int argc, const char **argv) {
   cl::opt<std::string, true> ObjectFilename(
       cl::Positional, cl::Required, cl::location(this->ObjectFilename),
       cl::desc("Covered executable or object file."));
 
   cl::list<std::string> InputSourceFiles(
       cl::Positional, cl::desc("<Source files>"), cl::ZeroOrMore);
 
cl::opt<double> LineCoverageGtFilter(
       cl::desc("Show code coverage only for functions with line coverage "
                "greater than the given threshold"),
       cl::cat(FilteringCategory));
 
   cl::opt<cl::boolOrDefault> UseColor(
       "use-color", cl::desc("Emit colored output (default=autodetect)"),
       cl::init(cl::BOU_UNSET));
 
+  cl::opt<bool> DumpInputFileList(
+      "dump-input-file-list", cl::Optional,
+      cl::desc("Dump just filenames traversing into the directories."));
+
   auto commandLineParser = [&, this](int argc, const char **argv) -> int {
     cl::ParseCommandLineOptions(argc, argv, "LLVM code coverage tool\n");
     ViewOpts.Debug = DebugDump;
     CompareFilenamesOnly = FilenameEquivalence;
+    DumpFileNames = DumpInputFileList;
 
     ViewOpts.Format = Format;
     switch (ViewOpts.Format) {
     case CoverageViewOptions::OutputFormat::Text:
       ViewOpts.Colors = UseColor == cl::BOU_UNSET
                             ? sys::Process::StandardOutHasColors()
                             : UseColor == cl::BOU_TRUE;
       break;
auto commandLineParser = [&, this](int argc, const char **argv) -> int {
 
     for (const auto &File : InputSourceFiles) {
       SmallString<128> Path(File);
       if (!CompareFilenamesOnly)
         if (std::error_code EC = sys::fs::make_absolute(Path)) {
           errs() << "error: " << File << ": " << EC.message();
           return 1;
         }
-      addCollectedPath(Path.str());
+      collectSourceFiles(Path.str());
+    }
+
+    if (DumpFileNames) {
+      for (StringRef SF : SourceFiles)
+        outs() << SF << "\n";
+      return 0;
     }
+
     return 0;
   };
 
   switch (Cmd) {
   case Show:
     return show(argc, argv, commandLineParser);
   case Report:
     return report(argc, argv, commandLineParser);
cl::opt<std::string> ShowOutputDirectory(
       cl::desc("Directory in which coverage information is written out"));
   cl::alias ShowOutputDirectoryA("o", cl::desc("Alias for --output-dir"),
                                  cl::aliasopt(ShowOutputDirectory));
 
   auto Err = commandLineParser(argc, argv);
   if (Err)
     return Err;
 
+  if (DumpFileNames)
+    return 0;
   ViewOpts.ShowLineNumbers = true;
   ViewOpts.ShowLineStats = ShowLineExecutionCounts.getNumOccurrences() != 0 ||
                            !ShowRegions || ShowBestLineRegionsCounts;
   ViewOpts.ShowRegionMarkers = ShowRegions || ShowBestLineRegionsCounts;
   ViewOpts.ShowLineStatsOrRegionMarkers = ShowBestLineRegionsCounts;
   ViewOpts.ShowExpandedRegions = ShowExpansions;
   ViewOpts.ShowFunctionInstantiations = ShowInstantiations;
   ViewOpts.ShowOutputDirectory = ShowOutputDirectory;
 

 }
 
 int CodeCoverageTool::report(int argc, const char **argv,
                              CommandLineParserType commandLineParser) {
   auto Err = commandLineParser(argc, argv);
   if (Err)
     return Err;
 
+  if (DumpFileNames)
+    return 0;
   auto Coverage = load();
   if (!Coverage)
     return 1;
 
   CoverageReport Report(ViewOpts, std::move(Coverage));
   if (SourceFiles.empty())
     Report.renderFileReports(llvm::outs());
   else