Hi, Sorry for bringing up such an old revision again, but:
On 07/11/2012 10:59 PM, Argyrios Kyrtzidis wrote: > Author: akirtzidis > Date: Wed Jul 11 15:59:04 2012 > New Revision: 160074 > > URL: http://llvm.org/viewvc/llvm-project?rev=160074&view=rev > Log: > Introduce a flag in SourceManager to treat non-system source files > as "volatile", meaning there's a high enough chance that they may > change while we are trying to use them. > > This flag is only enabled by libclang. > Currently "volatile" source files will be stat'ed immediately > before opening them, because the file size stat info > may not be accurate since when we got it (e.g. from the PCH). > This avoids crashes when trying to reference mmap'ed memory > from a file whose size is not what we expect. What about the cached "doesn't exist here" results? Aren't they an issue here, too (they are for us :-) Should we extend this to not cache lookup misses of volatile files? Axel. > Note that there's still a window for a racing issue to occur > but the window for it should be way smaller than before. > We can consider later on to avoid mmap completely on such files. > > rdar://11612916 > > Modified: > cfe/trunk/include/clang/Basic/FileManager.h > cfe/trunk/include/clang/Basic/SourceManager.h > cfe/trunk/include/clang/Frontend/ASTUnit.h > cfe/trunk/lib/Basic/FileManager.cpp > cfe/trunk/lib/Basic/SourceManager.cpp > cfe/trunk/lib/Frontend/ASTUnit.cpp > cfe/trunk/lib/Serialization/ASTReader.cpp > cfe/trunk/tools/libclang/CIndex.cpp > cfe/trunk/tools/libclang/Indexing.cpp > > Modified: cfe/trunk/include/clang/Basic/FileManager.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=160074&r1=160073&r2=160074&view=diff > ============================================================================== > --- cfe/trunk/include/clang/Basic/FileManager.h (original) > +++ cfe/trunk/include/clang/Basic/FileManager.h Wed Jul 11 15:59:04 2012 > @@ -221,7 +221,8 @@ > /// \brief Open the specified file as a MemoryBuffer, returning a new > /// MemoryBuffer if successful, otherwise returning null. > llvm::MemoryBuffer *getBufferForFile(const FileEntry *Entry, > - std::string *ErrorStr = 0); > + std::string *ErrorStr = 0, > + bool isVolatile = false); > llvm::MemoryBuffer *getBufferForFile(StringRef Filename, > std::string *ErrorStr = 0); > > > Modified: cfe/trunk/include/clang/Basic/SourceManager.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/SourceManager.h?rev=160074&r1=160073&r2=160074&view=diff > ============================================================================== > --- cfe/trunk/include/clang/Basic/SourceManager.h (original) > +++ cfe/trunk/include/clang/Basic/SourceManager.h Wed Jul 11 15:59:04 2012 > @@ -128,14 +128,20 @@ > /// When true, the original entry may be a virtual file that does not > /// exist. > unsigned BufferOverridden : 1; > + > + /// \brief True if this content cache was initially created for a source > + /// file considered as a system one. > + unsigned IsSystemFile : 1; > > ContentCache(const FileEntry *Ent = 0) > : Buffer(0, false), OrigEntry(Ent), ContentsEntry(Ent), > - SourceLineCache(0), NumLines(0), BufferOverridden(false) {} > + SourceLineCache(0), NumLines(0), BufferOverridden(false), > + IsSystemFile(false) {} > > ContentCache(const FileEntry *Ent, const FileEntry *contentEnt) > : Buffer(0, false), OrigEntry(Ent), ContentsEntry(contentEnt), > - SourceLineCache(0), NumLines(0), BufferOverridden(false) {} > + SourceLineCache(0), NumLines(0), BufferOverridden(false), > + IsSystemFile(false) {} > > ~ContentCache(); > > @@ -143,7 +149,8 @@ > /// a non-NULL Buffer or SourceLineCache. Ownership of allocated memory > /// is not transferred, so this is a logical error. > ContentCache(const ContentCache &RHS) > - : Buffer(0, false), SourceLineCache(0), BufferOverridden(false) > + : Buffer(0, false), SourceLineCache(0), BufferOverridden(false), > + IsSystemFile(false) > { > OrigEntry = RHS.OrigEntry; > ContentsEntry = RHS.ContentsEntry; > @@ -534,6 +541,10 @@ > /// files, should report the original file name. Defaults to true. > bool OverridenFilesKeepOriginalName; > > + /// \brief True if non-system source files should be treated as volatile > + /// (likely to change while trying to use them). Defaults to false. > + bool UserFilesAreVolatile; > + > struct OverriddenFilesInfoTy { > /// \brief Files that have been overriden with the contents from another > /// file. > @@ -639,7 +650,8 @@ > explicit SourceManager(const SourceManager&); > void operator=(const SourceManager&); > public: > - SourceManager(DiagnosticsEngine &Diag, FileManager &FileMgr); > + SourceManager(DiagnosticsEngine &Diag, FileManager &FileMgr, > + bool UserFilesAreVolatile = false); > ~SourceManager(); > > void clearIDTables(); > @@ -654,6 +666,10 @@ > OverridenFilesKeepOriginalName = value; > } > > + /// \brief True if non-system source files should be treated as volatile > + /// (likely to change while trying to use them). > + bool userFilesAreVolatile() const { return UserFilesAreVolatile; } > + > /// \brief Create the FileID for a memory buffer that will represent the > /// FileID for the main source. > /// > @@ -706,7 +722,9 @@ > FileID createFileID(const FileEntry *SourceFile, SourceLocation > IncludePos, > SrcMgr::CharacteristicKind FileCharacter, > int LoadedID = 0, unsigned LoadedOffset = 0) { > - const SrcMgr::ContentCache *IR = getOrCreateContentCache(SourceFile); > + const SrcMgr::ContentCache * > + IR = getOrCreateContentCache(SourceFile, > + /*isSystemFile=*/FileCharacter != > SrcMgr::C_User); > assert(IR && "getOrCreateContentCache() cannot return NULL"); > return createFileID(IR, IncludePos, FileCharacter, LoadedID, > LoadedOffset); > } > @@ -1519,7 +1537,8 @@ > int LoadedID, unsigned LoadedOffset); > > const SrcMgr::ContentCache * > - getOrCreateContentCache(const FileEntry *SourceFile); > + getOrCreateContentCache(const FileEntry *SourceFile, > + bool isSystemFile = false); > > /// \brief Create a new ContentCache for the specified memory buffer. > const SrcMgr::ContentCache* > > Modified: cfe/trunk/include/clang/Frontend/ASTUnit.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/ASTUnit.h?rev=160074&r1=160073&r2=160074&view=diff > ============================================================================== > --- cfe/trunk/include/clang/Frontend/ASTUnit.h (original) > +++ cfe/trunk/include/clang/Frontend/ASTUnit.h Wed Jul 11 15:59:04 2012 > @@ -254,6 +254,10 @@ > /// \brief Whether to include brief documentation within the set of code > /// completions cached. > bool IncludeBriefCommentsInCodeCompletion : 1; > + > + /// \brief True if non-system source files should be treated as volatile > + /// (likely to change while trying to use them). > + bool UserFilesAreVolatile : 1; > > /// \brief The language options used when we load an AST file. > LangOptions ASTFileLangOpts; > @@ -619,7 +623,8 @@ > /// \brief Create a ASTUnit. Gets ownership of the passed > CompilerInvocation. > static ASTUnit *create(CompilerInvocation *CI, > IntrusiveRefCntPtr<DiagnosticsEngine> Diags, > - bool CaptureDiagnostics = false); > + bool CaptureDiagnostics, > + bool UserFilesAreVolatile); > > /// \brief Create a ASTUnit from an AST file. > /// > @@ -636,7 +641,8 @@ > RemappedFile *RemappedFiles = 0, > unsigned NumRemappedFiles = 0, > bool CaptureDiagnostics = false, > - bool AllowPCHWithCompilerErrors = false); > + bool AllowPCHWithCompilerErrors = false, > + bool UserFilesAreVolatile = false); > > private: > /// \brief Helper function for \c LoadFromCompilerInvocation() and > @@ -687,6 +693,7 @@ > bool PrecompilePreamble = > false, > bool CacheCodeCompletionResults = > false, > bool IncludeBriefCommentsInCodeCompletion = > false, > + bool UserFilesAreVolatile = false, > OwningPtr<ASTUnit> *ErrAST = 0); > > /// LoadFromCompilerInvocation - Create an ASTUnit from a source file, > via a > @@ -707,7 +714,8 @@ > bool PrecompilePreamble = > false, > TranslationUnitKind TUKind = > TU_Complete, > bool CacheCodeCompletionResults = > false, > - bool IncludeBriefCommentsInCodeCompletion = > false); > + bool IncludeBriefCommentsInCodeCompletion = > false, > + bool UserFilesAreVolatile = > false); > > /// LoadFromCommandLine - Create an ASTUnit from a vector of command line > /// arguments, which must specify exactly one source file. > @@ -742,6 +750,7 @@ > bool IncludeBriefCommentsInCodeCompletion = > false, > bool AllowPCHWithCompilerErrors = > false, > bool SkipFunctionBodies = false, > + bool UserFilesAreVolatile = false, > OwningPtr<ASTUnit> *ErrAST = 0); > > /// \brief Reparse the source files using the same command-line options > that > > Modified: cfe/trunk/lib/Basic/FileManager.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=160074&r1=160073&r2=160074&view=diff > ============================================================================== > --- cfe/trunk/lib/Basic/FileManager.cpp (original) > +++ cfe/trunk/lib/Basic/FileManager.cpp Wed Jul 11 15:59:04 2012 > @@ -488,15 +488,21 @@ > } > > llvm::MemoryBuffer *FileManager:: > -getBufferForFile(const FileEntry *Entry, std::string *ErrorStr) { > +getBufferForFile(const FileEntry *Entry, std::string *ErrorStr, > + bool isVolatile) { > OwningPtr<llvm::MemoryBuffer> Result; > llvm::error_code ec; > > + uint64_t FileSize = Entry->getSize(); > + // If there's a high enough chance that the file have changed since we > + // got its size, force a stat before opening it. > + if (isVolatile) > + FileSize = -1; > + > const char *Filename = Entry->getName(); > // If the file is already open, use the open file descriptor. > if (Entry->FD != -1) { > - ec = llvm::MemoryBuffer::getOpenFile(Entry->FD, Filename, Result, > - Entry->getSize()); > + ec = llvm::MemoryBuffer::getOpenFile(Entry->FD, Filename, Result, > FileSize); > if (ErrorStr) > *ErrorStr = ec.message(); > > @@ -508,7 +514,7 @@ > // Otherwise, open the file. > > if (FileSystemOpts.WorkingDir.empty()) { > - ec = llvm::MemoryBuffer::getFile(Filename, Result, Entry->getSize()); > + ec = llvm::MemoryBuffer::getFile(Filename, Result, FileSize); > if (ec && ErrorStr) > *ErrorStr = ec.message(); > return Result.take(); > @@ -516,7 +522,7 @@ > > SmallString<128> FilePath(Entry->getName()); > FixupRelativePath(FilePath); > - ec = llvm::MemoryBuffer::getFile(FilePath.str(), Result, Entry->getSize()); > + ec = llvm::MemoryBuffer::getFile(FilePath.str(), Result, FileSize); > if (ec && ErrorStr) > *ErrorStr = ec.message(); > return Result.take(); > > Modified: cfe/trunk/lib/Basic/SourceManager.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/SourceManager.cpp?rev=160074&r1=160073&r2=160074&view=diff > ============================================================================== > --- cfe/trunk/lib/Basic/SourceManager.cpp (original) > +++ cfe/trunk/lib/Basic/SourceManager.cpp Wed Jul 11 15:59:04 2012 > @@ -97,7 +97,10 @@ > } > > std::string ErrorStr; > - Buffer.setPointer(SM.getFileManager().getBufferForFile(ContentsEntry, > &ErrorStr)); > + bool isVolatile = SM.userFilesAreVolatile() && !IsSystemFile; > + Buffer.setPointer(SM.getFileManager().getBufferForFile(ContentsEntry, > + &ErrorStr, > + isVolatile)); > > // If we were unable to open the file, then we are in an inconsistent > // situation where the content cache referenced a file which no longer > @@ -367,8 +370,10 @@ > // Private 'Create' methods. > > //===----------------------------------------------------------------------===// > > -SourceManager::SourceManager(DiagnosticsEngine &Diag, FileManager &FileMgr) > +SourceManager::SourceManager(DiagnosticsEngine &Diag, FileManager &FileMgr, > + bool UserFilesAreVolatile) > : Diag(Diag), FileMgr(FileMgr), OverridenFilesKeepOriginalName(true), > + UserFilesAreVolatile(UserFilesAreVolatile), > ExternalSLocEntries(0), LineTable(0), NumLinearScans(0), > NumBinaryProbes(0), FakeBufferForRecovery(0), > FakeContentCacheForRecovery(0) { > @@ -426,7 +431,8 @@ > /// getOrCreateContentCache - Create or return a cached ContentCache for the > /// specified file. > const ContentCache * > -SourceManager::getOrCreateContentCache(const FileEntry *FileEnt) { > +SourceManager::getOrCreateContentCache(const FileEntry *FileEnt, > + bool isSystemFile) { > assert(FileEnt && "Didn't specify a file entry to use?"); > > // Do we already have information about this file? > @@ -455,6 +461,8 @@ > new (Entry) ContentCache(FileEnt); > } > > + Entry->IsSystemFile = isSystemFile; > + > return Entry; > } > > > Modified: cfe/trunk/lib/Frontend/ASTUnit.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ASTUnit.cpp?rev=160074&r1=160073&r2=160074&view=diff > ============================================================================== > --- cfe/trunk/lib/Frontend/ASTUnit.cpp (original) > +++ cfe/trunk/lib/Frontend/ASTUnit.cpp Wed Jul 11 15:59:04 2012 > @@ -215,7 +215,7 @@ > PreambleRebuildCounter(0), SavedMainFileBuffer(0), PreambleBuffer(0), > NumWarningsInPreamble(0), > ShouldCacheCodeCompletionResults(false), > - IncludeBriefCommentsInCodeCompletion(false), > + IncludeBriefCommentsInCodeCompletion(false), UserFilesAreVolatile(false), > CompletionCacheTopLevelHashValue(0), > PreambleTopLevelHashValue(0), > CurrentTopLevelHashValue(0), > @@ -658,7 +658,8 @@ > RemappedFile *RemappedFiles, > unsigned NumRemappedFiles, > bool CaptureDiagnostics, > - bool AllowPCHWithCompilerErrors) { > + bool AllowPCHWithCompilerErrors, > + bool UserFilesAreVolatile) { > OwningPtr<ASTUnit> AST(new ASTUnit(true)); > > // Recover resources if we crash before exiting this method. > @@ -674,8 +675,10 @@ > AST->CaptureDiagnostics = CaptureDiagnostics; > AST->Diagnostics = Diags; > AST->FileMgr = new FileManager(FileSystemOpts); > + AST->UserFilesAreVolatile = UserFilesAreVolatile; > AST->SourceMgr = new SourceManager(AST->getDiagnostics(), > - AST->getFileManager()); > + AST->getFileManager(), > + UserFilesAreVolatile); > AST->HeaderInfo.reset(new HeaderSearch(AST->getFileManager(), > AST->getDiagnostics(), > AST->ASTFileLangOpts, > @@ -1077,7 +1080,8 @@ > LangOpts = &Clang->getLangOpts(); > FileSystemOpts = Clang->getFileSystemOpts(); > FileMgr = new FileManager(FileSystemOpts); > - SourceMgr = new SourceManager(getDiagnostics(), *FileMgr); > + SourceMgr = new SourceManager(getDiagnostics(), *FileMgr, > + UserFilesAreVolatile); > TheSema.reset(); > Ctx = 0; > PP = 0; > @@ -1665,7 +1669,8 @@ > > ASTUnit *ASTUnit::create(CompilerInvocation *CI, > IntrusiveRefCntPtr<DiagnosticsEngine> Diags, > - bool CaptureDiagnostics) { > + bool CaptureDiagnostics, > + bool UserFilesAreVolatile) { > OwningPtr<ASTUnit> AST; > AST.reset(new ASTUnit(false)); > ConfigureDiags(Diags, 0, 0, *AST, CaptureDiagnostics); > @@ -1673,7 +1678,9 @@ > AST->Invocation = CI; > AST->FileSystemOpts = CI->getFileSystemOpts(); > AST->FileMgr = new FileManager(AST->FileSystemOpts); > - AST->SourceMgr = new SourceManager(AST->getDiagnostics(), *AST->FileMgr); > + AST->UserFilesAreVolatile = UserFilesAreVolatile; > + AST->SourceMgr = new SourceManager(AST->getDiagnostics(), *AST->FileMgr, > + UserFilesAreVolatile); > > return AST.take(); > } > @@ -1689,6 +1696,7 @@ > bool PrecompilePreamble, > bool > CacheCodeCompletionResults, > bool > IncludeBriefCommentsInCodeCompletion, > + bool UserFilesAreVolatile, > OwningPtr<ASTUnit> *ErrAST) { > assert(CI && "A CompilerInvocation is required"); > > @@ -1696,7 +1704,7 @@ > ASTUnit *AST = Unit; > if (!AST) { > // Create the AST unit. > - OwnAST.reset(create(CI, Diags, CaptureDiagnostics)); > + OwnAST.reset(create(CI, Diags, CaptureDiagnostics, > UserFilesAreVolatile)); > AST = OwnAST.get(); > } > > @@ -1859,7 +1867,8 @@ > bool PrecompilePreamble, > TranslationUnitKind TUKind, > bool > CacheCodeCompletionResults, > - bool > IncludeBriefCommentsInCodeCompletion) { > + bool > IncludeBriefCommentsInCodeCompletion, > + bool UserFilesAreVolatile) { > // Create the AST unit. > OwningPtr<ASTUnit> AST; > AST.reset(new ASTUnit(false)); > @@ -1872,6 +1881,7 @@ > AST->IncludeBriefCommentsInCodeCompletion > = IncludeBriefCommentsInCodeCompletion; > AST->Invocation = CI; > + AST->UserFilesAreVolatile = UserFilesAreVolatile; > > // Recover resources if we crash before exiting this method. > llvm::CrashRecoveryContextCleanupRegistrar<ASTUnit> > @@ -1898,6 +1908,7 @@ > bool > IncludeBriefCommentsInCodeCompletion, > bool AllowPCHWithCompilerErrors, > bool SkipFunctionBodies, > + bool UserFilesAreVolatile, > OwningPtr<ASTUnit> *ErrAST) { > if (!Diags.getPtr()) { > // No diagnostics engine was provided, so create our own diagnostics > object > @@ -1957,6 +1968,7 @@ > AST->ShouldCacheCodeCompletionResults = CacheCodeCompletionResults; > AST->IncludeBriefCommentsInCodeCompletion > = IncludeBriefCommentsInCodeCompletion; > + AST->UserFilesAreVolatile = UserFilesAreVolatile; > AST->NumStoredDiagnosticsFromDriver = StoredDiagnostics.size(); > AST->StoredDiagnostics.swap(StoredDiagnostics); > AST->Invocation = CI; > > Modified: cfe/trunk/lib/Serialization/ASTReader.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=160074&r1=160073&r2=160074&view=diff > ============================================================================== > --- cfe/trunk/lib/Serialization/ASTReader.cpp (original) > +++ cfe/trunk/lib/Serialization/ASTReader.cpp Wed Jul 11 15:59:04 2012 > @@ -1127,8 +1127,9 @@ > // This is the module's main file. > IncludeLoc = getImportLocation(F); > } > - FileID FID = SourceMgr.createFileID(File, IncludeLoc, > - > (SrcMgr::CharacteristicKind)Record[2], > + SrcMgr::CharacteristicKind > + FileCharacter = (SrcMgr::CharacteristicKind)Record[2]; > + FileID FID = SourceMgr.createFileID(File, IncludeLoc, FileCharacter, > ID, BaseOffset + Record[0]); > SrcMgr::FileInfo &FileInfo = > > const_cast<SrcMgr::FileInfo&>(SourceMgr.getSLocEntry(FID).getFile()); > @@ -1145,7 +1146,8 @@ > } > > const SrcMgr::ContentCache *ContentCache > - = SourceMgr.getOrCreateContentCache(File); > + = SourceMgr.getOrCreateContentCache(File, > + /*isSystemFile=*/FileCharacter != > SrcMgr::C_User); > if (OverriddenBuffer && !ContentCache->BufferOverridden && > ContentCache->ContentsEntry == ContentCache->OrigEntry) { > unsigned Code = SLocEntryCursor.ReadCode(); > > Modified: cfe/trunk/tools/libclang/CIndex.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/CIndex.cpp?rev=160074&r1=160073&r2=160074&view=diff > ============================================================================== > --- cfe/trunk/tools/libclang/CIndex.cpp (original) > +++ cfe/trunk/tools/libclang/CIndex.cpp Wed Jul 11 15:59:04 2012 > @@ -2464,7 +2464,8 @@ > CXXIdx->getOnlyLocalDecls(), > 0, 0, > /*CaptureDiagnostics=*/true, > - /*AllowPCHWithCompilerErrors=*/true); > + /*AllowPCHWithCompilerErrors=*/true, > + /*UserFilesAreVolatile=*/true); > return MakeCXTranslationUnit(CXXIdx, TU); > } > > @@ -2612,6 +2613,7 @@ > IncludeBriefCommentsInCodeCompletion, > /*AllowPCHWithCompilerErrors=*/true, > SkipFunctionBodies, > + /*UserFilesAreVolatile=*/true, > &ErrUnit)); > > if (NumErrors != Diags->getClient()->getNumErrors()) { > > Modified: cfe/trunk/tools/libclang/Indexing.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/Indexing.cpp?rev=160074&r1=160073&r2=160074&view=diff > ============================================================================== > --- cfe/trunk/tools/libclang/Indexing.cpp (original) > +++ cfe/trunk/tools/libclang/Indexing.cpp Wed Jul 11 15:59:04 2012 > @@ -353,7 +353,8 @@ > CInvok->getDiagnosticOpts().IgnoreWarnings = true; > > ASTUnit *Unit = ASTUnit::create(CInvok.getPtr(), Diags, > - /*CaptureDiagnostics=*/true); > + /*CaptureDiagnostics=*/true, > + /*UserFilesAreVolatile=*/true); > OwningPtr<CXTUOwner> CXTU(new CXTUOwner(MakeCXTranslationUnit(CXXIdx, > Unit))); > > // Recover resources if we crash before exiting this method. > @@ -396,7 +397,9 @@ > OnlyLocalDecls, > > /*CaptureDiagnostics=*/true, > PrecompilePreamble, > - > CacheCodeCompletionResults); > + > CacheCodeCompletionResults, > + > /*IncludeBriefCommentsInCodeCompletion=*/false, > + > /*UserFilesAreVolatile=*/true); > if (DiagTrap.hasErrorOccurred() && CXXIdx->getDisplayDiagnostics()) > printDiagsToStderr(Unit); > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
