From 3965412f088ccdab52b897f89f29b937269e26fd Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Fri, 18 Jul 2014 04:53:37 +0000 Subject: [PATCH] [modules] Fix macro hiding bug exposed if: * A submodule of module A is imported into module B * Another submodule of module A that is not imported into B exports a macro * Some submodule of module B also exports a definition of the macro, and happens to be the first submodule of B that imports module A. In this case, we would incorrectly determine that A's macro redefines B's macro, and so we don't need to re-export B's macro at all. This happens with the 'assert' macro in an LLVM self-host. =( llvm-svn: 213348 --- clang/include/clang/Serialization/ASTReader.h | 5 +++-- clang/lib/Serialization/ASTReader.cpp | 19 ++++++++++++------- clang/lib/Serialization/ASTWriter.cpp | 12 +++++++++++- clang/test/Modules/Inputs/macro-hiding/a1.h | 1 + clang/test/Modules/Inputs/macro-hiding/a2.h | 0 clang/test/Modules/Inputs/macro-hiding/b1.h | 0 clang/test/Modules/Inputs/macro-hiding/b2.h | 2 ++ clang/test/Modules/Inputs/macro-hiding/c1.h | 2 ++ .../test/Modules/Inputs/macro-hiding/module.modulemap | 11 +++++++++++ clang/test/Modules/macro-hiding.cpp | 6 ++++++ 10 files changed, 48 insertions(+), 10 deletions(-) create mode 100644 clang/test/Modules/Inputs/macro-hiding/a1.h create mode 100644 clang/test/Modules/Inputs/macro-hiding/a2.h create mode 100644 clang/test/Modules/Inputs/macro-hiding/b1.h create mode 100644 clang/test/Modules/Inputs/macro-hiding/b2.h create mode 100644 clang/test/Modules/Inputs/macro-hiding/c1.h create mode 100644 clang/test/Modules/Inputs/macro-hiding/module.modulemap create mode 100644 clang/test/Modules/macro-hiding.cpp diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index 0698b98..3f44440 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -1366,7 +1366,8 @@ public: bool Complain); /// \brief Make the names within this set of hidden names visible. - void makeNamesVisible(const HiddenNames &Names, Module *Owner); + void makeNamesVisible(const HiddenNames &Names, Module *Owner, + bool FromFinalization); /// \brief Set the AST callbacks listener. void setListener(ASTReaderListener *listener) { @@ -1831,7 +1832,7 @@ public: ModuleFile &M, uint64_t Offset); void installImportedMacro(IdentifierInfo *II, ModuleMacroInfo *MMI, - Module *Owner); + Module *Owner, bool FromFinalization); typedef llvm::TinyPtrVector AmbiguousMacros; llvm::DenseMap AmbiguousMacroDefs; diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index f18122d..419f6b3 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -1790,7 +1790,7 @@ void ASTReader::resolvePendingMacro(IdentifierInfo *II, // install if we make this module visible. HiddenNamesMap[Owner].HiddenMacros.insert(std::make_pair(II, MMI)); } else { - installImportedMacro(II, MMI, Owner); + installImportedMacro(II, MMI, Owner, /*FromFinalization*/false); } } @@ -1941,11 +1941,11 @@ ASTReader::removeOverriddenMacros(IdentifierInfo *II, } void ASTReader::installImportedMacro(IdentifierInfo *II, ModuleMacroInfo *MMI, - Module *Owner) { + Module *Owner, bool FromFinalization) { assert(II && Owner); SourceLocation ImportLoc = Owner->MacroVisibilityLoc; - if (ImportLoc.isInvalid()) { + if (ImportLoc.isInvalid() && !FromFinalization) { // FIXME: If we made macros from this module visible but didn't provide a // source location for the import, we don't have a location for the macro. // Use the location at which the containing module file was first imported @@ -3320,7 +3320,9 @@ static void moveMethodToBackOfGlobalList(Sema &S, ObjCMethodDecl *Method) { } } -void ASTReader::makeNamesVisible(const HiddenNames &Names, Module *Owner) { +void ASTReader::makeNamesVisible(const HiddenNames &Names, Module *Owner, + bool FromFinalization) { + // FIXME: Only do this if Owner->NameVisibility == AllVisible. for (unsigned I = 0, N = Names.HiddenDecls.size(); I != N; ++I) { Decl *D = Names.HiddenDecls[I]; bool wasHidden = D->Hidden; @@ -3333,10 +3335,12 @@ void ASTReader::makeNamesVisible(const HiddenNames &Names, Module *Owner) { } } + assert((FromFinalization || Owner->NameVisibility >= Module::MacrosVisible) && + "nothing to make visible?"); for (HiddenMacrosMap::const_iterator I = Names.HiddenMacros.begin(), E = Names.HiddenMacros.end(); I != E; ++I) - installImportedMacro(I->first, I->second, Owner); + installImportedMacro(I->first, I->second, Owner, FromFinalization); } void ASTReader::makeModuleVisible(Module *Mod, @@ -3370,7 +3374,8 @@ void ASTReader::makeModuleVisible(Module *Mod, // mark them as visible. HiddenNamesMapType::iterator Hidden = HiddenNamesMap.find(Mod); if (Hidden != HiddenNamesMap.end()) { - makeNamesVisible(Hidden->second, Hidden->first); + makeNamesVisible(Hidden->second, Hidden->first, + /*FromFinalization*/false); HiddenNamesMap.erase(Hidden); } @@ -3897,7 +3902,7 @@ void ASTReader::finalizeForWriting() { for (HiddenNamesMapType::iterator Hidden = HiddenNamesMap.begin(), HiddenEnd = HiddenNamesMap.end(); Hidden != HiddenEnd; ++Hidden) { - makeNamesVisible(Hidden->second, Hidden->first); + makeNamesVisible(Hidden->second, Hidden->first, /*FromFinalization*/true); } HiddenNamesMap.clear(); } diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 97a5d0e..f0b1752 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -3089,7 +3089,17 @@ class ASTIdentifierTableTrait { } SubmoduleID getSubmoduleID(MacroDirective *MD) { - return Writer.inferSubmoduleIDFromLocation(MD->getLocation()); + if (MD->getLocation().isValid()) + return Writer.inferSubmoduleIDFromLocation(MD->getLocation()); + + // If we have no directive location, this macro was installed when + // finalizing the ASTReader. + if (DefMacroDirective *DefMD = dyn_cast(MD)) + return DefMD->getInfo()->getOwningModuleID(); + + // Skip imports that only produce #undefs for now. + // FIXME: We should still re-export them! + return 0; } public: diff --git a/clang/test/Modules/Inputs/macro-hiding/a1.h b/clang/test/Modules/Inputs/macro-hiding/a1.h new file mode 100644 index 0000000..b17c8ee --- /dev/null +++ b/clang/test/Modules/Inputs/macro-hiding/a1.h @@ -0,0 +1 @@ +#define assert(x) diff --git a/clang/test/Modules/Inputs/macro-hiding/a2.h b/clang/test/Modules/Inputs/macro-hiding/a2.h new file mode 100644 index 0000000..e69de29 diff --git a/clang/test/Modules/Inputs/macro-hiding/b1.h b/clang/test/Modules/Inputs/macro-hiding/b1.h new file mode 100644 index 0000000..e69de29 diff --git a/clang/test/Modules/Inputs/macro-hiding/b2.h b/clang/test/Modules/Inputs/macro-hiding/b2.h new file mode 100644 index 0000000..8391848 --- /dev/null +++ b/clang/test/Modules/Inputs/macro-hiding/b2.h @@ -0,0 +1,2 @@ +#include "a2.h" +#define assert(x) diff --git a/clang/test/Modules/Inputs/macro-hiding/c1.h b/clang/test/Modules/Inputs/macro-hiding/c1.h new file mode 100644 index 0000000..4b78b3c --- /dev/null +++ b/clang/test/Modules/Inputs/macro-hiding/c1.h @@ -0,0 +1,2 @@ +#include "b1.h" +#define assert(x) diff --git a/clang/test/Modules/Inputs/macro-hiding/module.modulemap b/clang/test/Modules/Inputs/macro-hiding/module.modulemap new file mode 100644 index 0000000..2657ab1 --- /dev/null +++ b/clang/test/Modules/Inputs/macro-hiding/module.modulemap @@ -0,0 +1,11 @@ +module a { + module a1 { header "a1.h" export * } + module a2 { header "a2.h" export * } +} +module b { + module b1 { header "b1.h" export * } + module b2 { header "b2.h" export * } +} +module c { + module c1 { header "c1.h" export * } +} diff --git a/clang/test/Modules/macro-hiding.cpp b/clang/test/Modules/macro-hiding.cpp new file mode 100644 index 0000000..a7a6064 --- /dev/null +++ b/clang/test/Modules/macro-hiding.cpp @@ -0,0 +1,6 @@ +// RUN: rm -rf %t +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s +#include "c1.h" +#include "b2.h" + +void h() { assert(true); } -- 2.7.4