From 740839260b15c75ef1503683cac19c40e9bd1b66 Mon Sep 17 00:00:00 2001 From: John Thompson Date: Wed, 18 Sep 2013 18:19:43 +0000 Subject: [PATCH] Check for #include in extern and namespace blocks. llvm-svn: 190950 --- clang-tools-extra/modularize/Modularize.cpp | 103 +++++++++--- .../modularize/PreprocessorTracker.cpp | 152 +++++++++++++++++- .../modularize/PreprocessorTracker.h | 16 ++ .../test/modularize/Inputs/Empty.h | 1 + .../test/modularize/Inputs/IncludeInExtern.h | 3 + .../modularize/Inputs/IncludeInNamespace.h | 3 + .../modularize/ProblemsExternC.modularize | 12 ++ .../modularize/ProblemsNamespace.modularize | 12 ++ 8 files changed, 278 insertions(+), 24 deletions(-) create mode 100644 clang-tools-extra/test/modularize/Inputs/Empty.h create mode 100644 clang-tools-extra/test/modularize/Inputs/IncludeInExtern.h create mode 100644 clang-tools-extra/test/modularize/Inputs/IncludeInNamespace.h create mode 100644 clang-tools-extra/test/modularize/ProblemsExternC.modularize create mode 100644 clang-tools-extra/test/modularize/ProblemsNamespace.modularize diff --git a/clang-tools-extra/modularize/Modularize.cpp b/clang-tools-extra/modularize/Modularize.cpp index f0b774fe97b1..1790827bfd1d 100644 --- a/clang-tools-extra/modularize/Modularize.cpp +++ b/clang-tools-extra/modularize/Modularize.cpp @@ -75,6 +75,19 @@ // ^ // Macro defined here. // +// Checks will also be performed for '#include' directives that are +// nested inside 'extern "C/C++" {}' or 'namespace (name) {}' blocks, +// and can produce error message like the following: +// +// IncludeInExtern.h:2:3 +// #include "Empty.h" +// ^ +// error: Include directive within extern "C" {}. +// IncludeInExtern.h:1:1 +// extern "C" { +// ^ +// The "extern "C" {}" block is here. +// // See PreprocessorTracker.cpp for additional details. // // Future directions: @@ -84,18 +97,15 @@ // // Some ideas: // -// 1. Check for and warn about "#include" directives inside 'extern "C/C++" {}' -// and "namespace (name) {}" blocks. +// 1. Omit duplicate "not always provided" messages // -// 2. Omit duplicate "not always provided" messages -// -// 3. Add options to disable any of the checks, in case +// 2. Add options to disable any of the checks, in case // there is some problem with them, or the messages get too verbose. // -// 4. Try to figure out the preprocessor conditional directives that +// 3. Try to figure out the preprocessor conditional directives that // contribute to problems and tie them to the inconsistent definitions. // -// 5. There are some legitimate uses of preprocessor macros that +// 4. There are some legitimate uses of preprocessor macros that // modularize will flag as errors, such as repeatedly #include'ing // a file and using interleaving defined/undefined macros // to change declarations in the included file. Is there a way @@ -103,7 +113,7 @@ // to ignore. Otherwise you can just exclude the file, after checking // for legitimate errors. // -// 6. What else? +// 5. What else? // // General clean-up and refactoring: // @@ -453,8 +463,11 @@ private: class CollectEntitiesVisitor : public RecursiveASTVisitor { public: - CollectEntitiesVisitor(SourceManager &SM, EntityMap &Entities) - : SM(SM), Entities(Entities) {} + CollectEntitiesVisitor(SourceManager &SM, EntityMap &Entities, + Preprocessor &PP, PreprocessorTracker &PPTracker, + int &HadErrors) + : SM(SM), Entities(Entities), PP(PP), PPTracker(PPTracker), + HadErrors(HadErrors) {} bool TraverseStmt(Stmt *S) { return true; } bool TraverseType(QualType T) { return true; } @@ -478,6 +491,42 @@ public: bool TraverseConstructorInitializer(CXXCtorInitializer *Init) { return true; } bool TraverseLambdaCapture(LambdaExpr::Capture C) { return true; } + // Check 'extern "*" {}' block for #include directives. + bool VisitLinkageSpecDecl(LinkageSpecDecl *D) { + // Bail if not a block. + if (!D->hasBraces()) + return true; + SourceRange BlockRange = D->getSourceRange(); + const char *LinkageLabel; + switch (D->getLanguage()) { + case LinkageSpecDecl::lang_c: + LinkageLabel = "extern \"C\" {}"; + break; + case LinkageSpecDecl::lang_cxx: + LinkageLabel = "extern \"C++\" {}"; + break; + default: + LinkageLabel = "extern \"\" {}"; + } + if (!PPTracker.checkForIncludesInBlock(PP, BlockRange, LinkageLabel, + errs())) + HadErrors = 1; + return true; + } + + // Check 'namespace (name) {}' block for #include directives. + bool VisitNamespaceDecl(const NamespaceDecl *D) { + SourceRange BlockRange = D->getSourceRange(); + std::string Label("namespace "); + Label += D->getName(); + Label += " {}"; + if (!PPTracker.checkForIncludesInBlock(PP, BlockRange, Label.c_str(), + errs())) + HadErrors = 1; + return true; + } + + // Collect definition entities. bool VisitNamedDecl(NamedDecl *ND) { // We only care about file-context variables. if (!ND->getDeclContext()->isFileContext()) @@ -517,14 +566,18 @@ public: private: SourceManager &SM; EntityMap &Entities; + Preprocessor &PP; + PreprocessorTracker &PPTracker; + int &HadErrors; }; class CollectEntitiesConsumer : public ASTConsumer { public: CollectEntitiesConsumer(EntityMap &Entities, PreprocessorTracker &preprocessorTracker, - Preprocessor &PP, StringRef InFile) - : Entities(Entities), PPTracker(preprocessorTracker), PP(PP) { + Preprocessor &PP, StringRef InFile, int &HadErrors) + : Entities(Entities), PPTracker(preprocessorTracker), PP(PP), + HadErrors(HadErrors) { PPTracker.handlePreprocessorEntry(PP, InFile); } @@ -534,7 +587,7 @@ public: SourceManager &SM = Ctx.getSourceManager(); // Collect declared entities. - CollectEntitiesVisitor(SM, Entities) + CollectEntitiesVisitor(SM, Entities, PP, PPTracker, HadErrors) .TraverseDecl(Ctx.getTranslationUnitDecl()); // Collect macro definitions. @@ -556,39 +609,46 @@ private: EntityMap &Entities; PreprocessorTracker &PPTracker; Preprocessor &PP; + int &HadErrors; }; class CollectEntitiesAction : public SyntaxOnlyAction { public: CollectEntitiesAction(EntityMap &Entities, - PreprocessorTracker &preprocessorTracker) - : Entities(Entities), PPTracker(preprocessorTracker) {} + PreprocessorTracker &preprocessorTracker, + int &HadErrors) + : Entities(Entities), PPTracker(preprocessorTracker), + HadErrors(HadErrors) {} protected: virtual clang::ASTConsumer *CreateASTConsumer(CompilerInstance &CI, StringRef InFile) { return new CollectEntitiesConsumer(Entities, PPTracker, - CI.getPreprocessor(), InFile); + CI.getPreprocessor(), InFile, HadErrors); } private: EntityMap &Entities; PreprocessorTracker &PPTracker; + int &HadErrors; }; class ModularizeFrontendActionFactory : public FrontendActionFactory { public: ModularizeFrontendActionFactory(EntityMap &Entities, - PreprocessorTracker &preprocessorTracker) - : Entities(Entities), PPTracker(preprocessorTracker) {} + PreprocessorTracker &preprocessorTracker, + int &HadErrors) + : Entities(Entities), PPTracker(preprocessorTracker), + HadErrors(HadErrors) {} virtual CollectEntitiesAction *create() { - return new CollectEntitiesAction(Entities, PPTracker); + return new CollectEntitiesAction(Entities, PPTracker, HadErrors); } private: EntityMap &Entities; PreprocessorTracker &PPTracker; + int &HadErrors; }; int main(int Argc, const char **Argv) { @@ -626,8 +686,9 @@ int main(int Argc, const char **Argv) { EntityMap Entities; ClangTool Tool(*Compilations, Headers); Tool.appendArgumentsAdjuster(new AddDependenciesAdjuster(Dependencies)); - int HadErrors = - Tool.run(new ModularizeFrontendActionFactory(Entities, *PPTracker)); + int HadErrors = 0; + HadErrors |= Tool.run( + new ModularizeFrontendActionFactory(Entities, *PPTracker, HadErrors)); // Create a place to save duplicate entity locations, separate bins per kind. typedef SmallVector LocationArray; diff --git a/clang-tools-extra/modularize/PreprocessorTracker.cpp b/clang-tools-extra/modularize/PreprocessorTracker.cpp index c0f20043479e..391afbe0151f 100644 --- a/clang-tools-extra/modularize/PreprocessorTracker.cpp +++ b/clang-tools-extra/modularize/PreprocessorTracker.cpp @@ -7,7 +7,7 @@ // //===--------------------------------------------------------------------===// // -// The Basic Idea +// The Basic Idea (Macro and Conditional Checking) // // Basically we install a PPCallbacks-derived object to track preprocessor // activity, namely when a header file is entered/exited, when a macro @@ -49,7 +49,17 @@ // ^ // Macro defined here. // -// Design and Implementation Details +// The Basic Idea ('Extern "C/C++" {}' Or 'namespace {}') With Nested +// '#include' Checking) +// +// To check for '#include' directives nested inside 'Extern "C/C++" {}' +// or 'namespace {}' blocks, we keep track of the '#include' directives +// while running the preprocessor, and later during a walk of the AST +// we call a function to check for any '#include' directies inside +// an 'Extern "C/C++" {}' or 'namespace {}' block, given its source +// range. +// +// Design and Implementation Details (Macro and Conditional Checking) // // A PreprocessorTrackerImpl class implements the PreprocessorTracker // interface. It uses a PreprocessorCallbacks class derived from PPCallbacks @@ -205,6 +215,22 @@ // to make clearer the separate reporting phases, I could add an output // message marking the phases. // +// Design and Implementation Details ('Extern "C/C++" {}' Or +// 'namespace {}') With Nested '#include' Checking) +// +// We override the InclusionDirective in PPCallbacks to record information +// about each '#include' directive encountered during preprocessing. +// We co-opt the PPItemKey class to store the information about each +// '#include' directive, including the source file name containing the +// directive, the name of the file being included, and the source line +// and column of the directive. We store these object in a vector, +// after first check to see if an entry already exists. +// +// Later, while the AST is being walked for other checks, we provide +// visit handlers for 'extern "C/C++" {}' and 'namespace (name) {}' +// blocks, checking to see if any '#include' directives occurred +// within the blocks, reporting errors if any found. +// // Future Directions // // We probably should add options to disable any of the checks, in case @@ -285,7 +311,7 @@ std::string getSourceString(clang::Preprocessor &PP, clang::SourceRange Range) { return llvm::StringRef(BeginPtr, Length).trim().str(); } -// Retrieve source line from file image. +// Retrieve source line from file image given a location. std::string getSourceLine(clang::Preprocessor &PP, clang::SourceLocation Loc) { const llvm::MemoryBuffer *MemBuffer = PP.getSourceManager().getBuffer(PP.getSourceManager().getFileID(Loc)); @@ -310,6 +336,39 @@ std::string getSourceLine(clang::Preprocessor &PP, clang::SourceLocation Loc) { return llvm::StringRef(BeginPtr, Length).str(); } +// Retrieve source line from file image given a file ID and line number. +std::string getSourceLine(clang::Preprocessor &PP, clang::FileID FileID, + int Line) { + const llvm::MemoryBuffer *MemBuffer = PP.getSourceManager().getBuffer(FileID); + const char *Buffer = MemBuffer->getBufferStart(); + const char *BufferEnd = MemBuffer->getBufferEnd(); + const char *BeginPtr = Buffer; + const char *EndPtr = BufferEnd; + int LineCounter = 1; + if (Line == 1) + BeginPtr = Buffer; + else { + while (Buffer < BufferEnd) { + if (*Buffer == '\n') { + if (++LineCounter == Line) { + BeginPtr = Buffer++ + 1; + break; + } + } + Buffer++; + } + } + while (Buffer < BufferEnd) { + if (*Buffer == '\n') { + EndPtr = Buffer; + break; + } + Buffer++; + } + size_t Length = EndPtr - BeginPtr; + return llvm::StringRef(BeginPtr, Length).str(); +} + // Get the string for the Unexpanded macro instance. // The soureRange is expected to end at the last token // for the macro instance, which in the case of a function-style @@ -765,6 +824,14 @@ public: ~PreprocessorCallbacks() {} // Overridden handlers. + void InclusionDirective(clang::SourceLocation HashLoc, + const clang::Token &IncludeTok, + llvm::StringRef FileName, bool IsAngled, + clang::CharSourceRange FilenameRange, + const clang::FileEntry *File, + llvm::StringRef SearchPath, + llvm::StringRef RelativePath, + const clang::Module *Imported); void FileChanged(clang::SourceLocation Loc, clang::PPCallbacks::FileChangeReason Reason, clang::SrcMgr::CharacteristicKind FileType, @@ -821,6 +888,70 @@ public: // Handle exiting a preprocessing session. void handlePreprocessorExit() { HeaderStack.clear(); } + // Handle include directive. + // This function is called every time an include directive is seen by the + // preprocessor, for the purpose of later checking for 'extern "" {}' or + // "namespace {}" blocks containing #include directives. + void handleIncludeDirective(llvm::StringRef DirectivePath, int DirectiveLine, + int DirectiveColumn, llvm::StringRef TargetPath) { + HeaderHandle CurrentHeaderHandle = findHeaderHandle(DirectivePath); + StringHandle IncludeHeaderHandle = addString(TargetPath); + for (std::vector::const_iterator I = IncludeDirectives.begin(), + E = IncludeDirectives.end(); + I != E; ++I) { + // If we already have an entry for this directive, return now. + if ((I->File == CurrentHeaderHandle) && (I->Line == DirectiveLine)) + return; + } + PPItemKey IncludeDirectiveItem(IncludeHeaderHandle, CurrentHeaderHandle, + DirectiveLine, DirectiveColumn); + IncludeDirectives.push_back(IncludeDirectiveItem); + } + + // Check for include directives within the given source line range. + // Report errors if any found. Returns true if no include directives + // found in block. + bool checkForIncludesInBlock(clang::Preprocessor &PP, + clang::SourceRange BlockSourceRange, + const char *BlockIdentifierMessage, + llvm::raw_ostream &OS) { + clang::SourceLocation BlockStartLoc = BlockSourceRange.getBegin(); + clang::SourceLocation BlockEndLoc = BlockSourceRange.getEnd(); + // Use block location to get FileID of both the include directive + // and block statement. + clang::FileID FileID = PP.getSourceManager().getFileID(BlockStartLoc); + std::string SourcePath = getSourceLocationFile(PP, BlockStartLoc); + HeaderHandle SourceHandle = findHeaderHandle(SourcePath); + int BlockStartLine, BlockStartColumn, BlockEndLine, BlockEndColumn; + bool returnValue = true; + getSourceLocationLineAndColumn(PP, BlockStartLoc, BlockStartLine, + BlockStartColumn); + getSourceLocationLineAndColumn(PP, BlockEndLoc, BlockEndLine, + BlockEndColumn); + for (std::vector::const_iterator I = IncludeDirectives.begin(), + E = IncludeDirectives.end(); + I != E; ++I) { + // If we find an entry within the block, report an error. + if ((I->File == SourceHandle) && (I->Line >= BlockStartLine) && + (I->Line < BlockEndLine)) { + returnValue = false; + OS << SourcePath << ":" << I->Line << ":" << I->Column << "\n"; + OS << getSourceLine(PP, FileID, I->Line) << "\n"; + if (I->Column > 0) + OS << std::string(I->Column - 1, ' ') << "^\n"; + OS << "error: Include directive within " << BlockIdentifierMessage + << ".\n"; + OS << SourcePath << ":" << BlockStartLine << ":" << BlockStartColumn + << "\n"; + OS << getSourceLine(PP, BlockStartLoc) << "\n"; + if (BlockStartColumn > 0) + OS << std::string(BlockStartColumn - 1, ' ') << "^\n"; + OS << "The \"" << BlockIdentifierMessage << "\" block is here.\n"; + } + } + return returnValue; + } + // Handle entering a header source file. void handleHeaderEntry(clang::Preprocessor &PP, llvm::StringRef HeaderPath) { // Ignore and to reduce message clutter. @@ -1176,6 +1307,7 @@ private: std::vector InclusionPaths; InclusionPathHandle CurrentInclusionPathHandle; llvm::SmallSet HeadersInThisCompile; + std::vector IncludeDirectives; MacroExpansionMap MacroExpansions; ConditionalExpansionMap ConditionalExpansions; bool InNestedHeader; @@ -1193,6 +1325,20 @@ PreprocessorTracker *PreprocessorTracker::create() { // Preprocessor callbacks for modularize. +// Handle include directive. +void PreprocessorCallbacks::InclusionDirective( + clang::SourceLocation HashLoc, const clang::Token &IncludeTok, + llvm::StringRef FileName, bool IsAngled, + clang::CharSourceRange FilenameRange, const clang::FileEntry *File, + llvm::StringRef SearchPath, llvm::StringRef RelativePath, + const clang::Module *Imported) { + int DirectiveLine, DirectiveColumn; + std::string HeaderPath = getSourceLocationFile(PP, HashLoc); + getSourceLocationLineAndColumn(PP, HashLoc, DirectiveLine, DirectiveColumn); + PPTracker.handleIncludeDirective(HeaderPath, DirectiveLine, DirectiveColumn, + FileName); +} + // Handle file entry/exit. void PreprocessorCallbacks::FileChanged( clang::SourceLocation Loc, clang::PPCallbacks::FileChangeReason Reason, diff --git a/clang-tools-extra/modularize/PreprocessorTracker.h b/clang-tools-extra/modularize/PreprocessorTracker.h index 0db9cf83d531..4f72d0146317 100644 --- a/clang-tools-extra/modularize/PreprocessorTracker.h +++ b/clang-tools-extra/modularize/PreprocessorTracker.h @@ -52,6 +52,22 @@ public: // object is destroyed.) virtual void handlePreprocessorExit() = 0; + // Handle include directive. + // This function is called every time an include directive is seen by the + // preprocessor, for the purpose of later checking for 'extern "" {}' or + // "namespace {}" blocks containing #include directives. + virtual void handleIncludeDirective(llvm::StringRef DirectivePath, + int DirectiveLine, int DirectiveColumn, + llvm::StringRef TargetPath) = 0; + + // Check for include directives within the given source line range. + // Report errors if any found. Returns true if no include directives + // found in block. + virtual bool checkForIncludesInBlock(clang::Preprocessor &PP, + clang::SourceRange BlockSourceRange, + const char *BlockIdentifierMessage, + llvm::raw_ostream &OS) = 0; + // Report on inconsistent macro instances. // Returns true if any mismatches. virtual bool reportInconsistentMacros(llvm::raw_ostream &OS) = 0; diff --git a/clang-tools-extra/test/modularize/Inputs/Empty.h b/clang-tools-extra/test/modularize/Inputs/Empty.h new file mode 100644 index 000000000000..d74d27d937fe --- /dev/null +++ b/clang-tools-extra/test/modularize/Inputs/Empty.h @@ -0,0 +1 @@ +// Empty header for testing #include directives in blocks. diff --git a/clang-tools-extra/test/modularize/Inputs/IncludeInExtern.h b/clang-tools-extra/test/modularize/Inputs/IncludeInExtern.h new file mode 100644 index 000000000000..da7de8faf2a5 --- /dev/null +++ b/clang-tools-extra/test/modularize/Inputs/IncludeInExtern.h @@ -0,0 +1,3 @@ +extern "C" { + #include "Empty.h" +} diff --git a/clang-tools-extra/test/modularize/Inputs/IncludeInNamespace.h b/clang-tools-extra/test/modularize/Inputs/IncludeInNamespace.h new file mode 100644 index 000000000000..212252882a12 --- /dev/null +++ b/clang-tools-extra/test/modularize/Inputs/IncludeInNamespace.h @@ -0,0 +1,3 @@ +namespace MyNamespace { + #include "Empty.h" +} diff --git a/clang-tools-extra/test/modularize/ProblemsExternC.modularize b/clang-tools-extra/test/modularize/ProblemsExternC.modularize new file mode 100644 index 000000000000..e16dcfdb62d5 --- /dev/null +++ b/clang-tools-extra/test/modularize/ProblemsExternC.modularize @@ -0,0 +1,12 @@ +# RUN: not modularize %s -x c++ 2>&1 | FileCheck %s + +Inputs/IncludeInExtern.h + +# CHECK: {{.*}}{{[/\\]}}Inputs{{[/\\]}}IncludeInExtern.h:2:3 +# CHECK-NEXT: #include "Empty.h" +# CHECK-NEXT: ^ +# CHECK-NEXT: error: Include directive within extern "C" {}. +# CHECK-NEXT: {{.*}}{{[/\\]}}Inputs{{[/\\]}}IncludeInExtern.h:1:1 +# CHECK-NEXT: extern "C" { +# CHECK-NEXT: ^ +# CHECK-NEXT: The "extern "C" {}" block is here. diff --git a/clang-tools-extra/test/modularize/ProblemsNamespace.modularize b/clang-tools-extra/test/modularize/ProblemsNamespace.modularize new file mode 100644 index 000000000000..193402b966c4 --- /dev/null +++ b/clang-tools-extra/test/modularize/ProblemsNamespace.modularize @@ -0,0 +1,12 @@ +# RUN: not modularize %s -x c++ 2>&1 | FileCheck %s + +Inputs/IncludeInNamespace.h + +# CHECK: {{.*}}{{[/\\]}}Inputs{{[/\\]}}IncludeInNamespace.h:2:3 +# CHECK-NEXT: #include "Empty.h" +# CHECK-NEXT: ^ +# CHECK-NEXT: error: Include directive within namespace MyNamespace {}. +# CHECK-NEXT: {{.*}}{{[/\\]}}Inputs{{[/\\]}}IncludeInNamespace.h:1:1 +# CHECK-NEXT: namespace MyNamespace { +# CHECK-NEXT: ^ +# CHECK-NEXT: The "namespace MyNamespace {}" block is here.