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

[Gulfem Savrun Yeniceri via Phabricator via llvm-commits]

gulfem added inline comments.


================
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
+
----------------
Should this be `-debuginfod` instead of `-debuginfod=0`? Are you trying to enable `debuginfod` option here?


================
Comment at: compiler-rt/test/profile/Linux/binary-id-lookup.c:14
+
+// CHECK: foo
+// CHECK: bar
----------------
Can you extend both tests to check the line and coverage numbers instead of just checking the function names? 


================
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.
----------------
Maybe "but its object file is not provided on the command line"?


================
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(
----------------
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? 



================
Comment at: llvm/tools/llvm-cov/CodeCoverage.cpp:770
+    } else {
+      BIDFetcher = std::make_unique<object::BuildIDFetcher>(DebugFileDirectory);
+    }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136702