Reverted in r366453 On Thu, Jul 18, 2019 at 5:38 PM Ilya Biryukov <ibiryu...@google.com> wrote:
> Hi Balazs, > > Your commit introduces a layering violation: ASTImporter (which is inside > clangAST library) cannot depend on ASTUnit (which is inside clangFrontend > library). > I'm afraid I'll have to revert the commit to unbreak our integrate. > > On Thu, Jul 18, 2019 at 5:22 PM Balazs Keri via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: balazske >> Date: Thu Jul 18 08:23:10 2019 >> New Revision: 366449 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=366449&view=rev >> Log: >> [CrossTU] Add a function to retrieve original source location. >> >> Summary: >> A new function will be added to get the original SourceLocation >> for a SourceLocation that was imported as result of getCrossTUDefinition. >> The returned SourceLocation is in the context of the (original) >> SourceManager for the original source file. Additionally the >> ASTUnit object for that source file is returned. This is needed >> to get a SourceManager to operate on with the returned source location. >> >> The new function works if multiple different source files are loaded >> with the same CrossTU context. >> >> This patch can be treated as part of a bigger change that is needed to >> improve macro expansion handliong at plist generation. >> >> Reviewers: martong, shafik, a_sidorin, xazax.hun >> >> Reviewed By: martong >> >> Subscribers: rnkovacs, dkrupp, Szelethus, gamesh411, cfe-commits >> >> Tags: #clang >> >> Differential Revision: https://reviews.llvm.org/D64554 >> >> Modified: >> cfe/trunk/include/clang/AST/ASTImporter.h >> cfe/trunk/include/clang/AST/ASTImporterSharedState.h >> cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h >> cfe/trunk/lib/AST/ASTImporter.cpp >> cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp >> cfe/trunk/unittests/CrossTU/CrossTranslationUnitTest.cpp >> >> Modified: cfe/trunk/include/clang/AST/ASTImporter.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTImporter.h?rev=366449&r1=366448&r2=366449&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/AST/ASTImporter.h (original) >> +++ cfe/trunk/include/clang/AST/ASTImporter.h Thu Jul 18 08:23:10 2019 >> @@ -34,6 +34,7 @@ namespace clang { >> >> class ASTContext; >> class ASTImporterSharedState; >> +class ASTUnit; >> class Attr; >> class CXXBaseSpecifier; >> class CXXCtorInitializer; >> @@ -229,6 +230,11 @@ class TypeSourceInfo; >> /// The file managers we're importing to and from. >> FileManager &ToFileManager, &FromFileManager; >> >> + /// The ASTUnit for the From context, if any. >> + /// This is used to create a mapping of imported ('To') FileID's to >> the >> + /// original ('From') FileID and ASTUnit. >> + ASTUnit *FromUnit; >> + >> /// Whether to perform a minimal import. >> bool Minimal; >> >> @@ -277,6 +283,11 @@ class TypeSourceInfo; >> >> void AddToLookupTable(Decl *ToD); >> >> + ASTImporter(ASTContext &ToContext, FileManager &ToFileManager, >> + ASTContext &FromContext, FileManager &FromFileManager, >> + ASTUnit *FromUnit, bool MinimalImport, >> + std::shared_ptr<ASTImporterSharedState> SharedState); >> + >> protected: >> /// Can be overwritten by subclasses to implement their own import >> logic. >> /// The overwritten method should call this method if it didn't >> import the >> @@ -287,7 +298,6 @@ class TypeSourceInfo; >> virtual bool returnWithErrorInTest() { return false; }; >> >> public: >> - >> /// \param ToContext The context we'll be importing into. >> /// >> /// \param ToFileManager The file manager we'll be importing into. >> @@ -307,6 +317,23 @@ class TypeSourceInfo; >> ASTContext &FromContext, FileManager &FromFileManager, >> bool MinimalImport, >> std::shared_ptr<ASTImporterSharedState> SharedState = >> nullptr); >> + /// \param ToContext The context we'll be importing into. >> + /// >> + /// \param ToFileManager The file manager we'll be importing into. >> + /// >> + /// \param FromUnit Pointer to an ASTUnit that contains the context >> and >> + /// file manager to import from. >> + /// >> + /// \param MinimalImport If true, the importer will attempt to import >> + /// as little as it can, e.g., by importing declarations as forward >> + /// declarations that can be completed at a later point. >> + /// >> + /// \param SharedState The importer specific lookup table which may >> be >> + /// shared amongst several ASTImporter objects. >> + /// If not set then the original C/C++ lookup is used. >> + ASTImporter(ASTContext &ToContext, FileManager &ToFileManager, >> + ASTUnit &FromUnit, bool MinimalImport, >> + std::shared_ptr<ASTImporterSharedState> SharedState = >> nullptr); >> >> virtual ~ASTImporter(); >> >> >> Modified: cfe/trunk/include/clang/AST/ASTImporterSharedState.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTImporterSharedState.h?rev=366449&r1=366448&r2=366449&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/AST/ASTImporterSharedState.h (original) >> +++ cfe/trunk/include/clang/AST/ASTImporterSharedState.h Thu Jul 18 >> 08:23:10 2019 >> @@ -17,18 +17,24 @@ >> >> #include "clang/AST/ASTImporterLookupTable.h" >> #include "clang/AST/Decl.h" >> +#include "clang/Basic/SourceLocation.h" >> #include "llvm/ADT/DenseMap.h" >> // FIXME We need this because of ImportError. >> #include "clang/AST/ASTImporter.h" >> >> namespace clang { >> >> +class ASTUnit; >> class TranslationUnitDecl; >> >> /// Importer specific state, which may be shared amongst several >> ASTImporter >> /// objects. >> class ASTImporterSharedState { >> +public: >> + using ImportedFileIDMap = >> + llvm::DenseMap<FileID, std::pair<FileID, ASTUnit *>>; >> >> +private: >> /// Pointer to the import specific lookup table. >> std::unique_ptr<ASTImporterLookupTable> LookupTable; >> >> @@ -43,6 +49,16 @@ class ASTImporterSharedState { >> // FIXME put ImportedFromDecls here! >> // And from that point we can better encapsulate the lookup table. >> >> + /// Map of imported FileID's (in "To" context) to FileID in "From" >> context >> + /// and the ASTUnit that contains the preprocessor and source manager >> for the >> + /// "From" FileID. This map is used to lookup a FileID and its >> SourceManager >> + /// when knowing only the FileID in the 'To' context. The FileID could >> be >> + /// imported by any of multiple ASTImporter objects. The map is used >> because >> + /// we do not want to loop over all ASTImporter's to find the one that >> + /// imported the FileID. (The ASTUnit is usable to get the >> SourceManager and >> + /// additional data.) >> + ImportedFileIDMap ImportedFileIDs; >> + >> public: >> ASTImporterSharedState() = default; >> >> @@ -75,6 +91,12 @@ public: >> void setImportDeclError(Decl *To, ImportError Error) { >> ImportErrors[To] = Error; >> } >> + >> + ImportedFileIDMap &getImportedFileIDs() { return ImportedFileIDs; } >> + >> + const ImportedFileIDMap &getImportedFileIDs() const { >> + return ImportedFileIDs; >> + } >> }; >> >> } // namespace clang >> >> Modified: cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h?rev=366449&r1=366448&r2=366449&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h (original) >> +++ cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h Thu Jul 18 >> 08:23:10 2019 >> @@ -153,8 +153,10 @@ public: >> /// was passed to the constructor. >> /// >> /// \return Returns the resulting definition or an error. >> - llvm::Expected<const FunctionDecl *> importDefinition(const >> FunctionDecl *FD); >> - llvm::Expected<const VarDecl *> importDefinition(const VarDecl *VD); >> + llvm::Expected<const FunctionDecl *> importDefinition(const >> FunctionDecl *FD, >> + ASTUnit *Unit); >> + llvm::Expected<const VarDecl *> importDefinition(const VarDecl *VD, >> + ASTUnit *Unit); >> >> /// Get a name to identify a named decl. >> static std::string getLookupName(const NamedDecl *ND); >> @@ -162,9 +164,19 @@ public: >> /// Emit diagnostics for the user for potential configuration errors. >> void emitCrossTUDiagnostics(const IndexError &IE); >> >> + /// Determine the original source location in the original TU for an >> + /// imported source location. >> + /// \p ToLoc Source location in the imported-to AST. >> + /// \return Source location in the imported-from AST and the >> corresponding >> + /// ASTUnit. >> + /// If any error happens (ToLoc is a non-imported source location) >> empty is >> + /// returned. >> + llvm::Optional<std::pair<SourceLocation /*FromLoc*/, ASTUnit *>> >> + getImportedFromSourceLocation(const clang::SourceLocation &ToLoc) >> const; >> + >> private: >> void lazyInitImporterSharedSt(TranslationUnitDecl *ToTU); >> - ASTImporter &getOrCreateASTImporter(ASTContext &From); >> + ASTImporter &getOrCreateASTImporter(ASTUnit *Unit); >> template <typename T> >> llvm::Expected<const T *> getCrossTUDefinitionImpl(const T *D, >> StringRef >> CrossTUDir, >> @@ -174,7 +186,7 @@ private: >> const T *findDefInDeclContext(const DeclContext *DC, >> StringRef LookupName); >> template <typename T> >> - llvm::Expected<const T *> importDefinitionImpl(const T *D); >> + llvm::Expected<const T *> importDefinitionImpl(const T *D, ASTUnit >> *Unit); >> >> llvm::StringMap<std::unique_ptr<clang::ASTUnit>> FileASTUnitMap; >> llvm::StringMap<clang::ASTUnit *> NameASTUnitMap; >> >> Modified: cfe/trunk/lib/AST/ASTImporter.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTImporter.cpp?rev=366449&r1=366448&r2=366449&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/AST/ASTImporter.cpp (original) >> +++ cfe/trunk/lib/AST/ASTImporter.cpp Thu Jul 18 08:23:10 2019 >> @@ -12,9 +12,9 @@ >> >> >> //===----------------------------------------------------------------------===// >> >> #include "clang/AST/ASTImporter.h" >> -#include "clang/AST/ASTImporterSharedState.h" >> #include "clang/AST/ASTContext.h" >> #include "clang/AST/ASTDiagnostic.h" >> +#include "clang/AST/ASTImporterSharedState.h" >> #include "clang/AST/ASTStructuralEquivalence.h" >> #include "clang/AST/Attr.h" >> #include "clang/AST/Decl.h" >> @@ -52,13 +52,14 @@ >> #include "clang/Basic/SourceLocation.h" >> #include "clang/Basic/SourceManager.h" >> #include "clang/Basic/Specifiers.h" >> +#include "clang/Frontend/ASTUnit.h" >> #include "llvm/ADT/APSInt.h" >> #include "llvm/ADT/ArrayRef.h" >> #include "llvm/ADT/DenseMap.h" >> #include "llvm/ADT/None.h" >> #include "llvm/ADT/Optional.h" >> -#include "llvm/ADT/ScopeExit.h" >> #include "llvm/ADT/STLExtras.h" >> +#include "llvm/ADT/ScopeExit.h" >> #include "llvm/ADT/SmallVector.h" >> #include "llvm/Support/Casting.h" >> #include "llvm/Support/ErrorHandling.h" >> @@ -7716,9 +7717,22 @@ ASTImporter::ASTImporter(ASTContext &ToC >> ASTContext &FromContext, FileManager >> &FromFileManager, >> bool MinimalImport, >> std::shared_ptr<ASTImporterSharedState> >> SharedState) >> + : ASTImporter(ToContext, ToFileManager, FromContext, FromFileManager, >> + nullptr, MinimalImport, SharedState){} >> +ASTImporter::ASTImporter(ASTContext &ToContext, FileManager >> &ToFileManager, >> + ASTUnit &FromUnit, bool MinimalImport, >> + std::shared_ptr<ASTImporterSharedState> >> SharedState) >> + : ASTImporter(ToContext, ToFileManager, FromUnit.getASTContext(), >> + FromUnit.getFileManager(), &FromUnit, MinimalImport, >> + SharedState){} >> + >> +ASTImporter::ASTImporter(ASTContext &ToContext, FileManager >> &ToFileManager, >> + ASTContext &FromContext, FileManager >> &FromFileManager, >> + ASTUnit *FromUnit, bool MinimalImport, >> + std::shared_ptr<ASTImporterSharedState> >> SharedState) >> : SharedState(SharedState), ToContext(ToContext), >> FromContext(FromContext), >> ToFileManager(ToFileManager), FromFileManager(FromFileManager), >> - Minimal(MinimalImport) { >> + FromUnit(FromUnit), Minimal(MinimalImport) { >> >> // Create a default state without the lookup table: LLDB case. >> if (!SharedState) { >> @@ -8421,6 +8435,13 @@ Expected<FileID> ASTImporter::Import(Fil >> assert(ToID.isValid() && "Unexpected invalid fileID was created."); >> >> ImportedFileIDs[FromID] = ToID; >> + if (FromUnit) { >> + assert(SharedState->getImportedFileIDs().find(ToID) == >> + SharedState->getImportedFileIDs().end() && >> + "FileID already imported!"); >> + SharedState->getImportedFileIDs()[ToID] = std::make_pair(FromID, >> FromUnit); >> + } >> + >> return ToID; >> } >> >> >> Modified: cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp?rev=366449&r1=366448&r2=366449&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp (original) >> +++ cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp Thu Jul 18 08:23:10 >> 2019 >> @@ -295,7 +295,7 @@ llvm::Expected<const T *> CrossTranslati >> >> TranslationUnitDecl *TU = >> Unit->getASTContext().getTranslationUnitDecl(); >> if (const T *ResultDecl = findDefInDeclContext<T>(TU, LookupName)) >> - return importDefinition(ResultDecl); >> + return importDefinition(ResultDecl, Unit); >> return llvm::make_error<IndexError>(index_error_code::failed_import); >> } >> >> @@ -411,10 +411,13 @@ llvm::Expected<ASTUnit *> CrossTranslati >> >> template <typename T> >> llvm::Expected<const T *> >> -CrossTranslationUnitContext::importDefinitionImpl(const T *D) { >> +CrossTranslationUnitContext::importDefinitionImpl(const T *D, ASTUnit >> *Unit) { >> assert(hasBodyOrInit(D) && "Decls to be imported should have body or >> init."); >> >> - ASTImporter &Importer = getOrCreateASTImporter(D->getASTContext()); >> + assert(&D->getASTContext() == &Unit->getASTContext() && >> + "ASTContext of Decl and the unit should match."); >> + ASTImporter &Importer = getOrCreateASTImporter(Unit); >> + >> auto ToDeclOrError = Importer.Import(D); >> if (!ToDeclOrError) { >> handleAllErrors(ToDeclOrError.takeError(), >> @@ -441,13 +444,15 @@ CrossTranslationUnitContext::importDefin >> } >> >> llvm::Expected<const FunctionDecl *> >> -CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD) { >> - return importDefinitionImpl(FD); >> +CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD, >> + ASTUnit *Unit) { >> + return importDefinitionImpl(FD, Unit); >> } >> >> llvm::Expected<const VarDecl *> >> -CrossTranslationUnitContext::importDefinition(const VarDecl *VD) { >> - return importDefinitionImpl(VD); >> +CrossTranslationUnitContext::importDefinition(const VarDecl *VD, >> + ASTUnit *Unit) { >> + return importDefinitionImpl(VD, Unit); >> } >> >> void CrossTranslationUnitContext::lazyInitImporterSharedSt( >> @@ -457,17 +462,40 @@ void CrossTranslationUnitContext::lazyIn >> } >> >> ASTImporter & >> -CrossTranslationUnitContext::getOrCreateASTImporter(ASTContext &From) { >> +CrossTranslationUnitContext::getOrCreateASTImporter(ASTUnit *Unit) { >> + ASTContext &From = Unit->getASTContext(); >> + >> auto I = ASTUnitImporterMap.find(From.getTranslationUnitDecl()); >> if (I != ASTUnitImporterMap.end()) >> return *I->second; >> lazyInitImporterSharedSt(Context.getTranslationUnitDecl()); >> - ASTImporter *NewImporter = new ASTImporter( >> - Context, Context.getSourceManager().getFileManager(), From, >> - From.getSourceManager().getFileManager(), false, ImporterSharedSt); >> + ASTImporter *NewImporter = >> + new ASTImporter(Context, >> Context.getSourceManager().getFileManager(), >> + *Unit, false, ImporterSharedSt); >> ASTUnitImporterMap[From.getTranslationUnitDecl()].reset(NewImporter); >> return *NewImporter; >> } >> >> +llvm::Optional<std::pair<SourceLocation, ASTUnit *>> >> +CrossTranslationUnitContext::getImportedFromSourceLocation( >> + const clang::SourceLocation &ToLoc) const { >> + if (!ImporterSharedSt) >> + return {}; >> + >> + const SourceManager &SM = Context.getSourceManager(); >> + auto DecToLoc = SM.getDecomposedLoc(ToLoc); >> + >> + auto I = ImporterSharedSt->getImportedFileIDs().find(DecToLoc.first); >> + if (I == ImporterSharedSt->getImportedFileIDs().end()) >> + return {}; >> + >> + FileID FromID = I->second.first; >> + clang::ASTUnit *Unit = I->second.second; >> + SourceLocation FromLoc = >> + Unit->getSourceManager().getComposedLoc(FromID, DecToLoc.second); >> + >> + return std::make_pair(FromLoc, Unit); >> +} >> + >> } // namespace cross_tu >> } // namespace clang >> >> Modified: cfe/trunk/unittests/CrossTU/CrossTranslationUnitTest.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/CrossTU/CrossTranslationUnitTest.cpp?rev=366449&r1=366448&r2=366449&view=diff >> >> ============================================================================== >> --- cfe/trunk/unittests/CrossTU/CrossTranslationUnitTest.cpp (original) >> +++ cfe/trunk/unittests/CrossTU/CrossTranslationUnitTest.cpp Thu Jul 18 >> 08:23:10 2019 >> @@ -28,13 +28,18 @@ public: >> : CTU(CI), Success(Success) {} >> >> void HandleTranslationUnit(ASTContext &Ctx) { >> + auto FindFInTU = [](const TranslationUnitDecl *TU) { >> + const FunctionDecl *FD = nullptr; >> + for (const Decl *D : TU->decls()) { >> + FD = dyn_cast<FunctionDecl>(D); >> + if (FD && FD->getName() == "f") >> + break; >> + } >> + return FD; >> + }; >> + >> const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl(); >> - const FunctionDecl *FD = nullptr; >> - for (const Decl *D : TU->decls()) { >> - FD = dyn_cast<FunctionDecl>(D); >> - if (FD && FD->getName() == "f") >> - break; >> - } >> + const FunctionDecl *FD = FindFInTU(TU); >> assert(FD && FD->getName() == "f"); >> bool OrigFDHasBody = FD->hasBody(); >> >> @@ -78,6 +83,28 @@ public: >> if (NewFDorError) { >> const FunctionDecl *NewFD = *NewFDorError; >> *Success = NewFD && NewFD->hasBody() && !OrigFDHasBody; >> + >> + if (NewFD) { >> + // Check GetImportedFromSourceLocation. >> + llvm::Optional<std::pair<SourceLocation, ASTUnit *>> SLocResult = >> + CTU.getImportedFromSourceLocation(NewFD->getLocation()); >> + EXPECT_TRUE(SLocResult); >> + if (SLocResult) { >> + SourceLocation OrigSLoc = (*SLocResult).first; >> + ASTUnit *OrigUnit = (*SLocResult).second; >> + // OrigUnit is created internally by CTU (is not the >> + // ASTWithDefinition). >> + TranslationUnitDecl *OrigTU = >> + OrigUnit->getASTContext().getTranslationUnitDecl(); >> + const FunctionDecl *FDWithDefinition = FindFInTU(OrigTU); >> + EXPECT_TRUE(FDWithDefinition); >> + if (FDWithDefinition) { >> + EXPECT_EQ(FDWithDefinition->getName(), "f"); >> + >> EXPECT_TRUE(FDWithDefinition->isThisDeclarationADefinition()); >> + EXPECT_EQ(OrigSLoc, FDWithDefinition->getLocation()); >> + } >> + } >> + } >> } >> } >> >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> > > > -- > Regards, > Ilya Biryukov > -- Regards, Ilya Biryukov
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
