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

[Noah Shutty via Phabricator via llvm-commits]

noajshu marked 18 inline comments as done.
noajshu added inline comments.


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:341
+      if (isDebugBinary(Object)) {
+        bool LockSucceeded = DebugBinariesMutex.lock();
+        assert(LockSucceeded && "Failed to acquire writer lock.");
----------------
mysterymath wrote:
> std::lock_guard should work with RWMutexes for exclusive locking, and it's easier to read than doing it manually.
Thanks! Since we're `std::shared_lock<RWMutex> lock(write);



================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:412-413
+    if (!Path) {
+      if (Error Err = updateIfStale())
+        return std::move(Err);
+      // try once more
----------------
mysterymath wrote:
> 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.
Good point, how about this? We return an `Expected<bool>` where if there are no errors during `update()`, returns whether the collection got updated. If it's not stale, we don't bother checking for the path again.


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

https://reviews.llvm.org/D114845