Hi doug.gregor, silvas,
It's a potential problem for modules (and headers in general) if '#include'
directives occur inside 'extern "C/C++" {}' or 'namespace (name) {}' blocks,
because that can produce different underlying definitions if other sources
include the header outside of such blocks. This patch to modularize checks for
these cases, treating them as errors, and produces error message referencing
both the offending '#include' and the enclosing 'extern "C/C++" {}' or
'namespace (name) {}' block.
http://llvm-reviews.chandlerc.com/D1648
Files:
test/modularize/Inputs/Empty.h
test/modularize/Inputs/IncludeInExtern.h
test/modularize/Inputs/IncludeInNamespace.h
test/modularize/ProblemsExternC.modularize
test/modularize/ProblemsNamespace.modularize
modularize/PreprocessorTracker.cpp
modularize/PreprocessorTracker.h
modularize/Modularize.cpp
Index: test/modularize/Inputs/Empty.h
===================================================================
--- test/modularize/Inputs/Empty.h
+++ test/modularize/Inputs/Empty.h
@@ -0,0 +1 @@
+// Empty header for testing #include directives in blocks.
Index: test/modularize/Inputs/IncludeInExtern.h
===================================================================
--- test/modularize/Inputs/IncludeInExtern.h
+++ test/modularize/Inputs/IncludeInExtern.h
@@ -0,0 +1,3 @@
+extern "C" {
+ #include "Empty.h"
+}
Index: test/modularize/Inputs/IncludeInNamespace.h
===================================================================
--- test/modularize/Inputs/IncludeInNamespace.h
+++ test/modularize/Inputs/IncludeInNamespace.h
@@ -0,0 +1,3 @@
+namespace MyNamespace {
+ #include "Empty.h"
+}
Index: test/modularize/ProblemsExternC.modularize
===================================================================
--- test/modularize/ProblemsExternC.modularize
+++ 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.
Index: test/modularize/ProblemsNamespace.modularize
===================================================================
--- test/modularize/ProblemsNamespace.modularize
+++ 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.
Index: modularize/PreprocessorTracker.cpp
===================================================================
--- modularize/PreprocessorTracker.cpp
+++ 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 @@
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 @@
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 @@
~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 @@
// 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<PPItemKey>::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<PPItemKey>::const_iterator I = IncludeDirectives.begin(),
+ E = IncludeDirectives.end();
+ I != E; ++I) {
+ // If we find an entry with 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 <built-in> and <command-line> to reduce message clutter.
@@ -1176,6 +1307,7 @@
std::vector<HeaderInclusionPath> InclusionPaths;
InclusionPathHandle CurrentInclusionPathHandle;
llvm::SmallSet<HeaderHandle, 128> HeadersInThisCompile;
+ std::vector<PPItemKey> IncludeDirectives;
MacroExpansionMap MacroExpansions;
ConditionalExpansionMap ConditionalExpansions;
bool InNestedHeader;
@@ -1193,6 +1325,20 @@
// 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,
Index: modularize/PreprocessorTracker.h
===================================================================
--- modularize/PreprocessorTracker.h
+++ modularize/PreprocessorTracker.h
@@ -52,6 +52,22 @@
// 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;
Index: modularize/Modularize.cpp
===================================================================
--- modularize/Modularize.cpp
+++ 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:
@@ -453,8 +466,11 @@
class CollectEntitiesVisitor
: public RecursiveASTVisitor<CollectEntitiesVisitor> {
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 +494,42 @@
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 +569,18 @@
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 +590,7 @@
SourceManager &SM = Ctx.getSourceManager();
// Collect declared entities.
- CollectEntitiesVisitor(SM, Entities)
+ CollectEntitiesVisitor(SM, Entities, PP, PPTracker, HadErrors)
.TraverseDecl(Ctx.getTranslationUnitDecl());
// Collect macro definitions.
@@ -556,39 +612,46 @@
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 +689,9 @@
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<Location, 8> LocationArray;
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
