This is an archive of the discontinued LLVM Phabricator instance.

[llvm] [Debuginfo] Debuginfod client library.
ClosedPublic

Authored by noajshu on Oct 28 2021, 2:15 PM.

Details

Summary

This adds a Debuginfod client library which queries servers specified by the DEBUGINFOD_URLS environment variable for the debuginfo, executable, or a specified source file associated with a given build id.

This diff was split out from D111252.

Diff Detail

Event Timeline

noajshu created this revision.Oct 28 2021, 2:15 PM
noajshu requested review of this revision.Oct 28 2021, 2:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2021, 2:15 PM
noajshu updated this revision to Diff 384284.Nov 2 2021, 5:18 PM
noajshu added a reviewer: labath.

Update for refactored HTTP client architecture.

noajshu updated this revision to Diff 384291.Nov 2 2021, 5:43 PM

remove extraneous files related to debuginfod-find tool

noajshu updated this revision to Diff 384312.Nov 2 2021, 7:55 PM

move CURL cmake option update to D112753

noajshu updated this revision to Diff 384318.Nov 2 2021, 8:02 PM

Clean up unrelated changes.

labath added inline comments.Nov 3 2021, 2:45 AM
llvm/lib/Debuginfod/Debuginfod.cpp
82

This isn't really appropriate as it would use a backslash on windows. I suppose it would be ok if you explicitly specify the posix path style as an argument.

llvm/unittests/Debuginfod/DebuginfodTests.cpp
14

I suppose you could test here that the asset can be retrieved from the cache without hitting the server, but I don't think that's really necessary (if you have such a test in the next patch) -- a lot of library code in llvm is only tested through the relevant tool.

noajshu updated this revision to Diff 384640.Nov 3 2021, 7:34 PM
noajshu marked an inline comment as done.

Fix backslashes on windows.

labath added inline comments.Nov 5 2021, 7:50 AM
llvm/lib/Debuginfod/CMakeLists.txt
2

This may be the first case, where the very existence of a component library is predicated on the availability of a third party dep. That's not necessarily a bad thing (it'd also be strange to enable it when it is completely non-functional), but it did catch my eye.

noajshu updated this revision to Diff 386668.Nov 11 2021, 2:28 PM

Refactor and add several convenience functions.

I integrated the client library into llvm-symbolizer (D113717) and found that a lot of boilerplate was required. So I refactored the interface to make it easier to use.
Now all the functionality is in standalone functions. These would be easy to wrap in an object oriented interface similar to the SymbolServer used in LLDB.

llvm/lib/Debuginfod/Debuginfod.cpp
82

Good catch, thanks!

llvm/unittests/Debuginfod/DebuginfodTests.cpp
14

Currently there is no such test in the next patch. There are only lit tests where the cache is emptied each time and we fetch assets from a server. Is it OK to write to the filesystem in a unit test? If so I would like to add one here per your suggestion. If not, I can add a lit test to make sure local caching works properly.

phosek added inline comments.Nov 18 2021, 2:50 PM
llvm/include/llvm/Debuginfod/Debuginfod.h
41

I'd consider using const ArrayRef<uint8_t> BuildID and do the conversion to string internally (in which case even buildIDToString could be an internal function).

dblaikie added inline comments.
llvm/lib/Debuginfod/CMakeLists.txt
2

Yeah, I had mixed feelings about this with the machine learning - if the API is going to always be exposed, maybe the API should be in the library, and it gets compiled into a trivially uninteresting library that always fails the query - rather than having the entry point outside the library, and having that entry point not call into the library if it isn't available/couldn't do something real?

llvm/lib/Debuginfod/Debuginfod.cpp
35

You can drop the top level const here and in similar places.

noajshu updated this revision to Diff 388631.Nov 19 2021, 2:35 PM
noajshu marked 3 inline comments as done.

Add a typedef ArrayRef<uint8_t> BuildID; and change external API of debuginfod library to require. Also remove unnecessary const qualifiers from args.

llvm/lib/Debuginfod/CMakeLists.txt
2

It should work this way right now. When built without curl, the debuginfod client will still be able to retrieve artifacts from the local cache.

llvm/lib/Debuginfod/Debuginfod.cpp
35

Thanks!

noajshu updated this revision to Diff 388798.Nov 21 2021, 11:18 PM

Introduce BuildIDRef and enable custom cache directory.

dblaikie added inline comments.Nov 22 2021, 4:59 PM
llvm/lib/Debuginfod/CMakeLists.txt
2

Hmm, I guess I'm not sure what @labath was referring to with "existence of a component library is predicated on the availability of a third party dep" - perhaps the comment is out of date? Which component library did you have in mind, @labath?

labath added inline comments.Nov 22 2021, 11:44 PM
llvm/lib/Debuginfod/CMakeLists.txt
2

This component. In the earlier versions of this patch the entire file was wrapped in if(LLVM_ENABLE_CURL).

dblaikie added inline comments.Nov 23 2021, 12:16 PM
llvm/lib/Debuginfod/CMakeLists.txt
2

Ah, OK!

noajshu updated this revision to Diff 390206.Nov 28 2021, 8:59 AM

Update to use DEBUGINFOD_TIMEOUT and DEBUGINFOD_CACHE_PATH env vars and add unit tests for cache hit / miss.

labath added inline comments.Nov 29 2021, 12:48 AM
llvm/include/llvm/Debuginfod/Debuginfod.h
43

how about returning std::chrono::milliseconds instead?

llvm/lib/Debuginfod/Debuginfod.cpp
33

Now you have both a using llvm declarating and you're putting everything inside the llvm namespace as well. :)

149

since we're passing them by value, I'd say we can also iterate over them in this way...

dblaikie added inline comments.Nov 29 2021, 1:29 PM
llvm/lib/Debuginfod/Debuginfod.cpp
67

I guess TimeoutMS is actually a timeout in seconds? (or it's in ms and shouldn't be multiplied by 1000)?

noajshu updated this revision to Diff 390476.Nov 29 2021, 2:39 PM
noajshu marked 3 inline comments as done.

Convert timeout for debuginfod client library to be in std::chrono::milliseconds, miscellaneous typo fixes.

noajshu added inline comments.
llvm/include/llvm/Debuginfod/Debuginfod.h
43

Sure, I've updated this. By the way, do you think the HTTPClient.setTimeout should accept std::chrono::milliseconds as well? If so I can update D112751 and D112753. Otherwise, it just converts back to an integer using .count() before passing to setTimeout.

llvm/lib/Debuginfod/Debuginfod.cpp
67

Good catch. I converted to use std::chrono::milliseconds as @labath suggested and also removed the MS suffix here.

149

ok thanks, this makes sense!

labath added inline comments.Nov 30 2021, 1:46 AM
llvm/include/llvm/Debuginfod/Debuginfod.h
43

Yes, I'd use it in all APIs that we control, and only convert it to int (or whatever) when we're passing it to curl. That way we can avoid questions like https://reviews.llvm.org/D112758#inline-1095701

noajshu updated this revision to Diff 390793.Nov 30 2021, 12:47 PM
noajshu marked an inline comment as done.

Update HTTPClient::setTimeout to accept std::chrono::milliseconds and use empty vector of URLs when DEBUGINFOD_URLS is not set.

noajshu marked 3 inline comments as done.Dec 3 2021, 10:58 AM
noajshu added inline comments.
llvm/unittests/Debuginfod/DebuginfodTests.cpp
14

Thanks for the suggestion! I've added this test below.
How about we divide up the testing responsibility like this:
D112758 -- (this diff) -- Unit test a local cache hit and miss (done)
D112759 -- (llvm-debuginfod-find) -- end-to-end test of local cache and server using Python simple http server
D113717 -- (symbolizer integration) -- end-to-end test of local cache only (just to hit the integration point)

The goal would be to get full coverage (local cache, network, client integration point) without too much overlap across the tests.

dblaikie accepted this revision.Dec 3 2021, 2:20 PM
dblaikie added inline comments.
llvm/unittests/Debuginfod/DebuginfodTests.cpp
14

Sounds good to me (I guess it's infeasible to test the network case in this patch (D112758) because it's a bunch more code to have a C++ server compared to the python http server that can be used once it's an actual command line tool, lit tested, etc as with llvm-debuginfod-find)

This revision is now accepted and ready to land.Dec 3 2021, 2:20 PM
noajshu marked an inline comment as done.Dec 5 2021, 7:38 PM
This revision was landed with ongoing or failed builds.Dec 5 2021, 8:45 PM
This revision was automatically updated to reflect the committed changes.
noajshu edited the summary of this revision. (Show Details)Dec 5 2021, 9:30 PM
noajshu updated this revision to Diff 391967.Dec 5 2021, 10:32 PM

Fix moving error returns to address buildbot failure.

This revision was landed with ongoing or failed builds.Dec 6 2021, 10:01 AM
RKSimon added inline comments.
llvm/unittests/Debuginfod/DebuginfodTests.cpp
41

@noajshu This is failing on windows builds - please can you revert?

E:\llvm\llvm-project\llvm\unittests\Debuginfod\DebuginfodTests.cpp(40): error C3861: 'setenv': identifier not found
noajshu marked an inline comment as done.Dec 6 2021, 11:29 AM
noajshu added inline comments.
llvm/unittests/Debuginfod/DebuginfodTests.cpp
41

Thank you. Reverted.

noajshu updated this revision to Diff 392139.EditedDec 6 2021, 11:36 AM
noajshu marked an inline comment as done.

Add _putenv_s workaround for unit testing cache path support on Windows.

Due to the passing x64 windows check with the setenv fix I'm going to try committing this again and watch for any issues.

estewart08 added inline comments.
llvm/unittests/Debuginfod/DebuginfodTests.cpp
35

This eventually returns a path to $HOME via a call to cache_directory. I ran into some permission issues on a CI build. Is it safe to assume that $HOME is always writable?

Should this test set XDG_CACHE_HOME instead?

setenv("XDG_CACHE_HOME", "/tmp", /*replace=*/1);
noajshu added inline comments.Dec 15 2021, 11:24 AM
llvm/unittests/Debuginfod/DebuginfodTests.cpp
35

Hi @estewart08 thanks for pointing this out. I think you're actually hitting that issue for the next test starting on line 42.

45

Needs to set DEBUGINFOD_CACHE_PATH
Does D115813 LGTY @estewart08 ?

estewart08 added inline comments.Dec 15 2021, 1:52 PM
llvm/unittests/Debuginfod/DebuginfodTests.cpp
45

My mistake, yes this is the correct portion of the test. I will go review D115813.