[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 a comment.

I only did an initial pass over the implementation. Some of the patterns are to be repeated throughout the code, I usually left only one comment for the first occurrence but the comment applies generally (for example the use of `auto`). I'd recommend reading through https://llvm.org/docs/CodingStandards.html which goes into more detail.



================
Comment at: llvm/include/llvm/Support/Debuginfod.h:1
+#ifdef LLVM_ENABLE_DEBUGINFOD_CLIENT
+
----------------
Every new file should have the LLVM license header, see https://llvm.org/docs/CodingStandards.html#file-headers


================
Comment at: llvm/include/llvm/Support/Debuginfod.h:10
+namespace llvm {
+namespace debuginfod {
+
----------------
We usually avoid nested namespace, I don't think in this case it should be necessary.


================
Comment at: llvm/include/llvm/Support/Debuginfod.h:12
+
+enum AssetType { Executable, Debuginfo, Source };
+
----------------
This should be either `enum class` (that would my preference) or the members should be prefixed with `AT_` to avoid name collision. 


================
Comment at: llvm/include/llvm/Support/Debuginfod.h:20-25
+  AssetCache(StringRef CacheDirectoryPath);
+  Expected<bool> contains(StringRef RelPath);
+  Error add(StringRef Key, StringRef Contents);
+  Error prune();
+  std::string absolutePath(StringRef Key);
+  Expected<std::string> contentsOf(StringRef Key);
----------------
Can you leave newlines between methods to visually separate them. Each method and function should also have a comment briefly explaining its purpose.


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:10-14
+struct Response {
+  long Code = 0;
+  std::string Body;
+};
+Expected<Response> get(const Twine &Url);
----------------
Can you visually separate things with newlines?


================
Comment at: llvm/lib/Support/CMakeLists.txt:144
   DebugCounter.cpp
+  Debuginfod.cpp
   DeltaAlgorithm.cpp
----------------
I think that Debuginfod should be a separate library rather than part of Support.


================
Comment at: llvm/lib/Support/Debuginfod.cpp:19-21
+AssetCache::AssetCache(StringRef CacheDirectoryPath) {
+  DirectoryPath = CacheDirectoryPath;
+}
----------------
This constructor can be inlined into header.


================
Comment at: llvm/lib/Support/Debuginfod.cpp:21
+  DirectoryPath = CacheDirectoryPath;
+}
+Expected<bool> AssetCache::contains(StringRef Key) {
----------------
The same in this file, can you separate methods by newlines?


================
Comment at: llvm/lib/Support/Debuginfod.cpp:28
+  LLVM_DEBUG(dbgs() << "checking if asset cache contains Key " << Key << "\n";);
+  auto AbsolutePath = absolutePath(Key);
+  file_status Status;
----------------
The LLVM style guide says to only use `auto` where the type is clear from the context, see https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable, in this case it's not the case so I'd prefer spelling it out. This also applies to many other uses of `auto` throughout this patch.


================
Comment at: llvm/lib/Support/Debuginfod.cpp:42
+  LLVM_DEBUG(dbgs() << "creating Root = " << Root << "\n";);
+  create_directories(Root);
+  if (Error Err = prune())
----------------
You should check the return value.


================
Comment at: llvm/lib/Support/Debuginfod.cpp:58
+}
+Expected<std::string> contentsOfFile(StringRef AbsolutePath) {
+
----------------
I think that MemoryBuffer would be better as a return value than `std::string`.


================
Comment at: llvm/lib/Support/Debuginfod.cpp:101-103
+  const std::vector<std::string> hexDigits = {"0", "1", "2", "3", "4", "5",
+                                              "6", "7", "8", "9", "a", "b",
+                                              "c", "d", "e", "f"};
----------------
There's `format_hex` in `llvm/include/llvm/Support/Format.h`.


================
Comment at: llvm/lib/Support/Debuginfod.cpp:107
+    unsigned int i2 = (int)c >> 4;
+    Output += hexDigits.at(i1);
+    Output += hexDigits.at(i2);
----------------
I'd use `raw_string_ostream`, see https://github.com/llvm/llvm-project/blob/3fe7fe44249b0c640031a09800f3485a06a61d2d/llvm/tools/llvm-readobj/ELFDumper.cpp#L4969.


================
Comment at: llvm/lib/Support/Debuginfod.cpp:116
+  std::string HexEncodedKey = hexEncode(Key.str());
+  auto AbsPath = DirectoryPath + "/llvmcache-debuginfod-" + HexEncodedKey;
+  LLVM_DEBUG(dbgs() << "HexEncodedKey = " << HexEncodedKey << "\n";);
----------------
Prefer `llvm::sys::path::append` when combining path components.


================
Comment at: llvm/lib/Support/Debuginfod.cpp:127
+                    << DebuginfodUrls.size() << "\n";);
+  std::string Suffix;
+  if (Type == Executable) {
----------------
This could be `SmallString` so it's allocated on the stack.


================
Comment at: llvm/lib/Support/Debuginfod.cpp:128-137
+  if (Type == Executable) {
+    Suffix = "executable";
+  } else if (Type == Debuginfo) {
+    Suffix = "debuginfo";
+  } else if (Type == Source) {
+    // Description is the absolute source path
+    Suffix = "source" + Description.str();
----------------
This should be a `switch` to make sure you don't miss any case.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:24
+  size_t realsize = size * nmemb;
+  struct MemoryStruct *mem = (struct MemoryStruct *)userp;
+
----------------
This code should be using C++ casts, not C casts.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:44
+
+  struct MemoryStruct chunk;
+
----------------
I'd use a `std::vector` here and resize it inside the callback as needed. That's not only more ergonomic, but also automatically releases memory.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:52
+  if (!Curl) {
+    return createStringError(errc::io_error, "http library error");
+  }
----------------
This leaks `chunk.memory`.


================
Comment at: llvm/test/lit.site.cfg.py.in:47
 config.have_zlib = @LLVM_ENABLE_ZLIB@
+set_default("enable_debuginfod_client", lit.util.pythonize_bool("@LLVM_ENABLE_DEBUGINFOD_CLIENT@"))
 config.have_libxar = @LLVM_HAVE_LIBXAR@
----------------
This kind of logic should live in CMake, this file should only have simple assignment as is done for other values.


================
Comment at: llvm/test/tools/llvm-debuginfod/find-test.sh:1
+set -x
+
----------------
Using shell for this test makes it non-portable across platforms (for example there may not be any shell on Windows) so I think we'll need a different approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111252