[PATCH] D48501: [dsymutil] Introduce a new CachedBinaryHolder

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere marked 7 inline comments as done.
JDevlieghere added inline comments.


================
Comment at: llvm/tools/dsymutil/BinaryHolder.h:44
+  struct ObjectEntry : public EntryBase {
+    std::vector<std::unique_ptr<object::ObjectFile>> Objects;
+
----------------
aprantl wrote:
> Could this be a `std::vector<object::ObjectFile>` instead?
No, we move it into the unique pointer from the loading logic. 


================
Comment at: llvm/tools/dsymutil/DwarfLinker.cpp:3998-3999
     sys::path::append(Path, Filename);
-  BinaryHolder ObjHolder(Options.Verbose);
+  // Don't use the cached binary holder because we have no thread-safety
+  // guarantee and the lifetime is limited.
+  CachedBinaryHolder ObjHolder(Options.Verbose);
----------------
friss wrote:
> It's not clear from reading the comment why it's not thread-safe here. I *think* I know why (this is called by the cloning thread, which is not the thread updating the BinaryHolder), but it would be good to be explicit about it so that others can understand it too.
> It would be good to document in another comment somewhere why the other CachedBinaryHolder is used in a thread-safe way. I suppose there's only ever one thread writing to it, but that's not stated anywhere. It would be even better to have a dynamic check that this is actually the case.
You are indeed correct. I didn't add the comment as I implemented your suggestion and made the cached binary holder thread safe. 


https://reviews.llvm.org/D48501