[PATCH] D111252: [llvm] [Support] [Debuginfo] Add http and debuginfod client libraries and llvm-debuginfod-find tool

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: llvm/CMakeLists.txt:975-977
+# make this boolean usable in lit config
+llvm_canonicalize_cmake_booleans(LLVM_ENABLE_CURL)
+
----------------
Why isn't this next to the other llvm_canonicalize_cmake_booleans calls?

(It would also help if you upload the full context with your patch (`arcanist` does it automatically, and you can achieve it with `git diff -U9999` if you do it manually).


================
Comment at: llvm/include/llvm/Debuginfod/Debuginfod.h:36
+/// may optionally be provided for further processing of the fetched asset.
+Expected<SmallString<64>> fetchDebuginfo(
+    StringRef CacheDirectoryPath, ArrayRef<StringRef> DebuginfodUrls,
----------------
Small strings are typically not [[ https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallstring-h | returned by value ]], and I don't think that the performance of this is particularly critical (if it was, you'd probably use a `SmallVectorImpl<char>&` by-ref argument), so I'd probably use a std::string here.


================
Comment at: llvm/lib/Support/CMakeLists.txt:79
+if (LLVM_ENABLE_CURL)
+  set(system_libs ${system_libs} ${CURL_LIBRARIES})
+endif()
----------------
noajshu wrote:
> labath wrote:
> > Who sets `CURL_LIBRARIES` ?
> When `LLVM_ENABLE_CURL` is on, `llvm/cmake/config-ix.cmake` calls `find_package(CURL)` which [[ https://cmake.org/cmake/help/latest/module/FindCURL.html | sets the ]] `CURL_LIBRARIES`. This was added in D111238.
I see. In that case, I think it would be good to also add `set(LLVM_ENABLE_CURL ... CACHE)` somewhere, so that this appears as a user-settable value in UIs and such.


================
Comment at: llvm/tools/llvm-debuginfod/llvm-debuginfod-find.cpp:110
+
+#undef DEBUG_TYPE
----------------
it seems `#undef DEBUG_TYPE` is rarely used in cpp files (headers are a different story), and when it is, it's usually because it's `#define`d  to a different files immediately afterwards. Undefining at the end of a file seems pointless.


================
Comment at: llvm/tools/llvm-debuginfod/llvm-debuginfod-find.cpp:78
+      !sys::path::cache_directory(CacheDirectoryPath))
+    CacheDirectoryPath = ".";
+
----------------
noajshu wrote:
> labath wrote:
> > When does the current directory fallback kick in? Is it actually useful? Should you exit instead?
> The fallback would kick in when `cache_directory` comes up empty-handed. This can happen on linux if neither `$XDG_CACHE_HOME` nor `$HOME` are in the environment. I would have no problem with removing the fallback and failing in this case as the user can always specify the current directory using `--cache-dir` anyways.
an unset HOME variable is going to be most likely an accident, and i think the usage of CWD would be surprising in that case. So, I'd leave the fallback out, but this is your tool, so I'm going to leave that up to you.


================
Comment at: llvm/unittests/Support/HTTPClient.cpp:15
+
+TEST(HTTPClientTests, invalidUrlTest) {
+  std::string invalidUrl = "llvm is fun";
----------------
noajshu wrote:
> labath wrote:
> > phosek wrote:
> > > Ditto for this one.
> > This would be better done as `EXPECT_THAT_EXPECTED(httpGet(...), Failed<StringError>())` or even `FailedWithMessage("whatever")`, though I agree that this is not very useful.
> > 
> > I think the interesting question is whether we're ok with not having any coverage for the lower level apis, and I'm not really sure what the answer to that is.
> Thanks, updated!
> I agree with the comments that these unit tests are not adding much value. One option until we have a server in llvm would be to GET http://llvm.org, although that does require an internet connection. By the lower level APIs, are you referring to the static callback functions used to implement `httpGet`, or to the CURL library itself?
I mean the `httpGet` function itself -- we generally don't test private implementation details (directly).
In an ideal world I'd split this patch into two (or even three, with the first part being the introduction of httpGet), and each would come with it's own tests. Testing the error message is nice to have, but it just scratches the surface. In httpGet, the content-length handling seems interesting to test, for example. 

But yes, you'd need some way to create/mock the server connection for that to work...


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

https://reviews.llvm.org/D111252