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

[Frederic Riss via Phabricator via llvm-commits]

friss added a comment.

While you're rewriting this part of the code, why not make the new cache actually thread-safe and share it between the threads handling different architectures. They will open the same files. This way you could also use it in loadClangModule.

In the initial design, the BinaryHolder would have only one file open at a time, now we're keeping them alive basically forever. Any concerns with this?



================
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);
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D48501