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