[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.


================
Comment at: include/clang/Basic/FileManager.h:176
+  /// Manage memory buffers associated with pcm files.
+  std::unique_ptr<PCMCache> BufferMgr;
+
----------------
manmanren wrote:
> benlangmuir wrote:
> > Why is this inside the FileManager? It isn't used by the FileManager.
> I initially had another patch that puts PCMCache object at the top level (i.e whenever we create a FileManager, we create a PCMCache object). The initial patch is much bigger than this patch. PCMCache is cacheing the content of a PCM File, and is shared across threads that we spawned to build modules implicitly, just like FileManager. I had some discussions with Bruno and Adrian, we felt FileManager is a better place. But if you have other suggestions, please let me know!
I see.  It's not ideal, but I see why it would be convenient to use the FileManager this way, since you want to share it in all the same places the FileManager is shared.  Since I don't have any better answer, I guess I'm okay with this...


================
Comment at: include/clang/Basic/FileManager.h:308
+  /// a thread, we pop a ThreadContext.
+  struct ThreadContext {
+    /// Keep track of all module files that have been validated in this thread
----------------
manmanren wrote:
> benlangmuir wrote:
> > Can we call this something like "ModuleLoadContext" or maybe "ModuleCompilationContext"?  The word thread is misleading, since the threads are not run concurrently, and are only an implementation detail of our crash recovery.
> > 
> > Also, can you explain why it is only necessary to keep information about the the current stack of contexts?  In a situation like
> > 
> > A imports B imports C
> > A imports D imports C
> > 
> > We would no longer have information about C being validated, right?  What happens if there's a mismatch there?  Can this never happen?
> Sure, we can call it ModuleCompilationContext because we are spawning threads to build modules implicitly.
> 
> The issue we are trying to detect is use-after-free, i.e we don't want a thread to hold on to an invalid FileEntry. We error out before trying to delete the FileEntry that is live in another thread.
> 
> In a situation like
> A imports B imports C
> A imports D imports C
> 
> A will start a thread to build B, and B will start a thread to build C, C will be validated in B's compilation thread and A's compilation thread. Later on, if D tries to invalidate C, D knows that an ancestor thread A has validated C, so the compiler will error out. And it is okay for A to invalidate C in A's compilation thread, because we can clean up the references to C's FileEntry.
> 
Okay


https://reviews.llvm.org/D28299