Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[llvm-cov] Add --debug-info flag to llvm-cov to lookup debug info file. #66798

Closed
wants to merge 1 commit into from

Conversation

ZequanWu
Copy link
Contributor

In https://reviews.llvm.org/D157913, I was assuming the debug info is always in the binary, which is not valid on Mac.

  1. This adds the same --debug-info flag as llvm-profdata has to llvm-cov to lookup debug info file.
  2. If a section size is 0, do not return it from lookupSections as some sections could have 0 size on Mac.

In https://reviews.llvm.org/D157913, I was assumeing the debug info is always in the binary, which is not valid on Mac.

1. This adds the same --debug-info flag as llvm-profdata has to llvm-cov to lookup debug info file.
2. If a section size is 0, do not return it from lookupSections as some sections could have 0 size on Mac.
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 19, 2023

@llvm/pr-subscribers-pgo

Changes

In https://reviews.llvm.org/D157913, I was assuming the debug info is always in the binary, which is not valid on Mac.

  1. This adds the same --debug-info flag as llvm-profdata has to llvm-cov to lookup debug info file.
  2. If a section size is 0, do not return it from lookupSections as some sections could have 0 size on Mac.

Full diff: https://github.com/llvm/llvm-project/pull/66798.diff

8 Files Affected:

  • (added) compiler-rt/test/profile/Darwin/coverage-debug-info-correlate.cpp (+78)
  • (modified) compiler-rt/test/profile/Linux/coverage-debug-info-correlate.cpp (+2-2)
  • (modified) llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h (+3-2)
  • (modified) llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h (+2-3)
  • (modified) llvm/lib/ProfileData/Coverage/CoverageMapping.cpp (+11-7)
  • (modified) llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp (+15-14)
  • (modified) llvm/tools/llvm-cov/CodeCoverage.cpp (+9-2)
  • (modified) llvm/tools/llvm-cov/CoverageViewOptions.h (+1)
diff --git a/compiler-rt/test/profile/Darwin/coverage-debug-info-correlate.cpp b/compiler-rt/test/profile/Darwin/coverage-debug-info-correlate.cpp
new file mode 100644
index 000000000000000..3849a7f57aa43d8
--- /dev/null
+++ b/compiler-rt/test/profile/Darwin/coverage-debug-info-correlate.cpp
@@ -0,0 +1,78 @@
+// Test debug info correlate with clang coverage.
+
+// Test the case when there is no __llvm_prf_names in the binary.
+// RUN: %clang_profgen -o %t.normal -fcoverage-mapping %S/../Inputs/instrprof-debug-info-correlate-main.cpp %S/../Inputs/instrprof-debug-info-correlate-foo.cpp
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t.normal
+// RUN: llvm-profdata merge -o %t.normal.profdata %t.profraw
+
+// RUN: %clang_profgen -o %t -g -mllvm --debug-info-correlate -fcoverage-mapping %S/../Inputs/instrprof-debug-info-correlate-main.cpp %S/../Inputs/instrprof-debug-info-correlate-foo.cpp
+// RUN: env LLVM_PROFILE_FILE=%t.proflite %run %t
+// RUN: llvm-profdata merge -o %t.profdata --debug-info=%t.dSYM %t.proflite
+
+// RUN: diff <(llvm-profdata show --all-functions --counts %t.normal.profdata) <(llvm-profdata show --all-functions --counts %t.profdata)
+
+// RUN: llvm-cov report --instr-profile=%t.profdata %t | FileCheck %s -check-prefix=NONAME
+
+// Test debug info correlate with clang coverage (online merging).
+
+// RUN: env LLVM_PROFILE_FILE=%t-1.profraw %run %t.normal
+// RUN: env LLVM_PROFILE_FILE=%t-2.profraw %run %t.normal
+// RUN: llvm-profdata merge -o %t.normal.profdata %t-1.profraw %t-2.profraw
+
+// RUN: rm -rf %t.profdir && mkdir %t.profdir
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/%m.proflite %run %t
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/%m.proflite %run %t
+// RUN: llvm-profdata merge -o %t.profdata --debug-info=%t.dSYM %t.profdir
+
+// RUN: diff <(llvm-profdata show --all-functions --counts %t.normal.profdata) <(llvm-profdata show --all-functions --counts %t.profdata)
+
+// RUN: llvm-cov report --instr-profile=%t.profdata %t | FileCheck %s -check-prefix=NONAME
+// RUN: llvm-cov report --instr-profile=%t.normal.profdata %t | FileCheck %s -check-prefix=NONAME
+
+// NONAME:      Filename                                    Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
+// NONAME-NEXT: --
+// NONAME-NEXT: instrprof-debug-info-correlate-bar.h              3                 1    66.67%           1                 0   100.00%           5                 1    80.00%           2                 1    50.00%
+// NONAME-NEXT: instrprof-debug-info-correlate-foo.cpp            5                 2    60.00%           2                 1    50.00%           6                 2    66.67%           2                 1    50.00%
+// NONAME-NEXT: instrprof-debug-info-correlate-main.cpp           4                 0   100.00%           1                 0   100.00%           5                 0   100.00%           2                 0   100.00%
+// NONAME-NEXT: --
+// NONAME-NEXT: TOTAL                                            12                 3    75.00%           4                 1    75.00%          16                 3    81.25%           6                 2    66.67%
+
+// Test the case when there is __llvm_prf_names in the binary (those are names of uninstrumented functions).
+// RUN: %clang_profgen -o %t.normal -fcoverage-mapping -mllvm -enable-name-compression=false %s
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t.normal
+// RUN: llvm-profdata merge -o %t.normal.profdata %t.profraw
+
+// RUN: %clang_profgen -o %t -g -mllvm --debug-info-correlate -fcoverage-mapping -mllvm -enable-name-compression=false %s
+// RUN: env LLVM_PROFILE_FILE=%t.proflite %run %t
+// RUN: llvm-profdata merge -o %t.profdata --debug-info=%t.dSYM %t.proflite
+
+// RUN: diff <(llvm-profdata show --all-functions --counts %t.normal.profdata) <(llvm-profdata show --all-functions --counts %t.profdata)
+
+// RUN: llvm-cov export --format=lcov --instr-profile=%t.profdata %t --debug-info=%t.dSYM | FileCheck %s -check-prefix=NAME
+
+// Test debug info correlate with clang coverage (online merging).
+
+// RUN: env LLVM_PROFILE_FILE=%t-1.profraw %run %t.normal
+// RUN: env LLVM_PROFILE_FILE=%t-2.profraw %run %t.normal
+// RUN: llvm-profdata merge -o %t.normal.profdata %t-1.profraw %t-2.profraw
+
+// RUN: rm -rf %t.profdir && mkdir %t.profdir
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/%m.proflite %run %t
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/%m.proflite %run %t
+// RUN: llvm-profdata merge -o %t.profdata --debug-info=%t.dSYM %t.profdir
+
+// RUN: diff <(llvm-profdata show --all-functions --counts %t.normal.profdata) <(llvm-profdata show --all-functions --counts %t.profdata)
+
+// RUN: llvm-cov export --format=lcov --instr-profile=%t.profdata %t --debug-info=%t.dSYM | FileCheck %s -check-prefix=NAME
+// NAME: _Z9used_funcv
+// NAME: main
+// NAME: _ZN1A11unused_funcEv
+
+struct A {
+  void unused_func() {}
+};
+void used_func() {}
+int main() {
+  used_func();
+  return 0;
+}
diff --git a/compiler-rt/test/profile/Linux/coverage-debug-info-correlate.cpp b/compiler-rt/test/profile/Linux/coverage-debug-info-correlate.cpp
index 06901de4a24291a..d44e258ce3a9868 100644
--- a/compiler-rt/test/profile/Linux/coverage-debug-info-correlate.cpp
+++ b/compiler-rt/test/profile/Linux/coverage-debug-info-correlate.cpp
@@ -48,7 +48,7 @@
 
 // RUN: diff <(llvm-profdata show --all-functions --counts %t.normal.profdata) <(llvm-profdata show --all-functions --counts %t.profdata)
 
-// RUN: llvm-cov export --format=lcov --instr-profile=%t.profdata %t | FileCheck %s -check-prefix=NAME
+// RUN: llvm-cov export --format=lcov --instr-profile=%t.profdata %t --debug-info=%t | FileCheck %s -check-prefix=NAME
 
 // Test debug info correlate with clang coverage (online merging).
 
@@ -63,7 +63,7 @@
 
 // RUN: diff <(llvm-profdata show --all-functions --counts %t.normal.profdata) <(llvm-profdata show --all-functions --counts %t.profdata)
 
-// RUN: llvm-cov export --format=lcov --instr-profile=%t.profdata %t | FileCheck %s -check-prefix=NAME
+// RUN: llvm-cov export --format=lcov --instr-profile=%t.profdata %t --debug-info=%t | FileCheck %s -check-prefix=NAME
 // NAME: _Z9used_funcv
 // NAME: main
 // NAME: _ZN1A11unused_funcEv
diff --git a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
index b407fe277c543b8..9a12e4e6f464173 100644
--- a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
+++ b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
@@ -594,6 +594,7 @@ class CoverageMapping {
   // Load coverage records from file.
   static Error
   loadFromFile(StringRef Filename, StringRef Arch, StringRef CompilationDir,
+               StringRef DebugInfoFilename,
                IndexedInstrProfReader &ProfileReader, CoverageMapping &Coverage,
                bool &DataFound,
                SmallVectorImpl<object::BuildID> *FoundBinaryIDs = nullptr);
@@ -623,8 +624,8 @@ class CoverageMapping {
   /// Ignores non-instrumented object files unless all are not instrumented.
   static Expected<std::unique_ptr<CoverageMapping>>
   load(ArrayRef<StringRef> ObjectFilenames, StringRef ProfileFilename,
-       vfs::FileSystem &FS, ArrayRef<StringRef> Arches = std::nullopt,
-       StringRef CompilationDir = "",
+       vfs::FileSystem &FS, StringRef DebugInfoFilename = "",
+       ArrayRef<StringRef> Arches = std::nullopt, StringRef CompilationDir = "",
        const object::BuildIDFetcher *BIDFetcher = nullptr,
        bool CheckBinaryIDs = false);
 
diff --git a/llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h b/llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
index 41935ae6bb39881..82395595eb8f89e 100644
--- a/llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
+++ b/llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
@@ -203,10 +203,9 @@ class BinaryCoverageReader : public CoverageMappingReader {
   BinaryCoverageReader &operator=(const BinaryCoverageReader &) = delete;
 
   static Expected<std::vector<std::unique_ptr<BinaryCoverageReader>>>
-  create(MemoryBufferRef ObjectBuffer,
-         StringRef Arch,
+  create(MemoryBufferRef ObjectBuffer, StringRef Arch,
          SmallVectorImpl<std::unique_ptr<MemoryBuffer>> &ObjectFileBuffers,
-         InstrProfSymtab& IndexedProfSymTab,
+         StringRef DebugInfoFilename, InstrProfSymtab &ProfSymTab,
          StringRef CompilationDir = "",
          SmallVectorImpl<object::BuildIDRef> *BinaryIDs = nullptr);
 
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index a99785ee0df5427..94bcaa98c950066 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -349,8 +349,9 @@ static Error handleMaybeNoDataFoundError(Error E) {
 
 Error CoverageMapping::loadFromFile(
     StringRef Filename, StringRef Arch, StringRef CompilationDir,
-    IndexedInstrProfReader &ProfileReader, CoverageMapping &Coverage,
-    bool &DataFound, SmallVectorImpl<object::BuildID> *FoundBinaryIDs) {
+    StringRef DebugInfoFilename, IndexedInstrProfReader &ProfileReader,
+    CoverageMapping &Coverage, bool &DataFound,
+    SmallVectorImpl<object::BuildID> *FoundBinaryIDs) {
   auto CovMappingBufOrErr = MemoryBuffer::getFileOrSTDIN(
       Filename, /*IsText=*/false, /*RequiresNullTerminator=*/false);
   if (std::error_code EC = CovMappingBufOrErr.getError())
@@ -362,7 +363,7 @@ Error CoverageMapping::loadFromFile(
 
   SmallVector<object::BuildIDRef> BinaryIDs;
   auto CoverageReadersOrErr = BinaryCoverageReader::create(
-      CovMappingBufRef, Arch, Buffers, ProfSymTab,
+      CovMappingBufRef, Arch, Buffers, DebugInfoFilename, ProfSymTab,
       CompilationDir, FoundBinaryIDs ? &BinaryIDs : nullptr);
   if (Error E = CoverageReadersOrErr.takeError()) {
     E = handleMaybeNoDataFoundError(std::move(E));
@@ -388,7 +389,8 @@ Error CoverageMapping::loadFromFile(
 
 Expected<std::unique_ptr<CoverageMapping>> CoverageMapping::load(
     ArrayRef<StringRef> ObjectFilenames, StringRef ProfileFilename,
-    vfs::FileSystem &FS, ArrayRef<StringRef> Arches, StringRef CompilationDir,
+    vfs::FileSystem &FS, StringRef DebugInfoFilename,
+    ArrayRef<StringRef> Arches, StringRef CompilationDir,
     const object::BuildIDFetcher *BIDFetcher, bool CheckBinaryIDs) {
   auto ProfileReaderOrErr = IndexedInstrProfReader::create(ProfileFilename, FS);
   if (Error E = ProfileReaderOrErr.takeError())
@@ -409,7 +411,8 @@ Expected<std::unique_ptr<CoverageMapping>> CoverageMapping::load(
   for (const auto &File : llvm::enumerate(ObjectFilenames)) {
     if (Error E =
             loadFromFile(File.value(), GetArch(File.index()), CompilationDir,
-                         *ProfileReader, *Coverage, DataFound, &FoundBinaryIDs))
+                         DebugInfoFilename, *ProfileReader, *Coverage,
+                         DataFound, &FoundBinaryIDs))
       return std::move(E);
   }
 
@@ -436,8 +439,9 @@ Expected<std::unique_ptr<CoverageMapping>> CoverageMapping::load(
       if (PathOpt) {
         std::string Path = std::move(*PathOpt);
         StringRef Arch = Arches.size() == 1 ? Arches.front() : StringRef();
-        if (Error E = loadFromFile(Path, Arch, CompilationDir, *ProfileReader,
-                                  *Coverage, DataFound))
+        if (Error E =
+                loadFromFile(Path, Arch, DebugInfoFilename, CompilationDir,
+                             *ProfileReader, *Coverage, DataFound))
           return std::move(E);
       } else if (CheckBinaryIDs) {
         return createFileError(
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
index b17caaf99807357..962202238ba3983 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -1014,7 +1014,7 @@ static Expected<std::vector<SectionRef>> lookupSections(ObjectFile &OF,
     Expected<StringRef> NameOrErr = Section.getName();
     if (!NameOrErr)
       return NameOrErr.takeError();
-    if (stripSuffix(*NameOrErr) == Name)
+    if (stripSuffix(*NameOrErr) == Name && Section.getSize() != 0)
       Sections.push_back(Section);
   }
   if (Sections.empty())
@@ -1022,10 +1022,10 @@ static Expected<std::vector<SectionRef>> lookupSections(ObjectFile &OF,
   return Sections;
 }
 
-static Error getProfileNamesFromDebugInfo(StringRef FileName,
+static Error getProfileNamesFromDebugInfo(StringRef DebugInfoFilename,
                                           InstrProfSymtab &ProfileNames) {
   std::unique_ptr<InstrProfCorrelator> Correlator;
-  if (auto E = InstrProfCorrelator::get(FileName).moveInto(Correlator))
+  if (auto E = InstrProfCorrelator::get(DebugInfoFilename).moveInto(Correlator))
     return E;
   if (auto E = Correlator->correlateCovUnusedFuncNames(0))
     return E;
@@ -1038,7 +1038,8 @@ static Error getProfileNamesFromDebugInfo(StringRef FileName,
 
 static Expected<std::unique_ptr<BinaryCoverageReader>>
 loadBinaryFormat(std::unique_ptr<Binary> Bin, StringRef Arch,
-                 InstrProfSymtab &ProfSymTab, StringRef CompilationDir = "",
+                 StringRef DebugInfoFilename, InstrProfSymtab &ProfSymTab,
+                 StringRef CompilationDir = "",
                  object::BuildIDRef *BinaryID = nullptr) {
   std::unique_ptr<ObjectFile> OF;
   if (auto *Universal = dyn_cast<MachOUniversalBinary>(Bin.get())) {
@@ -1077,9 +1078,9 @@ loadBinaryFormat(std::unique_ptr<Binary> Bin, StringRef Arch,
       lookupSections(*OF, getInstrProfSectionName(IPSK_name, ObjFormat,
                                                   /*AddSegmentInfo=*/false));
   if (auto E = NamesSection.takeError()) {
-    if (OF->hasDebugInfo()) {
+    if (!DebugInfoFilename.empty()) {
       if (auto E =
-              getProfileNamesFromDebugInfo(OF->getFileName(), ProfileNames))
+              getProfileNamesFromDebugInfo(DebugInfoFilename, ProfileNames))
         return make_error<CoverageMapError>(coveragemap_error::malformed);
     }
     consumeError(std::move(E));
@@ -1176,8 +1177,8 @@ Expected<std::vector<std::unique_ptr<BinaryCoverageReader>>>
 BinaryCoverageReader::create(
     MemoryBufferRef ObjectBuffer, StringRef Arch,
     SmallVectorImpl<std::unique_ptr<MemoryBuffer>> &ObjectFileBuffers,
-    InstrProfSymtab &ProfSymTab, StringRef CompilationDir,
-    SmallVectorImpl<object::BuildIDRef> *BinaryIDs) {
+    StringRef DebugInfoFilename, InstrProfSymtab &ProfSymTab,
+    StringRef CompilationDir, SmallVectorImpl<object::BuildIDRef> *BinaryIDs) {
   std::vector<std::unique_ptr<BinaryCoverageReader>> Readers;
 
   if (ObjectBuffer.getBuffer().size() > sizeof(TestingFormatMagic)) {
@@ -1221,8 +1222,8 @@ BinaryCoverageReader::create(
       }
 
       return BinaryCoverageReader::create(
-          ArchiveOrErr.get()->getMemoryBufferRef(), Arch,
-          ObjectFileBuffers, ProfSymTab, CompilationDir, BinaryIDs);
+          ArchiveOrErr.get()->getMemoryBufferRef(), Arch, ObjectFileBuffers,
+          DebugInfoFilename, ProfSymTab, CompilationDir, BinaryIDs);
     }
   }
 
@@ -1235,8 +1236,8 @@ BinaryCoverageReader::create(
         return ChildBufOrErr.takeError();
 
       auto ChildReadersOrErr = BinaryCoverageReader::create(
-          ChildBufOrErr.get(), Arch, ObjectFileBuffers, ProfSymTab,
-          CompilationDir, BinaryIDs);
+          ChildBufOrErr.get(), Arch, ObjectFileBuffers, DebugInfoFilename,
+          ProfSymTab, CompilationDir, BinaryIDs);
       if (!ChildReadersOrErr)
         return ChildReadersOrErr.takeError();
       for (auto &Reader : ChildReadersOrErr.get())
@@ -1257,8 +1258,8 @@ BinaryCoverageReader::create(
 
   object::BuildIDRef BinaryID;
   auto ReaderOrErr =
-      loadBinaryFormat(std::move(Bin), Arch, ProfSymTab, CompilationDir,
-                       BinaryIDs ? &BinaryID : nullptr);
+      loadBinaryFormat(std::move(Bin), Arch, DebugInfoFilename, ProfSymTab,
+                       CompilationDir, BinaryIDs ? &BinaryID : nullptr);
   if (!ReaderOrErr)
     return ReaderOrErr.takeError();
   Readers.push_back(std::move(ReaderOrErr.get()));
diff --git a/llvm/tools/llvm-cov/CodeCoverage.cpp b/llvm/tools/llvm-cov/CodeCoverage.cpp
index b5d763d8643cd7d..9a8bbfa291a679c 100644
--- a/llvm/tools/llvm-cov/CodeCoverage.cpp
+++ b/llvm/tools/llvm-cov/CodeCoverage.cpp
@@ -444,8 +444,9 @@ std::unique_ptr<CoverageMapping> CodeCoverageTool::load() {
               ObjectFilename);
   auto FS = vfs::getRealFileSystem();
   auto CoverageOrErr = CoverageMapping::load(
-      ObjectFilenames, PGOFilename, *FS, CoverageArches,
-      ViewOpts.CompilationDirectory, BIDFetcher.get(), CheckBinaryIDs);
+      ObjectFilenames, PGOFilename, *FS, ViewOpts.DebugInfoFilename,
+      CoverageArches, ViewOpts.CompilationDirectory, BIDFetcher.get(),
+      CheckBinaryIDs);
   if (Error E = CoverageOrErr.takeError()) {
     error("failed to load coverage: " + toString(std::move(E)));
     return nullptr;
@@ -774,6 +775,11 @@ int CodeCoverageTool::run(Command Cmd, int argc, const char **argv) {
       "check-binary-ids", cl::desc("Fail if an object couldn't be found for a "
                                    "binary ID in the profile"));
 
+  cl::opt<std::string> DebugInfoFilename(
+      "debug-info", cl::init(""),
+      cl::desc("Read and extract profile metadata from debug info and show "
+               "the functions it found."));
+
   auto commandLineParser = [&, this](int argc, const char **argv) -> int {
     cl::ParseCommandLineOptions(argc, argv, "LLVM code coverage tool\n");
     ViewOpts.Debug = DebugDump;
@@ -929,6 +935,7 @@ int CodeCoverageTool::run(Command Cmd, int argc, const char **argv) {
     ViewOpts.ExportSummaryOnly = SummaryOnly;
     ViewOpts.NumThreads = NumThreads;
     ViewOpts.CompilationDirectory = CompilationDirectory;
+    ViewOpts.DebugInfoFilename = DebugInfoFilename;
 
     return 0;
   };
diff --git a/llvm/tools/llvm-cov/CoverageViewOptions.h b/llvm/tools/llvm-cov/CoverageViewOptions.h
index eb852859e9cbe46..f0e37848a455059 100644
--- a/llvm/tools/llvm-cov/CoverageViewOptions.h
+++ b/llvm/tools/llvm-cov/CoverageViewOptions.h
@@ -54,6 +54,7 @@ struct CoverageViewOptions {
   std::string CompilationDirectory;
   float HighCovWatermark;
   float LowCovWatermark;
+  std::string DebugInfoFilename;
 
   /// Change the output's stream color if the colors are enabled.
   ColoredRawOstream colored_ostream(raw_ostream &OS,

@@ -774,6 +775,11 @@ int CodeCoverageTool::run(Command Cmd, int argc, const char **argv) {
"check-binary-ids", cl::desc("Fail if an object couldn't be found for a "
"binary ID in the profile"));

cl::opt<std::string> DebugInfoFilename(
"debug-info", cl::init(""),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"debug-info", cl::init(""),
"debug-info",

NIT: The default value is already the empty string

@@ -48,7 +48,7 @@

// RUN: diff <(llvm-profdata show --all-functions --counts %t.normal.profdata) <(llvm-profdata show --all-functions --counts %t.profdata)

// RUN: llvm-cov export --format=lcov --instr-profile=%t.profdata %t | FileCheck %s -check-prefix=NAME
// RUN: llvm-cov export --format=lcov --instr-profile=%t.profdata %t --debug-info=%t | FileCheck %s -check-prefix=NAME
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the binary already has debug info, we could use for correlation instead of passing the redundant --debug-info=%t, right? Of course this wouldn't work for MachO or if the binary is stripped of debug info. Is it possible (or at least easy to only require --debug-info if the provided binary doesn't already have it?

@dwblaikie
Copy link
Collaborator

Should this be able to work implicitly, same as a debugger? (presumably there's some logic (both as a concept, but concretely as code in lldb or somewhere similar) that knows how to find dsyms when they're in some standard layout (presumably binary_name has binary_name.dsym next to it, maybe some search path too)) Not sure if the debugger even has a way to explicitly tell it where a dsym is - and if it doesn't, probably not suitable to add such a thing to other debug info-consuming tools?

@@ -436,8 +439,9 @@ Expected<std::unique_ptr<CoverageMapping>> CoverageMapping::load(
if (PathOpt) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BIDFetcher->fetch(BinaryID); should return the unstripped file with debug info using either local filesystem or debuginfod, see

std::optional<std::string> BuildIDFetcher::fetch(BuildIDRef BuildID) const {

DebuginfodFetcher::fetch(ArrayRef<uint8_t> BuildID) const {

If PathOpt contains value and DebugInfoFilename is an empty string, we should be using *PathOpt as the debug info filename.

@petrhosek
Copy link
Member

Should this be able to work implicitly, same as a debugger? (presumably there's some logic (both as a concept, but concretely as code in lldb or somewhere similar) that knows how to find dsyms when they're in some standard layout (presumably binary_name has binary_name.dsym next to it, maybe some search path too)) Not sure if the debugger even has a way to explicitly tell it where a dsym is - and if it doesn't, probably not suitable to add such a thing to other debug info-consuming tools?

Yes, and llvm-cov already supports it through the BuildIDFetcher interface that @mysterymath created, we just need to wire it up.

@dwblaikie
Copy link
Collaborator

Should this be able to work implicitly, same as a debugger? (presumably there's some logic (both as a concept, but concretely as code in lldb or somewhere similar) that knows how to find dsyms when they're in some standard layout (presumably binary_name has binary_name.dsym next to it, maybe some search path too)) Not sure if the debugger even has a way to explicitly tell it where a dsym is - and if it doesn't, probably not suitable to add such a thing to other debug info-consuming tools?

Yes, and llvm-cov already supports it through the BuildIDFetcher interface that @mysterymath created, we just need to wire it up.

Should we do that ("wire it up") instead of this patch, or is this patch part of that, or is there some motivation to do both?

@ZequanWu
Copy link
Contributor Author

ZequanWu commented Sep 21, 2023

Actually, after thinking again about this. I feel we shouldn't move those unused function names from __llvm_prf_names to dwarf debug info. The reason for doing that was __llvm_prf_names is not strippable. However, with debug info correlation enabled for clang coverage, __llvm_prf_names will just have unused function names which are not used at runtime so we can make them strippable as same as __llvm_covmap and __llvm_covfun. llvm-cov could just lookup the unstripped binary for __llvm_prf_names.

I plan is to discard this change and restore __llvm_prf_names section when debug info correlation enabled with clang coverage. But before that, I want to hear about your opinions, @ellishg @petrhosek.

@ZequanWu
Copy link
Contributor Author

ZequanWu commented Sep 21, 2023

Maybe we can extend it further, make profile data section also strippable, because eventually we need to lookup __llvm_covfun and __llvm_covmap in the unstripped binary using llvm-cov for coverage results. And let llvm-cov construct the in-memory indexed profile from those 2 sections in the unstripped binary, and then creates coverage results.
That would be a different mode, not under debug info correlation.

@mysterymath
Copy link
Contributor

Should this be able to work implicitly, same as a debugger? (presumably there's some logic (both as a concept, but concretely as code in lldb or somewhere similar) that knows how to find dsyms when they're in some standard layout (presumably binary_name has binary_name.dsym next to it, maybe some search path too)) Not sure if the debugger even has a way to explicitly tell it where a dsym is - and if it doesn't, probably not suitable to add such a thing to other debug info-consuming tools?

Yes, and llvm-cov already supports it through the BuildIDFetcher interface that @mysterymath created, we just need to wire it up.

Should we do that ("wire it up") instead of this patch, or is this patch part of that, or is there some motivation to do both?

The whole BuildIDFetcher mechanism depends on the object being available for lookup via its build ID; it's common for linkers to stamp binaries in this way, but usually some specific setup has to be done to actually place the files there. So it seems somewhat orthogonal to a flag that allows directly pointing to a file.

@dwblaikie
Copy link
Collaborator

Should this be able to work implicitly, same as a debugger? (presumably there's some logic (both as a concept, but concretely as code in lldb or somewhere similar) that knows how to find dsyms when they're in some standard layout (presumably binary_name has binary_name.dsym next to it, maybe some search path too)) Not sure if the debugger even has a way to explicitly tell it where a dsym is - and if it doesn't, probably not suitable to add such a thing to other debug info-consuming tools?

Yes, and llvm-cov already supports it through the BuildIDFetcher interface that @mysterymath created, we just need to wire it up.

Should we do that ("wire it up") instead of this patch, or is this patch part of that, or is there some motivation to do both?

The whole BuildIDFetcher mechanism depends on the object being available for lookup via its build ID; it's common for linkers to stamp binaries in this way, but usually some specific setup has to be done to actually place the files there. So it seems somewhat orthogonal to a flag that allows directly pointing to a file.

OK, so I'd still come back to my original question: How does lldb currently find dsyms for binaries just on the local filesystem, and can llvm-cov use the same logic? (ideally literally share the same code, but if not that, then at least equivalent lookup rules) - I imagine llvm-symbolizer already has such logic as well for the same reason, so share that, maybe? Looks like llvm-symbolizer has a dsym-hint flag, so perhaps llvm-cov could do the same thing? Be good to find some consistency with existing tools - lldb, symbolizer, etc.

@dtellenbach
Copy link
Member

OK, so I'd still come back to my original question: How does lldb currently find dsyms for binaries just on the local filesystem, and can llvm-cov use the same logic? (ideally literally share the same code, but if not that, then at least equivalent lookup rules) - I imagine llvm-symbolizer already has such logic as well for the same reason, so share that, maybe? Looks like llvm-symbolizer has a dsym-hint flag, so perhaps llvm-cov could do the same thing? Be good to find some consistency with existing tools - lldb, symbolizer, etc.

On macOS you'd use UUIDs to look these things up by invoking dsymForUUID. Here's how lldb does it:

static FileSpec GetDsymForUUIDExecutable() {

Also note that ELF supports external debug info so the premise

I was assuming the debug info is always in the binary,

seems to be wrong for at least MachO and ELF.

@dwblaikie
Copy link
Collaborator

OK, so I'd still come back to my original question: How does lldb currently find dsyms for binaries just on the local filesystem, and can llvm-cov use the same logic? (ideally literally share the same code, but if not that, then at least equivalent lookup rules) - I imagine llvm-symbolizer already has such logic as well for the same reason, so share that, maybe? Looks like llvm-symbolizer has a dsym-hint flag, so perhaps llvm-cov could do the same thing? Be good to find some consistency with existing tools - lldb, symbolizer, etc.

On macOS you'd use UUIDs to look these things up by invoking dsymForUUID. Here's how lldb does it:

static FileSpec GetDsymForUUIDExecutable() {

Fair enough - presumably we have some code in llvm-symbolizer or some other LLVM tools that does something like this too? Otherwise we could move the lldb code into somewhere in LLVM to reuse.

Also note that ELF supports external debug info so the premise

Ish - there's been some support for using debuginfod to retrieve stripped debug info on ELF. Not sure if there's a clear abstraction over all these lookup mechanisms. Might be worth making one, or maybe they're significantly different codepaths anyway.

@ZequanWu
Copy link
Contributor Author

Abandoned in favor of #69493

@ZequanWu ZequanWu closed this Dec 20, 2023
@ZequanWu ZequanWu deleted the fix-cov-mac branch February 15, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler-rt PGO Profile Guided Optimizations tools:llvm-cov
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants