[PATCH] D114845: [llvm] [Debuginfod] DebuginfodCollection and DebuginfodServer for tracking local debuginfo.

[Daniel Thornburgh via Phabricator via llvm-commits]

mysterymath added inline comments.


================
Comment at: llvm/include/llvm/Debuginfod/Debuginfod.h:88
+  // Adds a log entry to end of the queue.
+  void push(StringRef Message);
+  // If there are log entries in the queue, pops and returns the first one.
----------------
Since the Message is commonly formed by concatenation, making this take `const Twine& Message` instead should save some heap allocations when logging.


================
Comment at: llvm/include/llvm/Debuginfod/Debuginfod.h:93
+  /// Blocking wait for the log entry queue to have a message.
+  void wait();
+};
----------------
>From how this is used, it looks like it'd be simpler to make pop() blocking and non-Optional.


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:242
+    : Log(Log), Pool(Pool), MinInterval(MinInterval) {
+  for (auto Path : PathsRef)
+    Paths.push_back(Path.str());
----------------
Prefer StringRef over auto, since the type is obvious and concrete.


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:249
+
+  for (auto Path : Paths) {
+    Log.push("Updating binaries at path " + Path);
----------------
const std::string& over auto; I think as written this will even copy the string.




================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:305
+Error DebuginfodCollection::findBinaries(StringRef Path) {
+  std::error_code ec;
+
----------------
EC, from the LLVM variable naming convention


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:307
+
+  for (sys::fs::recursive_directory_iterator i(Twine(Path), ec), e; i != e;
+       i.increment(ec)) {
----------------
`I` and `E`, from the LLVM variable naming convention


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:341
+      if (isDebugBinary(Object)) {
+        bool LockSucceeded = DebugBinariesMutex.lock();
+        assert(LockSucceeded && "Failed to acquire writer lock.");
----------------
std::lock_guard should work with RWMutexes for exclusive locking, and it's easier to read than doing it manually.


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:356
+    // Wait for empty queue before proceeding to the next file to avoid
+    // unbounded memory usage
+    Pool.waitQueueSize();
----------------



================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:364
+DebuginfodCollection::getBinaryPath(BuildIDRef ID) {
+
+  Log.push("getting binary path of ID " + buildIDToString(ID));
----------------
nit: remove empty line


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:385
+DebuginfodCollection::getDebugBinaryPath(BuildIDRef ID) {
+
+  Log.push("getting debug binary path of ID " + buildIDToString(ID));
----------------
nit: remove empty line


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:406
+  {
+    // check collection, perform on-demand update if stale
+    Expected<Optional<std::string>> PathOrErr = getBinaryPath(ID);
----------------



================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:412-413
+    if (!Path) {
+      if (Error Err = updateIfStale())
+        return std::move(Err);
+      // try once more
----------------
This path will always usually cause a "collection was not stale" error message if the binary was not found, which isn't as good as just "binary not found." It's also surprising that updateIfStale() will throw an error if the collection was not stale; usually methods with that naming convention do nothing, since the name suggests that it's not an error to violate the precondition.


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:414
+        return std::move(Err);
+      // try once more
+      PathOrErr = getBinaryPath(ID);
----------------



================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:424
+
+  // federation
+  Expected<std::string> PathOrErr = getCachedOrDownloadExecutable(ID);
----------------



================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:429
+
+  // fall-back to debug binary
+  return findDebugBinaryPath(ID);
----------------



================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:435
+  {
+    // check collection, perform on-demand update if stale
+    Expected<Optional<std::string>> PathOrErr = getDebugBinaryPath(ID);
----------------



================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:443
+        return std::move(Err);
+      // try once more
+      PathOrErr = getDebugBinaryPath(ID);
----------------



================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:453
+
+  // federation
+  return getCachedOrDownloadDebuginfo(ID);
----------------



================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:477
+        streamFile(Request, *PathOrErr);
+        return;
+      }));
----------------
This `return` doesn't do anything; remove.


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

https://reviews.llvm.org/D114845