[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
dexonsmith closed this revision. dexonsmith marked an inline comment as done. dexonsmith added a comment. Thanks for the reviews! Committed in r298165. https://reviews.llvm.org/D28299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
aprantl added a comment.
Drive-by comment.
Comment at: clang/lib/Basic/MemoryBufferCache.cpp:18
+ std::unique_ptr Buffer) {
+ auto Insertion = Buffers.insert(std::make_pair(
+ Filename, BufferEntry{std::move(Buffer), NextIndex++}));
we should be able to replace the `std::make_pair()` with `{}`.
https://reviews.llvm.org/D28299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
dexonsmith added a comment. > ! In https://reviews.llvm.org/D28299#701194, @dexonsmith wrote: > Rebased again, and cleaned up tests, and a ping... now that dependencies are > resolved. > > Richard and/or Ben, can you take a look? (Or anyone else that feels comfortable confirming that the ownership semantics of MemoryBufferCache are correct.) https://reviews.llvm.org/D28299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
dexonsmith updated this revision to Diff 91799. dexonsmith added a comment. Rebased again, and cleaned up tests, and a ping... now that dependencies are resolved. Richard and/or Ben, can you take a look? https://reviews.llvm.org/D28299 Files: clang/include/clang/Basic/DiagnosticSerializationKinds.td clang/include/clang/Basic/MemoryBufferCache.h clang/include/clang/Frontend/ASTUnit.h clang/include/clang/Frontend/CompilerInstance.h clang/include/clang/Lex/Preprocessor.h clang/include/clang/Serialization/ASTReader.h clang/include/clang/Serialization/ASTWriter.h clang/include/clang/Serialization/Module.h clang/include/clang/Serialization/ModuleManager.h clang/lib/Basic/CMakeLists.txt clang/lib/Basic/MemoryBufferCache.cpp clang/lib/Frontend/ASTUnit.cpp clang/lib/Frontend/CompilerInstance.cpp clang/lib/Lex/Preprocessor.cpp clang/lib/Serialization/ASTReader.cpp clang/lib/Serialization/ASTWriter.cpp clang/lib/Serialization/GeneratePCH.cpp clang/lib/Serialization/ModuleManager.cpp clang/test/Modules/Inputs/system-out-of-date/X.h clang/test/Modules/Inputs/system-out-of-date/Y.h clang/test/Modules/Inputs/system-out-of-date/Z.h clang/test/Modules/Inputs/system-out-of-date/module.map clang/test/Modules/Inputs/warning-mismatch/Mismatch.h clang/test/Modules/Inputs/warning-mismatch/System.h clang/test/Modules/Inputs/warning-mismatch/module.modulemap clang/test/Modules/outofdate-rebuild.m clang/test/Modules/system-out-of-date-test.m clang/test/Modules/warning-mismatch.m clang/unittests/Basic/CMakeLists.txt clang/unittests/Basic/MemoryBufferCacheTest.cpp clang/unittests/Basic/SourceManagerTest.cpp clang/unittests/Lex/LexerTest.cpp clang/unittests/Lex/PPCallbacksTest.cpp clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp Index: clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp === --- clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp +++ clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp @@ -12,6 +12,7 @@ #include "clang/Basic/DiagnosticOptions.h" #include "clang/Basic/FileManager.h" #include "clang/Basic/LangOptions.h" +#include "clang/Basic/MemoryBufferCache.h" #include "clang/Basic/SourceManager.h" #include "clang/Basic/TargetInfo.h" #include "clang/Basic/TargetOptions.h" @@ -93,10 +94,11 @@ SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(Buf))); VoidModuleLoader ModLoader; + MemoryBufferCache PCMCache; HeaderSearch HeaderInfo(std::make_shared(), SourceMgr, Diags, LangOpts, Target.get()); Preprocessor PP(std::make_shared(), Diags, LangOpts, - SourceMgr, HeaderInfo, ModLoader, + SourceMgr, PCMCache, HeaderInfo, ModLoader, /*IILookup =*/nullptr, /*OwnsHeaderSearch =*/false); PP.Initialize(*Target); Index: clang/unittests/Lex/PPCallbacksTest.cpp === --- clang/unittests/Lex/PPCallbacksTest.cpp +++ clang/unittests/Lex/PPCallbacksTest.cpp @@ -14,6 +14,7 @@ #include "clang/Basic/DiagnosticOptions.h" #include "clang/Basic/FileManager.h" #include "clang/Basic/LangOptions.h" +#include "clang/Basic/MemoryBufferCache.h" #include "clang/Basic/SourceManager.h" #include "clang/Basic/TargetInfo.h" #include "clang/Basic/TargetOptions.h" @@ -161,13 +162,14 @@ SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(Buf))); VoidModuleLoader ModLoader; +MemoryBufferCache PCMCache; HeaderSearch HeaderInfo(std::make_shared(), SourceMgr, Diags, LangOpts, Target.get()); AddFakeHeader(HeaderInfo, HeaderPath, SystemHeader); Preprocessor PP(std::make_shared(), Diags, LangOpts, -SourceMgr, HeaderInfo, ModLoader, +SourceMgr, PCMCache, HeaderInfo, ModLoader, /*IILookup =*/nullptr, /*OwnsHeaderSearch =*/false); PP.Initialize(*Target); @@ -198,11 +200,12 @@ SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(SourceBuf))); VoidModuleLoader ModLoader; +MemoryBufferCache PCMCache; HeaderSearch HeaderInfo(std::make_shared(), SourceMgr, Diags, OpenCLLangOpts, Target.get()); Preprocessor PP(std::make_shared(), Diags, -OpenCLLangOpts, SourceMgr, HeaderInfo, ModLoader, +OpenCLLangOpts, SourceMgr, PCMCache, HeaderInfo, ModLoader, /*IILookup =*/nullptr, /*OwnsHeaderSearch =*/false); PP.Initialize(*Target); Index: clang/unittests/Lex/LexerTest.cpp === --- clang/unittests/Lex/LexerTest.cpp +++ clang/unittests/Lex/LexerTest.cpp @@ -12,6 +12,7 @@ #include "clang/Basic/DiagnosticOptions.h" #include "clang/Basic/FileManager.h" #include
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
dexonsmith requested review of this revision. dexonsmith added a comment. This has changed enough it needs another review. Richard or Ben, could you take a(nother) look? https://reviews.llvm.org/D28299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
dexonsmith updated this revision to Diff 87257. dexonsmith added a comment. Herald added subscribers: mgorny, nemanjai. Rebased on top of the current https://reviews.llvm.org/D27689. This also has some substantive changes: - Renamed the data structure PCMCache to MemoryBufferCache, and split it out into its own files with tests. - Simplified logic around whether a MemoryBuffer has already been used. Tracking of what has already been validated now takes O(1) memory instead of O(n^2). - Moved FileManager::BufferMgr (the instance of PCMCache) to CompilerInstance::PCMCache and ModuleManager::PCMCache (instances of IntrusiveRefCntPtr). This took some work to thread through, but since the PCMCache isn't related to the FileManager, this seemed like a cleaner result. - Added some testcases for bugs we've found in the meantime (thanks to Adrian Prantl). Primarily, we no longer error or cause a use-after-free if a MemoryBuffer was validated by an ancestor. https://reviews.llvm.org/D28299 Files: clang/include/clang/Basic/DiagnosticSerializationKinds.td clang/include/clang/Basic/MemoryBufferCache.h clang/include/clang/Frontend/ASTUnit.h clang/include/clang/Frontend/CompilerInstance.h clang/include/clang/Lex/Preprocessor.h clang/include/clang/Serialization/ASTReader.h clang/include/clang/Serialization/ASTWriter.h clang/include/clang/Serialization/Module.h clang/include/clang/Serialization/ModuleManager.h clang/lib/Basic/CMakeLists.txt clang/lib/Basic/MemoryBufferCache.cpp clang/lib/Frontend/ASTUnit.cpp clang/lib/Frontend/CompilerInstance.cpp clang/lib/Lex/Preprocessor.cpp clang/lib/Serialization/ASTReader.cpp clang/lib/Serialization/ASTWriter.cpp clang/lib/Serialization/GeneratePCH.cpp clang/lib/Serialization/ModuleManager.cpp clang/test/Modules/Inputs/outofdate-rebuild/AppKit.h clang/test/Modules/Inputs/outofdate-rebuild/Cocoa.h clang/test/Modules/Inputs/outofdate-rebuild/CoreText.h clang/test/Modules/Inputs/outofdate-rebuild/CoreVideo.h clang/test/Modules/Inputs/outofdate-rebuild/Foundation.h clang/test/Modules/Inputs/outofdate-rebuild/module.modulemap clang/test/Modules/Inputs/system-out-of-date/X.h clang/test/Modules/Inputs/system-out-of-date/Y.h clang/test/Modules/Inputs/system-out-of-date/Z.h clang/test/Modules/Inputs/system-out-of-date/module.map clang/test/Modules/Inputs/warning-mismatch/Mismatch.h clang/test/Modules/Inputs/warning-mismatch/System.h clang/test/Modules/Inputs/warning-mismatch/module.modulemap clang/test/Modules/outofdate-rebuild.m clang/test/Modules/system-out-of-date-test.m clang/test/Modules/warning-mismatch.m clang/unittests/Basic/CMakeLists.txt clang/unittests/Basic/MemoryBufferCacheTest.cpp clang/unittests/Basic/SourceManagerTest.cpp clang/unittests/Lex/LexerTest.cpp clang/unittests/Lex/PPCallbacksTest.cpp clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp Index: clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp === --- clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp +++ clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp @@ -12,6 +12,7 @@ #include "clang/Basic/DiagnosticOptions.h" #include "clang/Basic/FileManager.h" #include "clang/Basic/LangOptions.h" +#include "clang/Basic/MemoryBufferCache.h" #include "clang/Basic/SourceManager.h" #include "clang/Basic/TargetInfo.h" #include "clang/Basic/TargetOptions.h" @@ -93,10 +94,11 @@ SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(Buf))); VoidModuleLoader ModLoader; + MemoryBufferCache PCMCache; HeaderSearch HeaderInfo(std::make_shared(), SourceMgr, Diags, LangOpts, Target.get()); Preprocessor PP(std::make_shared(), Diags, LangOpts, - SourceMgr, HeaderInfo, ModLoader, + SourceMgr, PCMCache, HeaderInfo, ModLoader, /*IILookup =*/nullptr, /*OwnsHeaderSearch =*/false); PP.Initialize(*Target); Index: clang/unittests/Lex/PPCallbacksTest.cpp === --- clang/unittests/Lex/PPCallbacksTest.cpp +++ clang/unittests/Lex/PPCallbacksTest.cpp @@ -14,6 +14,7 @@ #include "clang/Basic/DiagnosticOptions.h" #include "clang/Basic/FileManager.h" #include "clang/Basic/LangOptions.h" +#include "clang/Basic/MemoryBufferCache.h" #include "clang/Basic/SourceManager.h" #include "clang/Basic/TargetInfo.h" #include "clang/Basic/TargetOptions.h" @@ -161,13 +162,14 @@ SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(Buf))); VoidModuleLoader ModLoader; +MemoryBufferCache PCMCache; HeaderSearch HeaderInfo(std::make_shared(), SourceMgr, Diags, LangOpts, Target.get()); AddFakeHeader(HeaderInfo, HeaderPath, SystemHeader); Preprocessor PP(std::make_shared(), Diags, LangOpts, -SourceMgr, HeaderInfo,
Re: [PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
According to the comment at line 239:
if (LoadedSuccessfully.count(*victim) == 0) {
// Before removing the module file, check if it was validated in an
// ancestor thread, if yes, throw a hard error instead of causing
// use-after-free in the ancestor thread.
bool IsSystem;
if (FileMgr.getPCMCache()->isValidatedByAncestor((*victim)->FileName,
IsSystem))
ValidationConflicts.push_back((*victim)->FileName);
else
FileMgr.invalidateCache((*victim)->File);
FileMgr.getPCMCache()->removeFromConsistentBuffer((*victim)->FileName);
}
that looks like it would cause a use-after-free in the parent thread. What am
I missing?
> On 2017-Jan-24, at 17:56, Bruno Cardoso Lopes via Phabricator
> wrote:
>
> bruno added inline comments.
>
>
>
> Comment at: lib/Serialization/ASTReader.cpp:3692
> +ValidationConflicts);
> +for (auto N : ValidationConflicts)
> + Diag(diag::err_module_ancestor_conflict) << N;
>
> This should honor `ARR_OutOfDate`, so that the module can be rebuilt in case
> a user module mismatch via a different diagnostics
> (lib/Serialization/ASTReader.cpp:4076) and returns OutOfDate instead of
> Success:
>
>if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
> for (auto N : ValidationConflicts)
>Diag(diag::err_module_ancestor_conflict) << N;
>
>
> https://reviews.llvm.org/D28299
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
bruno added inline comments. Comment at: lib/Serialization/ASTReader.cpp:3692 +ValidationConflicts); +for (auto N : ValidationConflicts) + Diag(diag::err_module_ancestor_conflict) << N; This should honor `ARR_OutOfDate`, so that the module can be rebuilt in case a user module mismatch via a different diagnostics (lib/Serialization/ASTReader.cpp:4076) and returns OutOfDate instead of Success: if ((ClientLoadCapabilities & ARR_OutOfDate) == 0) for (auto N : ValidationConflicts) Diag(diag::err_module_ancestor_conflict) << N; https://reviews.llvm.org/D28299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
benlangmuir accepted this revision. benlangmuir added a comment. This revision is now accepted and ready to land. Sorry for the delay, I though you were still iterating with @aprantl for some reason. LGTM https://reviews.llvm.org/D28299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
manmanren added a comment. Ping :] Hoping to wrap this up this week. Cheers, Manman https://reviews.llvm.org/D28299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
manmanren updated this revision to Diff 84137.
manmanren added a comment.
Addressing review comments.
https://reviews.llvm.org/D28299
Files:
include/clang/Basic/DiagnosticSerializationKinds.td
include/clang/Basic/FileManager.h
include/clang/Serialization/ASTReader.h
include/clang/Serialization/ASTWriter.h
include/clang/Serialization/Module.h
include/clang/Serialization/ModuleManager.h
lib/Basic/FileManager.cpp
lib/Frontend/ASTUnit.cpp
lib/Frontend/ChainedIncludesSource.cpp
lib/Frontend/CompilerInstance.cpp
lib/Frontend/FrontendActions.cpp
lib/Serialization/ASTReader.cpp
lib/Serialization/ASTWriter.cpp
lib/Serialization/GeneratePCH.cpp
lib/Serialization/ModuleManager.cpp
test/Modules/Inputs/system-out-of-date/X.h
test/Modules/Inputs/system-out-of-date/Y.h
test/Modules/Inputs/system-out-of-date/Z.h
test/Modules/Inputs/system-out-of-date/module.map
test/Modules/system-out-of-date-test.m
Index: test/Modules/system-out-of-date-test.m
===
--- /dev/null
+++ test/Modules/system-out-of-date-test.m
@@ -0,0 +1,13 @@
+// RUN: rm -rf %t
+// RUN: echo '@import X;' | \
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t -I %S/Inputs/system-out-of-date \
+// RUN: -fsyntax-only -x objective-c -
+
+// We have an version built with different diagnostic options.
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/system-out-of-date -Wnon-modular-include-in-framework-module -Werror=non-modular-include-in-framework-module %s -fsyntax-only 2>&1 | FileCheck %s
+ @import X;
+
+ #import
+// CHECK: While building module 'Z' imported from
+// CHECK: {{.*}}Y-{{.*}}pcm' was validated as a system module and is now being imported as a non-system module
Index: test/Modules/Inputs/system-out-of-date/module.map
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/module.map
@@ -0,0 +1,12 @@
+module X [system] {
+ header "X.h" // imports Y
+ export *
+}
+module Y {
+ header "Y.h"
+ export *
+}
+module Z {
+ header "Z.h" // imports Y
+ export *
+}
Index: test/Modules/Inputs/system-out-of-date/Z.h
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/Z.h
@@ -0,0 +1 @@
+#import
Index: test/Modules/Inputs/system-out-of-date/Y.h
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/Y.h
@@ -0,0 +1 @@
+//empty
Index: test/Modules/Inputs/system-out-of-date/X.h
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/X.h
@@ -0,0 +1 @@
+#import
Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -108,7 +108,11 @@
// Load the contents of the module
if (std::unique_ptr Buffer = lookupBuffer(FileName)) {
// The buffer was already provided for us.
- ModuleEntry->Buffer = std::move(Buffer);
+ ModuleEntry->Buffer = Buffer.get();
+ FileMgr.getPCMCache()->addConsistentBuffer(FileName, std::move(Buffer));
+} else if(llvm::MemoryBuffer *ConsistentB =
+ FileMgr.getPCMCache()->lookupConsistentBuffer(FileName)) {
+ ModuleEntry->Buffer = ConsistentB;
} else {
// Open the AST file.
llvm::ErrorOr Buf(
@@ -131,7 +135,8 @@
return Missing;
}
- ModuleEntry->Buffer = std::move(*Buf);
+ ModuleEntry->Buffer = (*Buf).get();
+ FileMgr.getPCMCache()->addConsistentBuffer(FileName, std::move(*Buf));
}
// Initialize the stream.
@@ -148,8 +153,11 @@
ErrorStr = ModuleEntry->Signature != ASTFileSignature({{0}}) ?
"signature mismatch" : "could not read module signature";
- if (NewModule)
+ if (NewModule) {
+FileMgr.getPCMCache()->removeFromConsistentBuffer(
+ModuleEntry->FileName);
delete ModuleEntry;
+ }
return OutOfDate;
}
}
@@ -184,7 +192,8 @@
void ModuleManager::removeModules(
ModuleIterator first, ModuleIterator last,
llvm::SmallPtrSetImpl ,
-ModuleMap *modMap) {
+ModuleMap *modMap,
+llvm::SmallVectorImpl ) {
if (first == last)
return;
@@ -227,16 +236,76 @@
// Files that didn't make it through ReadASTCore successfully will be
// rebuilt (or there was an error). Invalidate them so that we can load the
// new files that will be renamed over the old ones.
-if (LoadedSuccessfully.count(*victim) == 0)
- FileMgr.invalidateCache((*victim)->File);
-
+if (LoadedSuccessfully.count(*victim) == 0) {
+ // Before removing the module file, check if it was validated in an
+ // ancestor
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
benlangmuir added inline comments.
Comment at: include/clang/Basic/FileManager.h:176
+ /// Manage memory buffers associated with pcm files.
+ std::unique_ptr 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
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
manmanren added inline comments. Comment at: include/clang/Basic/DiagnosticSerializationKinds.td:131 + "diagnostic options now it is a non-system module">, + InGroup; + aprantl wrote: > Is this better? > > "module file '%0' was validated as a system module and is now being imported > as a non-system module; any differences in diagnostics options will be > ignored" Thanks Adrian. Your wording sounds better. Comment at: include/clang/Basic/DiagnosticSerializationKinds.td:135 + "module file '%0' was validated in an ancestor thread, now it needs to " + "be invalidated">; + aprantl wrote: > I wonder if there is a way to rephrase this message such that an end-user > could understan how to interpret this error? I am not sure if we will emit this error diagnostic in some case, that is why I don't have a testing case. Maybe it is better to throw a hard error instead of a diagnostic that user can't understand? https://reviews.llvm.org/D28299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
manmanren updated this revision to Diff 83273.
manmanren added a comment.
Add testing case.
https://reviews.llvm.org/D28299
Files:
include/clang/Basic/DiagnosticSerializationKinds.td
include/clang/Basic/FileManager.h
include/clang/Serialization/ASTReader.h
include/clang/Serialization/ASTWriter.h
include/clang/Serialization/Module.h
include/clang/Serialization/ModuleManager.h
lib/Basic/FileManager.cpp
lib/Frontend/ASTUnit.cpp
lib/Frontend/ChainedIncludesSource.cpp
lib/Frontend/CompilerInstance.cpp
lib/Frontend/FrontendActions.cpp
lib/Serialization/ASTReader.cpp
lib/Serialization/ASTWriter.cpp
lib/Serialization/GeneratePCH.cpp
lib/Serialization/ModuleManager.cpp
test/Modules/Inputs/system-out-of-date/X.h
test/Modules/Inputs/system-out-of-date/Y.h
test/Modules/Inputs/system-out-of-date/Z.h
test/Modules/Inputs/system-out-of-date/module.map
test/Modules/system-out-of-date-test.m
Index: test/Modules/system-out-of-date-test.m
===
--- /dev/null
+++ test/Modules/system-out-of-date-test.m
@@ -0,0 +1,13 @@
+// RUN: rm -rf %t
+// RUN: echo '@import X;' | \
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t -I %S/Inputs/system-out-of-date \
+// RUN: -fsyntax-only -x objective-c -
+
+// We have an version built with different diagnostic options.
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/system-out-of-date -Wnon-modular-include-in-framework-module -Werror=non-modular-include-in-framework-module %s -fsyntax-only 2>&1 | FileCheck %s
+ @import X;
+
+ #import
+// CHECK: While building module 'Z' imported from
+// CHECK: {{.*}}Y-{{.*}}pcm' was validated as a system module, ignoring differences in diagnostic options
Index: test/Modules/Inputs/system-out-of-date/module.map
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/module.map
@@ -0,0 +1,12 @@
+module X [system] {
+ header "X.h" // imports Y
+ export *
+}
+module Y {
+ header "Y.h"
+ export *
+}
+module Z {
+ header "Z.h" // imports Y
+ export *
+}
Index: test/Modules/Inputs/system-out-of-date/Z.h
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/Z.h
@@ -0,0 +1 @@
+#import
Index: test/Modules/Inputs/system-out-of-date/Y.h
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/Y.h
@@ -0,0 +1 @@
+//empty
Index: test/Modules/Inputs/system-out-of-date/X.h
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/X.h
@@ -0,0 +1 @@
+#import
Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -108,7 +108,11 @@
// Load the contents of the module
if (std::unique_ptr Buffer = lookupBuffer(FileName)) {
// The buffer was already provided for us.
- ModuleEntry->Buffer = std::move(Buffer);
+ ModuleEntry->Buffer = Buffer.get();
+ FileMgr.getPCMCache()->addConsistentBuffer(FileName, std::move(Buffer));
+} else if(llvm::MemoryBuffer *ConsistentB =
+ FileMgr.getPCMCache()->lookupConsistentBuffer(FileName)) {
+ ModuleEntry->Buffer = ConsistentB;
} else {
// Open the AST file.
llvm::ErrorOr Buf(
@@ -131,7 +135,8 @@
return Missing;
}
- ModuleEntry->Buffer = std::move(*Buf);
+ ModuleEntry->Buffer = (*Buf).get();
+ FileMgr.getPCMCache()->addConsistentBuffer(FileName, std::move(*Buf));
}
// Initialize the stream.
@@ -148,8 +153,11 @@
ErrorStr = ModuleEntry->Signature != ASTFileSignature({{0}}) ?
"signature mismatch" : "could not read module signature";
- if (NewModule)
+ if (NewModule) {
+FileMgr.getPCMCache()->removeFromConsistentBuffer(
+ModuleEntry->FileName);
delete ModuleEntry;
+ }
return OutOfDate;
}
}
@@ -184,7 +192,8 @@
void ModuleManager::removeModules(
ModuleIterator first, ModuleIterator last,
llvm::SmallPtrSetImpl ,
-ModuleMap *modMap) {
+ModuleMap *modMap,
+llvm::SmallVectorImpl ) {
if (first == last)
return;
@@ -227,16 +236,76 @@
// Files that didn't make it through ReadASTCore successfully will be
// rebuilt (or there was an error). Invalidate them so that we can load the
// new files that will be renamed over the old ones.
-if (LoadedSuccessfully.count(*victim) == 0)
- FileMgr.invalidateCache((*victim)->File);
-
+if (LoadedSuccessfully.count(*victim) == 0) {
+ // Before removing the module file, check if it was validated in an
+ // ancestor thread, if yes, throw
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
manmanren added a comment.
I forgot to upload the testing case, I will add it now.
Manman
Comment at: include/clang/Basic/FileManager.h:176
+ /// Manage memory buffers associated with pcm files.
+ std::unique_ptr BufferMgr;
+
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!
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
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.
https://reviews.llvm.org/D28299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
benlangmuir added a comment.
Can we test the already-validated diagnostics?
Comment at: include/clang/Basic/FileManager.h:176
+ /// Manage memory buffers associated with pcm files.
+ std::unique_ptr BufferMgr;
+
Why is this inside the FileManager? It isn't used by the FileManager.
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
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?
https://reviews.llvm.org/D28299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
aprantl added inline comments. Comment at: include/clang/Basic/DiagnosticSerializationKinds.td:131 + "diagnostic options now it is a non-system module">, + InGroup; + Is this better? "module file '%0' was validated as a system module and is now being imported as a non-system module; any differences in diagnostics options will be ignored" Comment at: include/clang/Basic/DiagnosticSerializationKinds.td:135 + "module file '%0' was validated in an ancestor thread, now it needs to " + "be invalidated">; + I wonder if there is a way to rephrase this message such that an end-user could understan how to interpret this error? https://reviews.llvm.org/D28299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.
manmanren created this revision.
manmanren added reviewers: rsmith, benlangmuir, dexonsmith.
manmanren added subscribers: cfe-commits, bruno.
We create a PCMCache class to manage memory buffers associated with pcm files.
With implicit modules, we currently use lock files to make
sure we are making progress when a single clang invocation builds a module file
and then loads the module file immediately after. This
implementation requires lock file for correctness and causes all kinds of
locking issues.
This patch tries to use PCMCache to share the content between writes and reads
within a single clang invocation, so we can remove the
correctness-side of the lock file, and only use lock file for performance i.e
to reduce chance of building the same pcm file from multiple
clang invocations.
We also add ThreadContext in PCMCache to diagnose cases where an ancestor
validates a module and later on, a child thread tries
to invalidate it. Without the patch, this scenario will cause use-after-free of
the FileEntry held by the ancestor.
https://reviews.llvm.org/D28299
Files:
include/clang/Basic/DiagnosticSerializationKinds.td
include/clang/Basic/FileManager.h
include/clang/Serialization/ASTReader.h
include/clang/Serialization/ASTWriter.h
include/clang/Serialization/Module.h
include/clang/Serialization/ModuleManager.h
lib/Basic/FileManager.cpp
lib/Frontend/ASTUnit.cpp
lib/Frontend/ChainedIncludesSource.cpp
lib/Frontend/CompilerInstance.cpp
lib/Frontend/FrontendActions.cpp
lib/Serialization/ASTReader.cpp
lib/Serialization/ASTWriter.cpp
lib/Serialization/GeneratePCH.cpp
lib/Serialization/ModuleManager.cpp
Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -108,7 +108,11 @@
// Load the contents of the module
if (std::unique_ptr Buffer = lookupBuffer(FileName)) {
// The buffer was already provided for us.
- ModuleEntry->Buffer = std::move(Buffer);
+ ModuleEntry->Buffer = Buffer.get();
+ FileMgr.getPCMCache()->addConsistentBuffer(FileName, std::move(Buffer));
+} else if(llvm::MemoryBuffer *ConsistentB =
+ FileMgr.getPCMCache()->lookupConsistentBuffer(FileName)) {
+ ModuleEntry->Buffer = ConsistentB;
} else {
// Open the AST file.
llvm::ErrorOr Buf(
@@ -131,7 +135,8 @@
return Missing;
}
- ModuleEntry->Buffer = std::move(*Buf);
+ ModuleEntry->Buffer = (*Buf).get();
+ FileMgr.getPCMCache()->addConsistentBuffer(FileName, std::move(*Buf));
}
// Initialize the stream.
@@ -148,8 +153,11 @@
ErrorStr = ModuleEntry->Signature != ASTFileSignature({{0}}) ?
"signature mismatch" : "could not read module signature";
- if (NewModule)
+ if (NewModule) {
+FileMgr.getPCMCache()->removeFromConsistentBuffer(
+ModuleEntry->FileName);
delete ModuleEntry;
+ }
return OutOfDate;
}
}
@@ -184,7 +192,8 @@
void ModuleManager::removeModules(
ModuleIterator first, ModuleIterator last,
llvm::SmallPtrSetImpl ,
-ModuleMap *modMap) {
+ModuleMap *modMap,
+llvm::SmallVectorImpl ) {
if (first == last)
return;
@@ -227,16 +236,76 @@
// Files that didn't make it through ReadASTCore successfully will be
// rebuilt (or there was an error). Invalidate them so that we can load the
// new files that will be renamed over the old ones.
-if (LoadedSuccessfully.count(*victim) == 0)
- FileMgr.invalidateCache((*victim)->File);
-
+if (LoadedSuccessfully.count(*victim) == 0) {
+ // Before removing the module file, check if it was validated in an
+ // ancestor thread, if yes, throw a hard error instead of causing
+ // use-after-free in the ancestor thread.
+ bool IsSystem;
+ if (FileMgr.getPCMCache()->isValidatedByAncestor((*victim)->FileName,
+ IsSystem))
+ValidationConflicts.push_back((*victim)->FileName);
+ else
+FileMgr.invalidateCache((*victim)->File);
+ FileMgr.getPCMCache()->removeFromConsistentBuffer((*victim)->FileName);
+}
delete *victim;
}
// Remove the modules from the chain.
Chain.erase(first, last);
}
+llvm::MemoryBuffer*
+PCMCache::lookupConsistentBuffer(std::string Name) {
+ if (!ConsistentBuffers.count(Name))
+return nullptr;
+ return ConsistentBuffers[Name].first.get();
+}
+
+void
+PCMCache::addConsistentBuffer(std::string FileName,
+ std::unique_ptr Buffer) {
+ assert(!ConsistentBuffers.count(FileName) && "already has a buffer");
+ ConsistentBuffers[FileName] = std::make_pair(std::move(Buffer),
+ /*IsSystem*/false);
+}
+
+void PCMCache::removeFromConsistentBuffer(std::string FileName) {
+
