r190293 - C++ modules: fix a bug where loading a declaration with some name would prevent
Richard Smith
richard-llvm at metafoo.co.uk
Mon Sep 9 00:34:56 PDT 2013
Author: rsmith
Date: Mon Sep 9 02:34:56 2013
New Revision: 190293
URL: http://llvm.org/viewvc/llvm-project?rev=190293&view=rev
Log:
C++ modules: fix a bug where loading a declaration with some name would prevent
name lookup from lazily deserializing the other declarations with the same
name, by tracking a bit to indicate whether a name in a DeclContext might have
additional external results. This also allows lazier reconciling of the lookup
table if a module import adds decls to a pre-existing DC.
However, this exposes a pre-existing bug, which causes a regression in
test/Modules/decldef.mm: if we have a reference to a declaration, and a
later-imported module adds a redeclaration, nothing causes us to load that
redeclaration when we use or emit the reference (which can manifest as a
reference to an undefined inline function, a use of an incomplete type, and so
on). decldef.mm has been extended with an additional testcase which fails with
or without this change.
Modified:
cfe/trunk/include/clang/AST/DeclContextInternals.h
cfe/trunk/lib/AST/DeclBase.cpp
cfe/trunk/lib/Sema/SemaType.cpp
cfe/trunk/test/Modules/Inputs/def.h
cfe/trunk/test/Modules/Inputs/namespaces-top.h
cfe/trunk/test/Modules/cxx-templates.cpp
cfe/trunk/test/Modules/decldef.mm
cfe/trunk/test/Modules/namespaces.cpp
Modified: cfe/trunk/include/clang/AST/DeclContextInternals.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclContextInternals.h?rev=190293&r1=190292&r2=190293&view=diff
==============================================================================
--- cfe/trunk/include/clang/AST/DeclContextInternals.h (original)
+++ cfe/trunk/include/clang/AST/DeclContextInternals.h Mon Sep 9 02:34:56 2013
@@ -18,6 +18,7 @@
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclarationName.h"
#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/PointerIntPair.h"
#include "llvm/ADT/PointerUnion.h"
#include "llvm/ADT/SmallVector.h"
#include <algorithm>
@@ -26,23 +27,29 @@ namespace clang {
class DependentDiagnostic;
-/// StoredDeclsList - This is an array of decls optimized a common case of only
-/// containing one entry.
+/// \brief An array of decls optimized for the common case of only containing
+/// one entry.
struct StoredDeclsList {
- /// DeclsTy - When in vector form, this is what the Data pointer points to.
+ /// \brief When in vector form, this is what the Data pointer points to.
typedef SmallVector<NamedDecl *, 4> DeclsTy;
+ /// \brief A collection of declarations, with a flag to indicate if we have
+ /// further external declarations.
+ typedef llvm::PointerIntPair<DeclsTy *, 1, bool> DeclsAndHasExternalTy;
+
/// \brief The stored data, which will be either a pointer to a NamedDecl,
- /// or a pointer to a vector.
- llvm::PointerUnion<NamedDecl *, DeclsTy *> Data;
+ /// or a pointer to a vector with a flag to indicate if there are further
+ /// external declarations.
+ llvm::PointerUnion<NamedDecl*, DeclsAndHasExternalTy> Data;
public:
StoredDeclsList() {}
StoredDeclsList(const StoredDeclsList &RHS) : Data(RHS.Data) {
if (DeclsTy *RHSVec = RHS.getAsVector())
- Data = new DeclsTy(*RHSVec);
+ Data = DeclsAndHasExternalTy(new DeclsTy(*RHSVec),
+ RHS.hasExternalDecls());
}
~StoredDeclsList() {
@@ -56,7 +63,7 @@ public:
delete Vector;
Data = RHS.Data;
if (DeclsTy *RHSVec = RHS.getAsVector())
- Data = new DeclsTy(*RHSVec);
+ Data = DeclsAndHasExternalTy(new DeclsTy(*RHSVec), hasExternalDecls());
return *this;
}
@@ -66,8 +73,27 @@ public:
return Data.dyn_cast<NamedDecl *>();
}
+ DeclsAndHasExternalTy getAsVectorAndHasExternal() const {
+ return Data.dyn_cast<DeclsAndHasExternalTy>();
+ }
+
DeclsTy *getAsVector() const {
- return Data.dyn_cast<DeclsTy *>();
+ return getAsVectorAndHasExternal().getPointer();
+ }
+
+ bool hasExternalDecls() const {
+ return getAsVectorAndHasExternal().getInt();
+ }
+
+ void setHasExternalDecls() {
+ if (DeclsTy *Vec = getAsVector())
+ Data = DeclsAndHasExternalTy(Vec, true);
+ else {
+ DeclsTy *VT = new DeclsTy();
+ if (NamedDecl *OldD = getAsDecl())
+ VT->push_back(OldD);
+ Data = DeclsAndHasExternalTy(VT, true);
+ }
}
void setOnlyValue(NamedDecl *ND) {
@@ -110,6 +136,8 @@ public:
Vec.erase(std::remove_if(Vec.begin(), Vec.end(),
std::mem_fun(&Decl::isFromASTFile)),
Vec.end());
+ // Don't have any external decls any more.
+ Data = DeclsAndHasExternalTy(&Vec, false);
}
}
@@ -165,12 +193,14 @@ public:
/// not a redeclaration to merge it into the appropriate place in our list.
///
void AddSubsequentDecl(NamedDecl *D) {
+ assert(!isNull() && "don't AddSubsequentDecl when we have no decls");
+
// If this is the second decl added to the list, convert this to vector
// form.
if (NamedDecl *OldD = getAsDecl()) {
DeclsTy *VT = new DeclsTy();
VT->push_back(OldD);
- Data = VT;
+ Data = DeclsAndHasExternalTy(VT, false);
}
DeclsTy &Vec = *getAsVector();
Modified: cfe/trunk/lib/AST/DeclBase.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclBase.cpp?rev=190293&r1=190292&r2=190293&view=diff
==============================================================================
--- cfe/trunk/lib/AST/DeclBase.cpp (original)
+++ cfe/trunk/lib/AST/DeclBase.cpp Mon Sep 9 02:34:56 2013
@@ -926,11 +926,8 @@ void DeclContext::reconcileExternalVisib
NeedToReconcileExternalVisibleStorage = false;
StoredDeclsMap &Map = *LookupPtr.getPointer();
- ExternalASTSource *Source = getParentASTContext().getExternalSource();
- for (StoredDeclsMap::iterator I = Map.begin(); I != Map.end(); ++I) {
- I->second.removeExternalDecls();
- Source->FindExternalVisibleDeclsByName(this, I->first);
- }
+ for (StoredDeclsMap::iterator I = Map.begin(); I != Map.end(); ++I)
+ I->second.setHasExternalDecls();
}
/// \brief Load the declarations within this lexical storage from an
@@ -983,8 +980,7 @@ ExternalASTSource::SetNoExternalVisibleD
if (!(Map = DC->LookupPtr.getPointer()))
Map = DC->CreateStoredDeclsMap(Context);
- // Add an entry to the map for this name, if it's not already present.
- (*Map)[Name];
+ (*Map)[Name].removeExternalDecls();
return DeclContext::lookup_result();
}
@@ -1246,16 +1242,14 @@ DeclContext::lookup(DeclarationName Name
if (!Map)
Map = CreateStoredDeclsMap(getParentASTContext());
- // If a PCH/module has a result for this name, and we have a local
- // declaration, we will have imported the PCH/module result when adding the
- // local declaration or when reconciling the module.
+ // If we have a lookup result with no external decls, we are done.
std::pair<StoredDeclsMap::iterator, bool> R =
Map->insert(std::make_pair(Name, StoredDeclsList()));
- if (!R.second)
+ if (!R.second && !R.first->second.hasExternalDecls())
return R.first->second.getLookupResult();
ExternalASTSource *Source = getParentASTContext().getExternalSource();
- if (Source->FindExternalVisibleDeclsByName(this, Name)) {
+ if (Source->FindExternalVisibleDeclsByName(this, Name) || R.second) {
if (StoredDeclsMap *Map = LookupPtr.getPointer()) {
StoredDeclsMap::iterator I = Map->find(Name);
if (I != Map->end())
@@ -1304,7 +1298,8 @@ DeclContext::noload_lookup(DeclarationNa
LookupPtr.setInt(false);
// There may now be names for which we have local decls but are
- // missing the external decls.
+ // missing the external decls. FIXME: Just set the hasExternalDecls
+ // flag on those names that have external decls.
NeedToReconcileExternalVisibleStorage = true;
Map = LookupPtr.getPointer();
@@ -1461,7 +1456,18 @@ void DeclContext::makeDeclVisibleInConte
// Insert this declaration into the map.
StoredDeclsList &DeclNameEntries = (*Map)[D->getDeclName()];
- if (DeclNameEntries.isNull()) {
+
+ if (Internal) {
+ // If this is being added as part of loading an external declaration,
+ // this may not be the only external declaration with this name.
+ // In this case, we never try to replace an existing declaration; we'll
+ // handle that when we finalize the list of declarations for this name.
+ DeclNameEntries.setHasExternalDecls();
+ DeclNameEntries.AddSubsequentDecl(D);
+ return;
+ }
+
+ else if (DeclNameEntries.isNull()) {
DeclNameEntries.setOnlyValue(D);
return;
}
Modified: cfe/trunk/lib/Sema/SemaType.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaType.cpp?rev=190293&r1=190292&r2=190293&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaType.cpp (original)
+++ cfe/trunk/lib/Sema/SemaType.cpp Mon Sep 9 02:34:56 2013
@@ -5070,6 +5070,14 @@ bool Sema::RequireCompleteTypeImpl(Sourc
return false;
}
+ // FIXME: If there's an unimported definition of this type in a module (for
+ // instance, because we forward declared it, then imported the definition),
+ // import that definition now.
+ // FIXME: What about other cases where an import extends a redeclaration
+ // chain for a declaration that can be accessed through a mechanism other
+ // than name lookup (eg, referenced in a template, or a variable whose type
+ // could be completed by the module)?
+
const TagType *Tag = T->getAs<TagType>();
const ObjCInterfaceType *IFace = 0;
Modified: cfe/trunk/test/Modules/Inputs/def.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/def.h?rev=190293&r1=190292&r2=190293&view=diff
==============================================================================
--- cfe/trunk/test/Modules/Inputs/def.h (original)
+++ cfe/trunk/test/Modules/Inputs/def.h Mon Sep 9 02:34:56 2013
@@ -17,4 +17,11 @@ class Def2 {
public:
void func();
};
+
+namespace Def3NS {
+ class Def3 {
+ public:
+ void func();
+ };
+}
#endif
Modified: cfe/trunk/test/Modules/Inputs/namespaces-top.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/namespaces-top.h?rev=190293&r1=190292&r2=190293&view=diff
==============================================================================
--- cfe/trunk/test/Modules/Inputs/namespaces-top.h (original)
+++ cfe/trunk/test/Modules/Inputs/namespaces-top.h Mon Sep 9 02:34:56 2013
@@ -12,3 +12,8 @@ namespace N3 {
namespace N12 { }
+namespace N13 {
+ void f();
+ int f(int);
+ void (*p)() = &f;
+}
Modified: cfe/trunk/test/Modules/cxx-templates.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/cxx-templates.cpp?rev=190293&r1=190292&r2=190293&view=diff
==============================================================================
--- cfe/trunk/test/Modules/cxx-templates.cpp (original)
+++ cfe/trunk/test/Modules/cxx-templates.cpp Mon Sep 9 02:34:56 2013
@@ -82,11 +82,8 @@ typedef SomeTemplate<int&> SomeTemplateI
SomeTemplate<char*> some_template_char_ptr;
SomeTemplate<char&> some_template_char_ref;
-// FIXME: There should only be two 'f's here.
// CHECK-GLOBAL: DeclarationName 'f'
// CHECK-GLOBAL-NEXT: |-FunctionTemplate {{.*}} 'f'
-// CHECK-GLOBAL-NEXT: |-FunctionTemplate {{.*}} 'f'
-// CHECK-GLOBAL-NEXT: |-FunctionTemplate {{.*}} 'f'
// CHECK-GLOBAL-NEXT: `-FunctionTemplate {{.*}} 'f'
// CHECK-NAMESPACE-N: DeclarationName 'f'
Modified: cfe/trunk/test/Modules/decldef.mm
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/decldef.mm?rev=190293&r1=190292&r2=190293&view=diff
==============================================================================
--- cfe/trunk/test/Modules/decldef.mm (original)
+++ cfe/trunk/test/Modules/decldef.mm Mon Sep 9 02:34:56 2013
@@ -6,8 +6,10 @@
@class Def;
Def *def;
-class Def2;
+class Def2; // expected-note {{forward decl}}
Def2 *def2;
+namespace Def3NS { class Def3; } // expected-note {{forward decl}}
+Def3NS::Def3 *def3;
@interface Unrelated
- defMethod;
@@ -39,5 +41,9 @@ void testDef() {
}
void testDef2() {
- def2->func();
+ // FIXME: These should both work, since we've (implicitly) imported
+ // decldef.Def here, but they don't, because nothing has triggered the lazy
+ // loading of the definitions of these classes.
+ def2->func(); // expected-error {{incomplete}}
+ def3->func(); // expected-error {{incomplete}}
}
Modified: cfe/trunk/test/Modules/namespaces.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/namespaces.cpp?rev=190293&r1=190292&r2=190293&view=diff
==============================================================================
--- cfe/trunk/test/Modules/namespaces.cpp (original)
+++ cfe/trunk/test/Modules/namespaces.cpp Mon Sep 9 02:34:56 2013
@@ -75,3 +75,10 @@ void testAnonymousNotMerged() {
// expected-note at Inputs/namespaces-right.h:60 {{passing argument to parameter here}}
// expected-note at Inputs/namespaces-right.h:67 {{passing argument to parameter here}}
+
+// Test that bringing in one name from an overload set does not hide the rest.
+void testPartialImportOfOverloadSet() {
+ void (*p)() = N13::p;
+ p();
+ N13::f(0);
+}
More information about the cfe-commits
mailing list