[PATCH] D136702: [llvm-cov] Look up object files using debuginfod.

[Daniel Thornburgh via Phabricator via llvm-commits]

mysterymath added a comment.

In D136702#4053086 <https://reviews.llvm.org/D136702#4053086>, @gulfem wrote:

> Can you remove the dot at the end of the commit message? ([llvm-cov] Look up object files using debuginfod.

Done.



================
Comment at: compiler-rt/test/profile/Linux/binary-id-debuginfod.c:13
+// RUN: echo "bad" > %t/buildid/abcd1234/debuginfo
+// RUN: env DEBUGINFOD_URLS=file://%t llvm-cov show -instr-profile %t.profdata -debuginfod=0 %t.main | FileCheck %s --check-prefix=MAINONLY
+
----------------
gulfem wrote:
> Should this be `-debuginfod` instead of `-debuginfod=0`? Are you trying to enable `debuginfod` option here?
Changed this to -debuginfod=false and renamed the CHECK to NODEBUGINFOD to clarify. debuginfod should default on when compiled with curl and if DEBUGINFOD_URLS is specified.


================
Comment at: llvm/docs/CommandGuide/llvm-cov.rst:356
+attempted by default for binary IDs present in the profile but not provided on
+the command line, so long as debuginfod is compiled in and configured via
+DEBUGINFOD_URLS.
----------------
gulfem wrote:
> Maybe "but its object file is not provided on the command line"?
Tightened up the wording here along those lines. Done.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:447
   // had coverage data. Return an error in the latter case.
   if (!DataFound && !ObjectFilenames.empty())
     return createFileError(
----------------
gulfem wrote:
> What happens when `-debug-file-directory` option is provided and there is no `debug` file that includes coverage data? For example, what happens if an empty directory or a directory with uninstrumented binaries is provided? When the object file that is provided does not contain any coverage instrumentation, this line should catch that it is a `no_data_found` error. With the new changes, I think this line would not be an error because `ObjectFilenames` will be empty if we only provide `-debug-file-directory`. So, I think it won't be able report any coverage, but it won't report an error. That's why I suggest adding a test with empty directory and check the expected behavior? 
> 
I was originally worried about changing the behavior of this line WRT when ObjectFilenames were empty, but looking at the broader codebase, it doesn't look like the empty ObjectFilename case was ever possible. An early exit occured on empty ObjectFilenames before this line on every path that called CoverageMapping::load.

Accordingly, I've just changed this to emit an error if no data was found. I've also added a check for this error.


================
Comment at: llvm/tools/llvm-cov/CodeCoverage.cpp:770
+    } else {
+      BIDFetcher = std::make_unique<object::BuildIDFetcher>(DebugFileDirectory);
+    }
----------------
gulfem wrote:
> Should we only create a `BIDFetcher` when `-debuginfod` or `-debug-file-directory` options are provided? This seems to create a BIDFetcher by default even if those options are not provided.
That's intentional, to parallel how the other utilities in llvm work. There are system default directories provided by Linux distributions that contain DWARF object files whenever debug packages are installed; these directories are implicitly present in the `-debug-file-directory` list, and the default BIDFetcher will pick these up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136702