[PATCH] D112758: [llvm] [Debuginfo] Debuginfod client library.

[Noah Shutty via Phabricator via llvm-commits]

noajshu added a comment.

I integrated the client library into llvm-symbolizer (D113717 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D75750#1950734>.



================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:81
+    SmallString<64> AssetUrl;
+    sys::path::append(AssetUrl, ServerUrl, "buildid", BuildID, Suffix);
+
----------------
labath wrote:
> 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.
Good catch, thanks!


================
Comment at: llvm/unittests/Debuginfod/DebuginfodTests.cpp:13
+
+TEST(DebuginfodTests, noDebuginfodUrlsFetchInfoTest) {
+  EXPECT_THAT_EXPECTED(fetchDebuginfo("./", {}, "fakeBuildId",
----------------
labath wrote:
> 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.
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.


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

https://reviews.llvm.org/D112758