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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to