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

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:51
+  llvm::Regex HeaderPattern(
+      "^content-length[[:space:]]*:[[:space:]]*([[:digit:]]+)[[:space:]]*$",
+      llvm::Regex::RegexFlags::IgnoreCase);
----------------
According to the spec, `Content-Length` is the only acceptable spelling so I think we could be a little more strict here and require that particular case.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:91
+
+  size_t ContentLengthParsed = std::stoll(ContentLength.str());
+  Buf->MemBuffer =
----------------
Is it possible to use `StringRef::getAsInteger` instead?


================
Comment at: llvm/test/tools/llvm-debuginfod/find-debuginfod-asset.py:8-12
+argv = sys.argv
+
+server_directory = argv[1]
+port = argv[2]
+debuginfod_find_args = argv[3:]
----------------
I'd consider using `argparse` and give the arguments more descriptive names.


================
Comment at: llvm/test/tools/llvm-debuginfod/find-debuginfod-asset.py:33
+# test the client
+client = subprocess.Popen(['llvm-debuginfod-find',
+                           'fake_build_id'] + debuginfod_find_args,
----------------
The path of the tool should be passed as a variable to this script from the test (which should get it as a lit substitution from CMake). Otherwise, you might accidentally pick up the system version which is undesirable.


================
Comment at: llvm/test/tools/llvm-debuginfod/llvm-debuginfod-find.test:2
+# REQUIRES: debuginfod_client
+# RUN: python %S/find-debuginfod-asset.py %S 13576 --executable
+# RUN: python %S/find-debuginfod-asset.py %S 13577 --debuginfo
----------------
This would ideally be a substitution.


================
Comment at: llvm/tools/llvm-debuginfod/llvm-debuginfod-find.cpp:71-79
+  const char *HomeEnv = std::getenv("HOME");
+  if (HomeEnv == NULL) {
+    LLVM_DEBUG(dbgs() << "HOME not set\n";);
+    return 1;
+  }
+  LLVM_DEBUG(dbgs() << "HOME = " << HomeEnv << "\n";);
+  SmallString<64> CacheDirectoryPath;
----------------
I think this should use `XDG_CACHE_HOME` when set instead (which typically defaults to `$HOME/.cache`), see https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html. We should also check what the macOS and Windows alternatives are. This implementation should only be used as a fallback.


================
Comment at: llvm/unittests/Debuginfod/DebuginfodTests.cpp:13
+
+TEST(DebuginfodTests, noDebuginfodUrlsFetchInfoTest) {
+  auto InfoOrErr = fetchDebuginfo("./", {}, "fakeBuildId",
----------------
I don't think this is a particularly useful test, but we should be able to improve this once we also have a server.


================
Comment at: llvm/unittests/Support/HTTPClient.cpp:15
+
+TEST(HTTPClientTests, invalidUrlTest) {
+  std::string invalidUrl = "llvm is fun";
----------------
Ditto for this one.


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

https://reviews.llvm.org/D111252