r213348 - [modules] Fix macro hiding bug exposed if:
Ben Langmuir
blangmuir at apple.com
Fri Jul 18 11:22:31 PDT 2014
Hey Richard,
After this commit, the system modules on Darwin don’t build.
Using your test case as a starting point, if you add a header conddef.h:
#include “a2.h”
#ifndef assert
#define assert(X)
#endif
which has an obvious module:
module conddef { header “conddef.h” export * }
And then include that and try to use assert(), it will not be there. It looks like assert is incorrectly visible in conddef.h, but now we’re not re-exporting it. So it might not really be this change that’s the problem, but maybe it’s interacting really poorly with another bug?
This seems really bad to me, because this pattern of ifndef define is really common. Do you know what the fix is, or can we revert for the time being?
Ben
> On Jul 17, 2014, at 9:53 PM, Richard Smith <richard-llvm at metafoo.co.uk> wrote:
>
> Author: rsmith
> Date: Thu Jul 17 23:53:37 2014
> New Revision: 213348
>
> URL: http://llvm.org/viewvc/llvm-project?rev=213348&view=rev
> Log:
> [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. =(
>
> Added:
> cfe/trunk/test/Modules/Inputs/macro-hiding/
> cfe/trunk/test/Modules/Inputs/macro-hiding/a1.h
> cfe/trunk/test/Modules/Inputs/macro-hiding/a2.h
> cfe/trunk/test/Modules/Inputs/macro-hiding/b1.h
> cfe/trunk/test/Modules/Inputs/macro-hiding/b2.h
> cfe/trunk/test/Modules/Inputs/macro-hiding/c1.h
> cfe/trunk/test/Modules/Inputs/macro-hiding/module.modulemap
> cfe/trunk/test/Modules/macro-hiding.cpp
> Modified:
> cfe/trunk/include/clang/Serialization/ASTReader.h
> cfe/trunk/lib/Serialization/ASTReader.cpp
> cfe/trunk/lib/Serialization/ASTWriter.cpp
>
> Modified: cfe/trunk/include/clang/Serialization/ASTReader.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTReader.h?rev=213348&r1=213347&r2=213348&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Serialization/ASTReader.h (original)
> +++ cfe/trunk/include/clang/Serialization/ASTReader.h Thu Jul 17 23:53:37 2014
> @@ -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<DefMacroDirective *> AmbiguousMacros;
> llvm::DenseMap<IdentifierInfo*, AmbiguousMacros> AmbiguousMacroDefs;
>
> Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=213348&r1=213347&r2=213348&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
> +++ cfe/trunk/lib/Serialization/ASTReader.cpp Thu Jul 17 23:53:37 2014
> @@ -1790,7 +1790,7 @@ void ASTReader::resolvePendingMacro(Iden
> // 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(Identi
> }
>
> 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
> }
> }
>
> -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 H
> }
> }
>
> + 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
> // 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();
> }
>
> Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=213348&r1=213347&r2=213348&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
> +++ cfe/trunk/lib/Serialization/ASTWriter.cpp Thu Jul 17 23:53:37 2014
> @@ -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<DefMacroDirective>(MD))
> + return DefMD->getInfo()->getOwningModuleID();
> +
> + // Skip imports that only produce #undefs for now.
> + // FIXME: We should still re-export them!
> + return 0;
> }
>
> public:
>
> Added: cfe/trunk/test/Modules/Inputs/macro-hiding/a1.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/a1.h?rev=213348&view=auto
> ==============================================================================
> --- cfe/trunk/test/Modules/Inputs/macro-hiding/a1.h (added)
> +++ cfe/trunk/test/Modules/Inputs/macro-hiding/a1.h Thu Jul 17 23:53:37 2014
> @@ -0,0 +1 @@
> +#define assert(x)
>
> Added: cfe/trunk/test/Modules/Inputs/macro-hiding/a2.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/a2.h?rev=213348&view=auto
> ==============================================================================
> (empty)
>
> Added: cfe/trunk/test/Modules/Inputs/macro-hiding/b1.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/b1.h?rev=213348&view=auto
> ==============================================================================
> (empty)
>
> Added: cfe/trunk/test/Modules/Inputs/macro-hiding/b2.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/b2.h?rev=213348&view=auto
> ==============================================================================
> --- cfe/trunk/test/Modules/Inputs/macro-hiding/b2.h (added)
> +++ cfe/trunk/test/Modules/Inputs/macro-hiding/b2.h Thu Jul 17 23:53:37 2014
> @@ -0,0 +1,2 @@
> +#include "a2.h"
> +#define assert(x)
>
> Added: cfe/trunk/test/Modules/Inputs/macro-hiding/c1.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/c1.h?rev=213348&view=auto
> ==============================================================================
> --- cfe/trunk/test/Modules/Inputs/macro-hiding/c1.h (added)
> +++ cfe/trunk/test/Modules/Inputs/macro-hiding/c1.h Thu Jul 17 23:53:37 2014
> @@ -0,0 +1,2 @@
> +#include "b1.h"
> +#define assert(x)
>
> Added: cfe/trunk/test/Modules/Inputs/macro-hiding/module.modulemap
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/module.modulemap?rev=213348&view=auto
> ==============================================================================
> --- cfe/trunk/test/Modules/Inputs/macro-hiding/module.modulemap (added)
> +++ cfe/trunk/test/Modules/Inputs/macro-hiding/module.modulemap Thu Jul 17 23:53:37 2014
> @@ -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 * }
> +}
>
> Added: cfe/trunk/test/Modules/macro-hiding.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/macro-hiding.cpp?rev=213348&view=auto
> ==============================================================================
> --- cfe/trunk/test/Modules/macro-hiding.cpp (added)
> +++ cfe/trunk/test/Modules/macro-hiding.cpp Thu Jul 17 23:53:37 2014
> @@ -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); }
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list