r213348 - [modules] Fix macro hiding bug exposed if:

Richard Smith richard at metafoo.co.uk
Fri Jul 18 11:39:13 PDT 2014


Please revert if this is blocking you; I'll dig into the root cause today.
On 18 Jul 2014 11:22, "Ben Langmuir" <blangmuir at apple.com> wrote:

> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140718/6a01a91e/attachment.html>


More information about the cfe-commits mailing list