Details
- Reviewers
gulfem jhenderson MaskRay davidxl ellis - Commits
- rG0a51eeda1ef4: [NFC] [llvm-cov] Remove unnecessary logic from llvm-cov debuginfod.
rG0eb01a9c4581: [llvm-cov] Add split-file to compiler-rt test requirements.
rG9977b65ae240: [llvm-cov] Fix logic error in debuginfod lookup.
rGa3b0dde4edb9: Reland: [llvm-cov] Look up object files using debuginfod
rGefbc8bb18eda: [llvm-cov] Look up object files using debuginfod
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Couple of drive-by comments, as my Herald rule triggered due to the llvm-objdump/llvm-symbolizer changes (which look fine to me).
llvm/lib/Debuginfod/Debuginfod.cpp | ||
---|---|---|
61 | In other parts of the codebase, consumeError without any explanation is generally a code smell. A comment explaining why it's appropriate instead of returning the error up the stack would be worthwhile here. | |
llvm/tools/llvm-cov/CodeCoverage.cpp | ||
659 | There seems to be an unfortunate mixture of help descriptions ending with full stops and others that don't. It seems no full stop in this file is more prevalent, so please remove it. |
Address review comments.
Refactor getDefaultDebuginfodUrls.
llvm/lib/Debuginfod/Debuginfod.cpp | ||
---|---|---|
61 | Several of the debuginfod functions return Expected even though they have no possibility of failure, which seems like the root of the smell. I've refactored this call to just return a value. |
Thanks for the minor changes. I'll leave reviewing the rest of this to someone else with more applicable knowledge.
llvm/lib/Debuginfod/Debuginfod.cpp | ||
---|---|---|
61 | Cool, looks good. |
llvm/docs/CommandGuide/llvm-cov.rst | ||
---|---|---|
352 | Can we add a test that uses this option? Maybe a test to make sure we can disable it without problem. | |
llvm/tools/llvm-cov/CodeCoverage.cpp | ||
658 | I personally prefer cl::ZeroOrMore rather than cl::Optional because I believe cl::Optional will fail if this option occurs twice. Does that make sense for these options? |
Addressed review comments.
Testing this patch using file:// URLs uncovered an minor issue in the
Debuginfod client. Fixed this, allowing file URLs to work.
llvm/tools/llvm-cov/CodeCoverage.cpp | ||
---|---|---|
658 | SGTM; I'm generally a fan of the the last-flag-wins semantics as well. |
compiler-rt/test/profile/Linux/binary-id-debuginfod.c | ||
---|---|---|
13 ↗ | (On Diff #482976) | Should this be -debuginfod instead of -debuginfod=0? Are you trying to enable debuginfod option here? |
compiler-rt/test/profile/Linux/binary-id-lookup.c | ||
15 | Can you extend both tests to check the line and coverage numbers instead of just checking the function names? | |
llvm/docs/CommandGuide/llvm-cov.rst | ||
356 | Maybe "but its object file is not provided on the command line"? | |
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp | ||
448 | 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? | |
llvm/tools/llvm-cov/CodeCoverage.cpp | ||
768 | 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. |
Can you remove the dot at the end of the commit message? ([llvm-cov] Look up object files using debuginfod.)
Done.
compiler-rt/test/profile/Linux/binary-id-debuginfod.c | ||
---|---|---|
13 ↗ | (On Diff #482976) | 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. |
llvm/docs/CommandGuide/llvm-cov.rst | ||
356 | Tightened up the wording here along those lines. Done. | |
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp | ||
448 | 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. | |
llvm/tools/llvm-cov/CodeCoverage.cpp | ||
768 | 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. |
@mysterymath this change is causing problems on several bots related to your detection on whether curl is present or not. Can you take a look and revert if you need time to investigate?
- https://lab.llvm.org/buildbot/#/builders/247/builds/884
- https://lab.llvm.org/buildbot/#/builders/231/builds/7688
- https://lab.llvm.org/buildbot/#/builders/121/builds/27389
- https://lab.llvm.org/buildbot/#/builders/230/builds/8464
- https://lab.llvm.org/buildbot/#/builders/57/builds/24209
- https://lab.llvm.org/buildbot/#/builders/127/builds/42722
@mysterymath I reverted this change as well as follow-up change you made in order to unblock the build bots.
compiler-rt/test/profile/lit.site.cfg.py.in | ||
---|---|---|
7 ↗ | (On Diff #492252) | You need to pythonize this variable. For this to work with the runtimes build, we also need to export LLVM_ENABLE_CURL in LLVMConfig.cmake, see https://github.com/llvm/llvm-project/blob/7f48154ca11fee6579fe29a51ab2a329424d31c4/llvm/cmake/modules/LLVMConfig.cmake.in#L81 There's a broader question of whether we should be passing the variables like LLVM_ENABLE_CURL to the runtimes in the first build since they are only relevant to the LLVM build. A better approach would be to provide some indication whether debuginfod is available in the llvm-cov --help output (or perhaps have a dedicated flag) and then parse that information when running compiler-rt CMake build and use the result here. |
compiler-rt/test/profile/lit.site.cfg.py.in | ||
---|---|---|
7 ↗ | (On Diff #492252) |
It looks like compiler-rt does this directly in cmake; I'll add this there for consistency.
Seems like the capabilities of the LLVM build are relevant to the compiler-rt tests, since they control the capabilities of the tools used in those tests. We could add optional capability advertisement generally to utilities, but I think one would probably want a principled way to do this; either something centralized like llvm-config, or a common flag to extract out optional capabilities in some format or another. |
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp | ||
---|---|---|
422 | We're getting build errors here (and the similar call to std::unique on line 426) with -Wunused-result. This looks like a bug: std::unique returns the new end of the range, and typically you need to call container.erase(new_end, container.end()) after the call to std::unique to reduce the container size to match the new logical size. See https://en.cppreference.com/w/cpp/algorithm/unique. |
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp | ||
---|---|---|
422 | Thanks; did a quick fix forward for this. This uncovered a mistake with Compare as well; std::unique requires a == comparator, while sort and set_difference a < comparator. |
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp | ||
---|---|---|
422 | Is there test coverage for these bugs/fixes? |
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp | ||
---|---|---|
422 | Partially; the unused result lint caught the issue with std::unique; fixing that caused the original test cases to fail due to the comparator issue. |
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp | ||
---|---|---|
422 | Could you see about adding a test case that would've failed with the code as originally committed? (something that demonstrates the need for the unique call in the first place) |
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp | ||
---|---|---|
422 | I took a closer look at the gulfem's preceding change (https://reviews.llvm.org/D135929), and it appears that the std::unique here is superfluous; there's no effective way to produce duplicate build IDs in ProfileBinaryIDs, and in that case, they're harmless in FoundBinaryIDs, since std::max(1-k, 0) is 0 for any k >= 1. The ProfileBinaryIDs sort is also superfluous. That does mean there's intrinsically no way to test it, since it makes no observable difference. I've thus removed the uniquing logic completely. |
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp | ||
---|---|---|
422 | ah, excellent - the best code is no code. Could you provide a link here to the commit hash/etc that removed it, for the record? |
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp | ||
---|---|---|
422 |
Can you extend both tests to check the line and coverage numbers instead of just checking the function names?