r367829 - [CrossTU][NFCI] Refactor loadExternalAST function
Nico Weber via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 5 07:03:56 PDT 2019
The msan bot doesn't like this, it reports an uninitialized read a
t clang/lib/CrossTU/CrossTranslationUnit.cpp :
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/34087/steps/check-clang%20msan/logs/stdio
********************
Testing: 0
FAIL: Clang :: Analysis/ctu-unknown-parts-in-triples.cpp (492 of 15321)
******************** TEST 'Clang ::
Analysis/ctu-unknown-parts-in-triples.cpp' FAILED ********************
Script:
--
: 'RUN: at line 4'; rm -rf
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/clang/test/Analysis/Output/ctu-unknown-parts-in-triples.cpp.tmp
&& mkdir
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/clang/test/Analysis/Output/ctu-unknown-parts-in-triples.cpp.tmp
: 'RUN: at line 5'; mkdir -p
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/clang/test/Analysis/Output/ctu-unknown-parts-in-triples.cpp.tmp/ctudir
: 'RUN: at line 6';
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1
-internal-isystem
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/10.0.0/include
-nostdsysteminc -triple x86_64-pc-linux-gnu -emit-pch -o
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/clang/test/Analysis/Output/ctu-unknown-parts-in-triples.cpp.tmp/ctudir/ctu-other.cpp.ast
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/Inputs/ctu-other.cpp
: 'RUN: at line 8'; cp
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/clang/test/Analysis/Output/ctu-unknown-parts-in-triples.cpp.tmp/ctudir/externalDefMap.txt
: 'RUN: at line 9';
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1
-internal-isystem
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/10.0.0/include
-nostdsysteminc -analyze -analyzer-constraints=range -triple
x86_64-unknown-linux-gnu -analyzer-checker=core,debug.ExprInspection
-analyzer-config experimental-enable-naive-ctu-analysis=true
-analyzer-config
ctu-dir=/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/clang/test/Analysis/Output/ctu-unknown-parts-in-triples.cpp.tmp/ctudir
-Werror=ctu -verify
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/ctu-unknown-parts-in-triples.cpp
--
Exit Code: 77
Command Output (stderr):
--
==5072==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0xb05c3c4 in
clang::cross_tu::CrossTranslationUnitContext::loadExternalAST(llvm::StringRef,
llvm::StringRef, llvm::StringRef, bool)
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/CrossTU/CrossTranslationUnit.cpp:467:7
#1 0xb053a98 in llvm::Expected<clang::FunctionDecl const*>
clang::cross_tu::CrossTranslationUnitContext::getCrossTUDefinitionImpl<clang::FunctionDecl>(clang::FunctionDecl
const*, llvm::StringRef, llvm::StringRef, bool)
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/CrossTU/CrossTranslationUnit.cpp:241:7
#2 0xb053466 in
clang::cross_tu::CrossTranslationUnitContext::getCrossTUDefinition(clang::FunctionDecl
const*, llvm::StringRef, llvm::StringRef, bool)
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/CrossTU/CrossTranslationUnit.cpp:307:10
#3 0xadb69f5 in clang::ento::AnyFunctionCall::getRuntimeDefinition()
const
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Core/CallEvent.cpp:575:14
On Mon, Aug 5, 2019 at 7:05 AM Endre Fulop via cfe-commits <
cfe-commits at lists.llvm.org> wrote:
> Author: gamesh411
> Date: Mon Aug 5 04:06:41 2019
> New Revision: 367829
>
> URL: http://llvm.org/viewvc/llvm-project?rev=367829&view=rev
> Log:
> [CrossTU][NFCI] Refactor loadExternalAST function
>
> Summary:
> Refactor loadExternalAST method of CrossTranslationUnitContext in order to
> reduce maintenance burden and so that features are easier to add in the
> future.
>
> Reviewers: martong
>
> Subscribers: rnkovacs, dkrupp, Szelethus, cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D64753
>
> Modified:
> cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h
> cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
>
> Modified: cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h?rev=367829&r1=367828&r2=367829&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h (original)
> +++ cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h Mon Aug 5
> 04:06:41 2019
> @@ -192,11 +192,11 @@ private:
> template <typename T>
> llvm::Expected<const T *> importDefinitionImpl(const T *D, ASTUnit
> *Unit);
>
> - llvm::StringMap<std::unique_ptr<clang::ASTUnit>> FileASTUnitMap;
> - llvm::StringMap<clang::ASTUnit *> NameASTUnitMap;
> - llvm::StringMap<std::string> NameFileMap;
> - llvm::DenseMap<TranslationUnitDecl *, std::unique_ptr<ASTImporter>>
> - ASTUnitImporterMap;
> + using ImporterMapTy =
> + llvm::DenseMap<TranslationUnitDecl *, std::unique_ptr<ASTImporter>>;
> +
> + ImporterMapTy ASTUnitImporterMap;
> +
> CompilerInstance &CI;
> ASTContext &Context;
> std::shared_ptr<ASTImporterSharedState> ImporterSharedSt;
> @@ -209,10 +209,105 @@ private:
> /// imported the FileID.
> ImportedFileIDMap ImportedFileIDs;
>
> + /// Functor for loading ASTUnits from AST-dump files.
> + class ASTFileLoader {
> + public:
> + ASTFileLoader(const CompilerInstance &CI);
> + std::unique_ptr<ASTUnit> operator()(StringRef ASTFilePath);
> +
> + private:
> + const CompilerInstance &CI;
> + };
> +
> + /// Storage for ASTUnits, cached access, and providing searchability
> are the
> + /// concerns of ASTUnitStorage class.
> + class ASTUnitStorage {
> + public:
> + ASTUnitStorage(const CompilerInstance &CI);
> + /// Loads an ASTUnit for a function.
> + ///
> + /// \param FuncitionName USR name of the function.
> + /// \param CrossTUDir Path to the directory used to store CTU related
> files.
> + /// \param IndexName Name of the file inside \p CrossTUDir which maps
> + /// function USR names to file paths. These files contain the
> corresponding
> + /// AST-dumps.
> + ///
> + /// \return An Expected instance which contains the ASTUnit pointer
> or the
> + /// error occured during the load.
> + llvm::Expected<ASTUnit *> getASTUnitForFunction(StringRef
> FunctionName,
> + StringRef CrossTUDir,
> + StringRef IndexName);
> + /// Identifies the path of the file which can be used to load the
> ASTUnit
> + /// for a given function.
> + ///
> + /// \param FuncitionName USR name of the function.
> + /// \param CrossTUDir Path to the directory used to store CTU related
> files.
> + /// \param IndexName Name of the file inside \p CrossTUDir which maps
> + /// function USR names to file paths. These files contain the
> corresponding
> + /// AST-dumps.
> + ///
> + /// \return An Expected instance containing the filepath.
> + llvm::Expected<std::string> getFileForFunction(StringRef FunctionName,
> + StringRef CrossTUDir,
> + StringRef IndexName);
> +
> + private:
> + llvm::Error ensureCTUIndexLoaded(StringRef CrossTUDir, StringRef
> IndexName);
> + llvm::Expected<ASTUnit *> getASTUnitForFile(StringRef FileName);
> +
> + template <typename... T> using BaseMapTy = llvm::StringMap<T...>;
> + using OwningMapTy = BaseMapTy<std::unique_ptr<clang::ASTUnit>>;
> + using NonOwningMapTy = BaseMapTy<clang::ASTUnit *>;
> +
> + OwningMapTy FileASTUnitMap;
> + NonOwningMapTy NameASTUnitMap;
> +
> + using IndexMapTy = BaseMapTy<std::string>;
> + IndexMapTy NameFileMap;
> +
> + ASTFileLoader FileAccessor;
> + };
> +
> + ASTUnitStorage ASTStorage;
> +
> /// \p CTULoadTreshold should serve as an upper limit to the number of
> TUs
> /// imported in order to reduce the memory footprint of CTU analysis.
> const unsigned CTULoadThreshold;
> - unsigned NumASTLoaded{0u};
> +
> + /// The number successfully loaded ASTs. Used to indicate, and - with
> the
> + /// appropriate threshold value - limit the memory usage of the
> + /// CrossTranslationUnitContext.
> + unsigned NumASTLoaded;
> +
> + /// RAII counter to signal 'threshold reached' condition, and to
> increment the
> + /// NumASTLoaded counter upon a successful load.
> + class LoadGuard {
> + public:
> + LoadGuard(unsigned Limit, unsigned &Counter)
> + : Counter(Counter), Enabled(Counter < Limit){};
> + ~LoadGuard() {
> + if (StoreSuccess)
> + ++Counter;
> + }
> + /// Flag the LoadGuard instance as successful, meaning that the load
> + /// operation succeeded, and the memory footprint of the AST storage
> + /// actually increased. In this case, \p Counter should be
> incremented upon
> + /// destruction.
> + void storedSuccessfully() { StoreSuccess = true; }
> + /// Indicates, whether a new load operation is permitted, it is
> within the
> + /// threshold.
> + operator bool() const { return Enabled; };
> +
> + private:
> + /// The number of ASTs actually imported. LoadGuard does not own the
> + /// counter, just uses on given to it at construction time.
> + unsigned &Counter;
> + /// Indicates whether a load operation can begin, which is equivalent
> to the
> + /// 'threshold not reached' condition.
> + bool Enabled;
> + /// Shows the state of the current load operation.
> + bool StoreSuccess;
> + };
> };
>
> } // namespace cross_tu
>
> Modified: cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp?rev=367829&r1=367828&r2=367829&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp (original)
> +++ cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp Mon Aug 5 04:06:41 2019
> @@ -188,7 +188,7 @@ template <typename T> static bool hasBod
> }
>
> CrossTranslationUnitContext::CrossTranslationUnitContext(CompilerInstance
> &CI)
> - : CI(CI), Context(CI.getASTContext()),
> + : CI(CI), Context(CI.getASTContext()), ASTStorage(CI),
> CTULoadThreshold(CI.getAnalyzerOpts()->CTUImportThreshold) {}
>
> CrossTranslationUnitContext::~CrossTranslationUnitContext() {}
> @@ -237,8 +237,8 @@ llvm::Expected<const T *> CrossTranslati
> if (LookupName.empty())
> return llvm::make_error<IndexError>(
> index_error_code::failed_to_generate_usr);
> - llvm::Expected<ASTUnit *> ASTUnitOrError = loadExternalAST(
> - LookupName, CrossTUDir, IndexName, DisplayCTUProgress);
> + llvm::Expected<ASTUnit *> ASTUnitOrError =
> + loadExternalAST(LookupName, CrossTUDir, IndexName,
> DisplayCTUProgress);
> if (!ASTUnitOrError)
> return ASTUnitOrError.takeError();
> ASTUnit *Unit = *ASTUnitOrError;
> @@ -340,6 +340,118 @@ void CrossTranslationUnitContext::emitCr
> }
> }
>
> +CrossTranslationUnitContext::ASTFileLoader::ASTFileLoader(
> + const CompilerInstance &CI)
> + : CI(CI) {}
> +
> +std::unique_ptr<ASTUnit>
> +CrossTranslationUnitContext::ASTFileLoader::operator()(StringRef
> ASTFilePath) {
> + // Load AST from ast-dump.
> + IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new
> DiagnosticOptions();
> + TextDiagnosticPrinter *DiagClient =
> + new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts);
> + IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
> + IntrusiveRefCntPtr<DiagnosticsEngine> Diags(
> + new DiagnosticsEngine(DiagID, &*DiagOpts, DiagClient));
> +
> + return ASTUnit::LoadFromASTFile(
> + ASTFilePath, CI.getPCHContainerOperations()->getRawReader(),
> + ASTUnit::LoadEverything, Diags, CI.getFileSystemOpts());
> +}
> +
> +CrossTranslationUnitContext::ASTUnitStorage::ASTUnitStorage(
> + const CompilerInstance &CI)
> + : FileAccessor(CI) {}
> +
> +llvm::Expected<ASTUnit *>
> +CrossTranslationUnitContext::ASTUnitStorage::getASTUnitForFile(StringRef
> FileName) {
> + // Try the cache first.
> + auto ASTCacheEntry = FileASTUnitMap.find(FileName);
> + if (ASTCacheEntry == FileASTUnitMap.end()) {
> + // Load the ASTUnit from the pre-dumped AST file specified by
> ASTFileName.
> + std::unique_ptr<ASTUnit> LoadedUnit = FileAccessor(FileName);
> +
> + // Need the raw pointer and the unique_ptr as well.
> + ASTUnit* Unit = LoadedUnit.get();
> +
> + // Update the cache.
> + FileASTUnitMap[FileName] = std::move(LoadedUnit);
> + return Unit;
> +
> + } else {
> + // Found in the cache.
> + return ASTCacheEntry->second.get();
> + }
> +}
> +
> +llvm::Expected<ASTUnit *>
> +CrossTranslationUnitContext::ASTUnitStorage::getASTUnitForFunction(
> + StringRef FunctionName, StringRef CrossTUDir, StringRef IndexName) {
> + // Try the cache first.
> + auto ASTCacheEntry = NameASTUnitMap.find(FunctionName);
> + if (ASTCacheEntry == NameASTUnitMap.end()) {
> + // Load the ASTUnit from the pre-dumped AST file specified by
> ASTFileName.
> +
> + // Ensure that the Index is loaded, as we need to search in it.
> + if (llvm::Error IndexLoadError =
> + ensureCTUIndexLoaded(CrossTUDir, IndexName))
> + return std::move(IndexLoadError);
> +
> + // Check if there is and entry in the index for the function.
> + if (!NameFileMap.count(FunctionName)) {
> + ++NumNotInOtherTU;
> + return
> llvm::make_error<IndexError>(index_error_code::missing_definition);
> + }
> +
> + // Search in the index for the filename where the definition of
> FuncitonName
> + // resides.
> + if (llvm::Expected<ASTUnit *> FoundForFile =
> + getASTUnitForFile(NameFileMap[FunctionName])) {
> +
> + // Update the cache.
> + NameASTUnitMap[FunctionName] = *FoundForFile;
> + return *FoundForFile;
> +
> + } else {
> + return FoundForFile.takeError();
> + }
> + } else {
> + // Found in the cache.
> + return ASTCacheEntry->second;
> + }
> +}
> +
> +llvm::Expected<std::string>
> +CrossTranslationUnitContext::ASTUnitStorage::getFileForFunction(
> + StringRef FunctionName, StringRef CrossTUDir, StringRef IndexName) {
> + if (llvm::Error IndexLoadError = ensureCTUIndexLoaded(CrossTUDir,
> IndexName))
> + return std::move(IndexLoadError);
> + return NameFileMap[FunctionName];
> +}
> +
> +llvm::Error
> CrossTranslationUnitContext::ASTUnitStorage::ensureCTUIndexLoaded(
> + StringRef CrossTUDir, StringRef IndexName) {
> + // Dont initialize if the map is filled.
> + if (!NameFileMap.empty())
> + return llvm::Error::success();
> +
> + // Get the absolute path to the index file.
> + SmallString<256> IndexFile = CrossTUDir;
> + if (llvm::sys::path::is_absolute(IndexName))
> + IndexFile = IndexName;
> + else
> + llvm::sys::path::append(IndexFile, IndexName);
> +
> + if (auto IndexMapping = parseCrossTUIndex(IndexFile, CrossTUDir)) {
> + // Initialize member map.
> + NameFileMap = *IndexMapping;
> + return llvm::Error::success();
> + } else {
> + // Error while parsing CrossTU index file.
> + return IndexMapping.takeError();
> + };
> +}
> +
> llvm::Expected<ASTUnit *> CrossTranslationUnitContext::loadExternalAST(
> StringRef LookupName, StringRef CrossTUDir, StringRef IndexName,
> bool DisplayCTUProgress) {
> @@ -348,64 +460,41 @@ llvm::Expected<ASTUnit *> CrossTranslati
> // translation units contains decls with the same lookup name an
> // error will be returned.
>
> - if (NumASTLoaded >= CTULoadThreshold) {
> + // RAII incrementing counter is used to count successful loads.
> + LoadGuard LoadOperation(CTULoadThreshold, NumASTLoaded);
> +
> + // If import threshold is reached, don't import anything.
> + if (!LoadOperation) {
> ++NumASTLoadThresholdReached;
> return llvm::make_error<IndexError>(
> index_error_code::load_threshold_reached);
> }
>
> - ASTUnit *Unit = nullptr;
> - auto NameUnitCacheEntry = NameASTUnitMap.find(LookupName);
> - if (NameUnitCacheEntry == NameASTUnitMap.end()) {
> - if (NameFileMap.empty()) {
> - SmallString<256> IndexFile = CrossTUDir;
> - if (llvm::sys::path::is_absolute(IndexName))
> - IndexFile = IndexName;
> - else
> - llvm::sys::path::append(IndexFile, IndexName);
> - llvm::Expected<llvm::StringMap<std::string>> IndexOrErr =
> - parseCrossTUIndex(IndexFile, CrossTUDir);
> - if (IndexOrErr)
> - NameFileMap = *IndexOrErr;
> - else
> - return IndexOrErr.takeError();
> - }
> + // Try to get the value from the heavily cached storage.
> + llvm::Expected<ASTUnit *> Unit =
> + ASTStorage.getASTUnitForFunction(LookupName, CrossTUDir, IndexName);
>
> - auto It = NameFileMap.find(LookupName);
> - if (It == NameFileMap.end()) {
> - ++NumNotInOtherTU;
> - return
> llvm::make_error<IndexError>(index_error_code::missing_definition);
> - }
> - StringRef ASTFileName = It->second;
> - auto ASTCacheEntry = FileASTUnitMap.find(ASTFileName);
> - if (ASTCacheEntry == FileASTUnitMap.end()) {
> - IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new
> DiagnosticOptions();
> - TextDiagnosticPrinter *DiagClient =
> - new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts);
> - IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
> - IntrusiveRefCntPtr<DiagnosticsEngine> Diags(
> - new DiagnosticsEngine(DiagID, &*DiagOpts, DiagClient));
> -
> - std::unique_ptr<ASTUnit> LoadedUnit(ASTUnit::LoadFromASTFile(
> - ASTFileName, CI.getPCHContainerOperations()->getRawReader(),
> - ASTUnit::LoadEverything, Diags, CI.getFileSystemOpts()));
> - Unit = LoadedUnit.get();
> - FileASTUnitMap[ASTFileName] = std::move(LoadedUnit);
> - ++NumASTLoaded;
> - if (DisplayCTUProgress) {
> - llvm::errs() << "CTU loaded AST file: "
> - << ASTFileName << "\n";
> - }
> - } else {
> - Unit = ASTCacheEntry->second.get();
> - }
> - NameASTUnitMap[LookupName] = Unit;
> - } else {
> - Unit = NameUnitCacheEntry->second;
> - }
> if (!Unit)
> + return Unit.takeError();
> +
> + // Check whether the backing pointer of the Expected is a nullptr.
> + if (!*Unit)
> return llvm::make_error<IndexError>(
> index_error_code::failed_to_get_external_ast);
> +
> + // The backing pointer is not null, loading was successful. If anything
> goes
> + // wrong from this point on, the AST is already stored, so the load
> part is
> + // finished.
> + LoadOperation.storedSuccessfully();
> +
> + if (DisplayCTUProgress) {
> + if (llvm::Expected<std::string> FileName =
> + ASTStorage.getFileForFunction(LookupName, CrossTUDir,
> IndexName))
> + llvm::errs() << "CTU loaded AST file: " << *FileName << "\n";
> + else
> + return FileName.takeError();
> + }
> +
> return Unit;
> }
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190805/dcda5698/attachment-0001.html>
More information about the cfe-commits
mailing list