r213348 - [modules] Fix macro hiding bug exposed if:
Ben Langmuir
blangmuir at apple.com
Fri Jul 18 17:40:40 PDT 2014
> On Jul 18, 2014, at 3:22 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> On Fri, Jul 18, 2014 at 11:48 AM, Ben Langmuir <blangmuir at apple.com> wrote:
> Thanks! Reverted in r213395. For your convenience, I’ve attached my test case as a patch on top of your test cases.
>
> Thanks for the revert and the testcase; re-committed with a fix in r213416. Please let me know if this is still a problem; I don't have a Darwin system to test with.
Looks good on Darwin. Thanks, Richard!
>
> Ben
>
>
>
>> On Jul 18, 2014, at 11:39 AM, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>> 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/b213f93d/attachment.html>
More information about the cfe-commits
mailing list