r284382 - Revert "Reinstate r281429, reverted in r281452, with a fix for its mishandling of"
Benjamin Kramer via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 17 08:07:58 PDT 2016
I'm running into something else and your patch doesn't fix it. It
boils down to 'std::is_nothrow_move_constructible<Foo>' from libstdc++
not compiling because Foo is not visible inside the noexcept
specification of that template. I failed to come up with a small test
case so far as it involves at least 2 modules :[
On Mon, Oct 17, 2016 at 4:44 PM, Vassil Vassilev <v.g.vassilev at gmail.com> wrote:
> On 17/10/16 16:40, Benjamin Kramer wrote:
>>
>> Too slow ;)
>>
>> Do you have the fix somewhere, so I can try it?
>
> That would be https://reviews.llvm.org/D25678
>
> Do you run into something else than what I have as a test case (merging
> templated constexpr variables)?
>
>>
>> On Mon, Oct 17, 2016 at 4:38 PM, Vassil Vassilev <v.g.vassilev at gmail.com>
>> wrote:
>>>
>>> I was just to commit a fix :(
>>>
>>> On 17/10/16 15:00, Benjamin Kramer via cfe-commits wrote:
>>>>
>>>> Author: d0k
>>>> Date: Mon Oct 17 08:00:44 2016
>>>> New Revision: 284382
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=284382&view=rev
>>>> Log:
>>>> Revert "Reinstate r281429, reverted in r281452, with a fix for its
>>>> mishandling of"
>>>>
>>>> This reverts commit r284176. It still marks some modules as invisible
>>>> that should be visible. Will follow up with the author with a test case.
>>>>
>>>> Removed:
>>>> cfe/trunk/test/Modules/Inputs/merge-var-template-def/a.h
>>>> cfe/trunk/test/Modules/Inputs/merge-var-template-def/b1.h
>>>> cfe/trunk/test/Modules/Inputs/merge-var-template-def/b2.h
>>>>
>>>> cfe/trunk/test/Modules/Inputs/merge-var-template-def/module.modulemap
>>>> cfe/trunk/test/Modules/merge-var-template-def.cpp
>>>> Modified:
>>>> cfe/trunk/include/clang/AST/ASTContext.h
>>>> cfe/trunk/lib/AST/ASTContext.cpp
>>>> cfe/trunk/lib/Serialization/ASTReader.cpp
>>>>
>>>> cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/a.h
>>>>
>>>> cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/b.h
>>>>
>>>> cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/c.h
>>>>
>>>> cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/d.h
>>>> cfe/trunk/test/Modules/merge-template-pattern-visibility.cpp
>>>>
>>>> Modified: cfe/trunk/include/clang/AST/ASTContext.h
>>>> URL:
>>>>
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=284382&r1=284381&r2=284382&view=diff
>>>>
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/include/clang/AST/ASTContext.h (original)
>>>> +++ cfe/trunk/include/clang/AST/ASTContext.h Mon Oct 17 08:00:44 2016
>>>> @@ -308,18 +308,10 @@ class ASTContext : public RefCountedBase
>>>> /// merged into.
>>>> llvm::DenseMap<Decl*, Decl*> MergedDecls;
>>>> - /// The modules into which a definition has been merged, or a map
>>>> from a
>>>> - /// merged definition to its canonical definition. This is really a
>>>> union of
>>>> - /// a NamedDecl* and a vector of Module*.
>>>> - struct MergedModulesOrCanonicalDef {
>>>> - llvm::TinyPtrVector<Module*> MergedModules;
>>>> - NamedDecl *CanonicalDef = nullptr;
>>>> - };
>>>> -
>>>> /// \brief A mapping from a defining declaration to a list of
>>>> modules
>>>> (other
>>>> /// than the owning module of the declaration) that contain merged
>>>> /// definitions of that entity.
>>>> - llvm::DenseMap<NamedDecl*, MergedModulesOrCanonicalDef>
>>>> MergedDefModules;
>>>> + llvm::DenseMap<NamedDecl*, llvm::TinyPtrVector<Module*>>
>>>> MergedDefModules;
>>>> /// \brief Initializers for a module, in order. Each Decl will be
>>>> either
>>>> /// something that has a semantic effect on startup (such as a
>>>> variable with
>>>> @@ -891,7 +883,6 @@ public:
>>>> /// and should be visible whenever \p M is visible.
>>>> void mergeDefinitionIntoModule(NamedDecl *ND, Module *M,
>>>> bool NotifyListeners = true);
>>>> - void mergeDefinitionIntoModulesOf(NamedDecl *ND, NamedDecl *Other);
>>>> /// \brief Clean up the merged definition list. Call this if you
>>>> might
>>>> have
>>>> /// added duplicates into the list.
>>>> void deduplicateMergedDefinitonsFor(NamedDecl *ND);
>>>> @@ -902,9 +893,7 @@ public:
>>>> auto MergedIt = MergedDefModules.find(Def);
>>>> if (MergedIt == MergedDefModules.end())
>>>> return None;
>>>> - if (auto *CanonDef = MergedIt->second.CanonicalDef)
>>>> - return getModulesWithMergedDefinition(CanonDef);
>>>> - return MergedIt->second.MergedModules;
>>>> + return MergedIt->second;
>>>> }
>>>> /// Add a declaration to the list of declarations that are
>>>> initialized
>>>>
>>>> Modified: cfe/trunk/lib/AST/ASTContext.cpp
>>>> URL:
>>>>
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=284382&r1=284381&r2=284382&view=diff
>>>>
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/lib/AST/ASTContext.cpp (original)
>>>> +++ cfe/trunk/lib/AST/ASTContext.cpp Mon Oct 17 08:00:44 2016
>>>> @@ -888,74 +888,18 @@ void ASTContext::mergeDefinitionIntoModu
>>>> if (auto *Listener = getASTMutationListener())
>>>> Listener->RedefinedHiddenDefinition(ND, M);
>>>> - auto *Merged = &MergedDefModules[ND];
>>>> - if (auto *CanonDef = Merged->CanonicalDef) {
>>>> - ND = CanonDef;
>>>> - Merged = &MergedDefModules[ND];
>>>> - }
>>>> - assert(!Merged->CanonicalDef && "canonical def not canonical");
>>>> -
>>>> - Merged->MergedModules.push_back(M);
>>>> -
>>>> - if (!getLangOpts().ModulesLocalVisibility)
>>>> + if (getLangOpts().ModulesLocalVisibility)
>>>> + MergedDefModules[ND].push_back(M);
>>>> + else
>>>> ND->setHidden(false);
>>>> }
>>>> -void ASTContext::mergeDefinitionIntoModulesOf(NamedDecl *Def,
>>>> - NamedDecl *Other) {
>>>> - // We need to know the owning module of the merge source.
>>>> - assert(Other->isFromASTFile() && "merge of non-imported decl not
>>>> supported");
>>>> - assert(Def != Other && "merging definition into itself");
>>>> -
>>>> - if (!Other->isHidden()) {
>>>> - Def->setHidden(false);
>>>> - return;
>>>> - }
>>>> -
>>>> - assert(Other->getImportedOwningModule() &&
>>>> - "hidden, imported declaration has no owning module");
>>>> -
>>>> - // Mark Def as the canonical definition of merged definition Other.
>>>> - {
>>>> - auto &OtherMerged = MergedDefModules[Other];
>>>> - assert((!OtherMerged.CanonicalDef || OtherMerged.CanonicalDef ==
>>>> Def)
>>>> &&
>>>> - "mismatched canonical definitions for declaration");
>>>> - OtherMerged.CanonicalDef = Def;
>>>> - }
>>>> -
>>>> - auto &Merged = MergedDefModules[Def];
>>>> - // Grab this again, we potentially just invalidated our reference.
>>>> - auto &OtherMerged = MergedDefModules[Other];
>>>> -
>>>> - if (Module *M = Other->getImportedOwningModule())
>>>> - Merged.MergedModules.push_back(M);
>>>> -
>>>> - // If this definition had any others merged into it, they're now
>>>> merged
>>>> into
>>>> - // the canonical definition instead.
>>>> - if (!OtherMerged.MergedModules.empty()) {
>>>> - assert(!Merged.CanonicalDef && "canonical definition not
>>>> canonical");
>>>> - if (Merged.MergedModules.empty())
>>>> - Merged.MergedModules = std::move(OtherMerged.MergedModules);
>>>> - else
>>>> - Merged.MergedModules.insert(Merged.MergedModules.end(),
>>>> - OtherMerged.MergedModules.begin(),
>>>> - OtherMerged.MergedModules.end());
>>>> - OtherMerged.MergedModules.clear();
>>>> - }
>>>> -}
>>>> -
>>>> void ASTContext::deduplicateMergedDefinitonsFor(NamedDecl *ND) {
>>>> auto It = MergedDefModules.find(ND);
>>>> if (It == MergedDefModules.end())
>>>> return;
>>>> - if (auto *CanonDef = It->second.CanonicalDef) {
>>>> - It = MergedDefModules.find(CanonDef);
>>>> - if (It == MergedDefModules.end())
>>>> - return;
>>>> - }
>>>> -
>>>> - auto &Merged = It->second.MergedModules;
>>>> + auto &Merged = It->second;
>>>> llvm::DenseSet<Module*> Found;
>>>> for (Module *&M : Merged)
>>>> if (!Found.insert(M).second)
>>>>
>>>> Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
>>>> URL:
>>>>
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=284382&r1=284381&r2=284382&view=diff
>>>>
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
>>>> +++ cfe/trunk/lib/Serialization/ASTReader.cpp Mon Oct 17 08:00:44 2016
>>>> @@ -3481,10 +3481,23 @@ void ASTReader::makeModuleVisible(Module
>>>> /// visible.
>>>> void ASTReader::mergeDefinitionVisibility(NamedDecl *Def,
>>>> NamedDecl *MergedDef) {
>>>> + // FIXME: This doesn't correctly handle the case where MergedDef is
>>>> visible
>>>> + // in modules other than its owning module. We should instead give
>>>> the
>>>> + // ASTContext a list of merged definitions for Def.
>>>> if (Def->isHidden()) {
>>>> // If MergedDef is visible or becomes visible, make the
>>>> definition
>>>> visible.
>>>> - getContext().mergeDefinitionIntoModulesOf(Def, MergedDef);
>>>> - PendingMergedDefinitionsToDeduplicate.insert(Def);
>>>> + if (!MergedDef->isHidden())
>>>> + Def->Hidden = false;
>>>> + else if (getContext().getLangOpts().ModulesLocalVisibility) {
>>>> + getContext().mergeDefinitionIntoModule(
>>>> + Def, MergedDef->getImportedOwningModule(),
>>>> + /*NotifyListeners*/ false);
>>>> + PendingMergedDefinitionsToDeduplicate.insert(Def);
>>>> + } else {
>>>> + auto SubmoduleID = MergedDef->getOwningModuleID();
>>>> + assert(SubmoduleID && "hidden definition in no module");
>>>> + HiddenNamesMap[getSubmodule(SubmoduleID)].push_back(Def);
>>>> + }
>>>> }
>>>> }
>>>> @@ -8619,7 +8632,7 @@ void ASTReader::finishPendingActions() {
>>>> const FunctionDecl *Defn = nullptr;
>>>> if (!getContext().getLangOpts().Modules || !FD->hasBody(Defn))
>>>> FD->setLazyBody(PB->second);
>>>> - else if (FD != Defn)
>>>> + else
>>>> mergeDefinitionVisibility(const_cast<FunctionDecl*>(Defn),
>>>> FD);
>>>> continue;
>>>> }
>>>>
>>>> Modified:
>>>> cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/a.h
>>>> URL:
>>>>
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/a.h?rev=284382&r1=284381&r2=284382&view=diff
>>>>
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/a.h
>>>> (original)
>>>> +++ cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/a.h
>>>> Mon Oct 17 08:00:44 2016
>>>> @@ -4,8 +4,3 @@ template<typename T> struct B;
>>>> template<typename, typename> struct A {};
>>>> template<typename T> struct B : A<T> {};
>>>> template<typename T> inline auto C(T) {}
>>>> -
>>>> -namespace CrossModuleMerge {
>>>> - template<typename T> inline auto D(T) {}
>>>> - struct E {};
>>>> -}
>>>>
>>>> Modified:
>>>> cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/b.h
>>>> URL:
>>>>
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/b.h?rev=284382&r1=284381&r2=284382&view=diff
>>>>
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/b.h
>>>> (original)
>>>> +++ cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/b.h
>>>> Mon Oct 17 08:00:44 2016
>>>> @@ -17,7 +17,4 @@ namespace CrossModuleMerge {
>>>> template<typename, typename> struct A {};
>>>> template<typename T> struct B : A<T> {};
>>>> template<typename T> inline auto C(T) {}
>>>> -
>>>> - template<typename T> inline auto D(T) {}
>>>> - struct E;
>>>> }
>>>>
>>>> Modified:
>>>> cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/c.h
>>>> URL:
>>>>
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/c.h?rev=284382&r1=284381&r2=284382&view=diff
>>>>
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/c.h
>>>> (original)
>>>> +++ cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/c.h
>>>> Mon Oct 17 08:00:44 2016
>>>> @@ -5,8 +5,5 @@ namespace CrossModuleMerge {
>>>> template<typename, typename> struct A {};
>>>> template<typename T> struct B : A<T> {};
>>>> template<typename T> inline auto C(T) {}
>>>> -
>>>> - template<typename T> inline auto D(T) {}
>>>> - struct E {};
>>>> }
>>>>
>>>> Modified:
>>>> cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/d.h
>>>> URL:
>>>>
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/d.h?rev=284382&r1=284381&r2=284382&view=diff
>>>>
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/d.h
>>>> (original)
>>>> +++ cfe/trunk/test/Modules/Inputs/merge-template-pattern-visibility/d.h
>>>> Mon Oct 17 08:00:44 2016
>>>> @@ -1,3 +1 @@
>>>> -namespace CrossModuleMerge {
>>>> - struct E {};
>>>> -}
>>>> +// d.h: empty
>>>>
>>>> Removed: cfe/trunk/test/Modules/Inputs/merge-var-template-def/a.h
>>>> URL:
>>>>
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-var-template-def/a.h?rev=284381&view=auto
>>>>
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Modules/Inputs/merge-var-template-def/a.h (original)
>>>> +++ cfe/trunk/test/Modules/Inputs/merge-var-template-def/a.h (removed)
>>>> @@ -1,6 +0,0 @@
>>>> -#ifndef A_H
>>>> -#define A_H
>>>> -template<typename T> struct A { static bool b; };
>>>> -template<typename T> bool A<T>::b = false;
>>>> -template<typename T> void *get() { return &(A<T>::b); }
>>>> -#endif
>>>>
>>>> Removed: cfe/trunk/test/Modules/Inputs/merge-var-template-def/b1.h
>>>> URL:
>>>>
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-var-template-def/b1.h?rev=284381&view=auto
>>>>
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Modules/Inputs/merge-var-template-def/b1.h (original)
>>>> +++ cfe/trunk/test/Modules/Inputs/merge-var-template-def/b1.h (removed)
>>>> @@ -1,7 +0,0 @@
>>>> -#ifndef B1_H
>>>> -#define B1_H
>>>> -template<typename T> struct A { static bool b; };
>>>> -template<typename T> bool A<T>::b = false;
>>>> -template<typename T> void *get() { return &(A<T>::b); }
>>>> -#include "a.h"
>>>> -#endif
>>>>
>>>> Removed: cfe/trunk/test/Modules/Inputs/merge-var-template-def/b2.h
>>>> URL:
>>>>
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-var-template-def/b2.h?rev=284381&view=auto
>>>>
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Modules/Inputs/merge-var-template-def/b2.h (original)
>>>> +++ cfe/trunk/test/Modules/Inputs/merge-var-template-def/b2.h (removed)
>>>> @@ -1,6 +0,0 @@
>>>> -#ifndef B2_H
>>>> -#define B2_H
>>>> -template<typename T> struct A { static bool b; };
>>>> -template<typename T> bool A<T>::b = false;
>>>> -template<typename T> void *get() { return &(A<T>::b); }
>>>> -#endif
>>>>
>>>> Removed:
>>>> cfe/trunk/test/Modules/Inputs/merge-var-template-def/module.modulemap
>>>> URL:
>>>>
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-var-template-def/module.modulemap?rev=284381&view=auto
>>>>
>>>>
>>>> ==============================================================================
>>>> ---
>>>> cfe/trunk/test/Modules/Inputs/merge-var-template-def/module.modulemap
>>>> (original)
>>>> +++
>>>> cfe/trunk/test/Modules/Inputs/merge-var-template-def/module.modulemap
>>>> (removed)
>>>> @@ -1,5 +0,0 @@
>>>> -module a { header "a.h" export * }
>>>> -module b {
>>>> - module b1 { header "b1.h" export * }
>>>> - module b2 { header "b2.h" export * }
>>>> -}
>>>>
>>>> Modified: cfe/trunk/test/Modules/merge-template-pattern-visibility.cpp
>>>> URL:
>>>>
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/merge-template-pattern-visibility.cpp?rev=284382&r1=284381&r2=284382&view=diff
>>>>
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Modules/merge-template-pattern-visibility.cpp
>>>> (original)
>>>> +++ cfe/trunk/test/Modules/merge-template-pattern-visibility.cpp Mon Oct
>>>> 17 08:00:44 2016
>>>> @@ -7,12 +7,6 @@
>>>> // RUN: -fmodules-local-submodule-visibility -o %t/Y.pcm
>>>> // RUN: %clang_cc1 -fmodules -fno-modules-error-recovery -std=c++14
>>>> -fmodule-file=%t/X.pcm -fmodule-file=%t/Y.pcm \
>>>> // RUN: -fmodules-local-submodule-visibility -verify %s
>>>> -I%S/Inputs/merge-template-pattern-visibility
>>>> -// RUN: %clang_cc1 -fmodules -fno-modules-error-recovery -std=c++14
>>>> -fmodule-file=%t/Y.pcm -fmodule-file=%t/X.pcm \
>>>> -// RUN: -fmodules-local-submodule-visibility -verify %s
>>>> -I%S/Inputs/merge-template-pattern-visibility
>>>> -// RUN: %clang_cc1 -fmodules -fno-modules-error-recovery -std=c++14
>>>> -fmodule-file=%t/X.pcm -fmodule-file=%t/Y.pcm \
>>>> -// RUN: -verify %s
>>>> -I%S/Inputs/merge-template-pattern-visibility
>>>> -// RUN: %clang_cc1 -fmodules -fno-modules-error-recovery -std=c++14
>>>> -fmodule-file=%t/Y.pcm -fmodule-file=%t/X.pcm \
>>>> -// RUN: -verify %s
>>>> -I%S/Inputs/merge-template-pattern-visibility
>>>> #include "b.h"
>>>> #include "d.h"
>>>> @@ -21,6 +15,4 @@
>>>> void g() {
>>>> CrossModuleMerge::B<int> bi;
>>>> CrossModuleMerge::C(0);
>>>> - CrossModuleMerge::D(0);
>>>> - CrossModuleMerge::E e;
>>>> }
>>>>
>>>> Removed: cfe/trunk/test/Modules/merge-var-template-def.cpp
>>>> URL:
>>>>
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/merge-var-template-def.cpp?rev=284381&view=auto
>>>>
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Modules/merge-var-template-def.cpp (original)
>>>> +++ cfe/trunk/test/Modules/merge-var-template-def.cpp (removed)
>>>> @@ -1,7 +0,0 @@
>>>> -// RUN: rm -rf %t
>>>> -// RUN: %clang_cc1 -I%S/Inputs/merge-var-template-def -verify -fmodules
>>>> -Werror=undefined-internal -fmodules-local-submodule-visibility
>>>> -fmodules-cache-path=%t -fimplicit-module-maps %s
>>>> -// expected-no-diagnostics
>>>> -
>>>> -#include "b2.h"
>>>> -namespace { struct X; }
>>>> -void *x = get<X>();
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>>
>>>
>
More information about the cfe-commits
mailing list