r258524 - Merge templated static member variables, fixes http://llvm.org/pr26179.
Vassil Vassilev via cfe-commits
cfe-commits at lists.llvm.org
Sun Feb 7 12:33:27 PST 2016
Improve a comment.
--Vassil
On 07/02/16 20:48, Vassil Vassilev wrote:
> Would this patch be any better?
> --Vassil
> On 05/02/16 21:49, Richard Smith wrote:
>> This belongs in ASTDeclReader::attachPreviousDecl[Impl]. That's where
>> we propagate data that's supposed to be consistent across a
>> redeclaration chain from earlier declarations to later ones.
>>
>> There's another wrinkle here: we should only be propagating the type
>> from a previous declaration that's declared in the same scope (in
>> particular, we should skip over declarations declared as local extern
>> declarations within a function). This may in some cases require
>> walking backwards along the redeclaration chain to find the previous
>> declaration that was not a local extern declaration. That'd be
>> observable in a case like this:
>>
>> modulemap:
>> module A { module A1 { header "a1.h" } module A2 { header "a2.h" } }
>> module B { header "b.h" }
>>
>> a1.h:
>> int a[];
>>
>> b.h:
>> void g(int(*)[], int);
>> void g(int(*)[1], int) = delete;
>> template<typename T> void f(T t) {
>> extern int a[];
>> g(&a, t);
>> }
>>
>> a2.h:
>> int a[1];
>>
>> x.cc:
>> #include "a1.h"
>> #include "b.h"
>> void h() { f(0); } // should not produce an error; type of 'a' within
>> 'f' should not have been updated
>>
>> That example actually reveals another problem: we really don't want
>> the completed type to be visible unless there is a visible
>> declaration with the completed type. That suggests that propagating
>> the type across the redeclaration chain may be the wrong approach,
>> and we should instead handle this by changing isPreferredLookupResult
>> (in SemaLookup.cpp) to prefer a VarDecl with a complete type over one
>> with an incomplete type.
>>
>> On Fri, Feb 5, 2016 at 12:27 PM, Vassil Vassilev
>> <v.g.vassilev at gmail.com> wrote:
>>
>> I am not sure where else to put this logic if not in
>> isSameEntity... Could you point me to a better place?
>> --Vassil
>>
>> On 05/02/16 19:56, Richard Smith wrote:
>>> On Fri, Feb 5, 2016 at 7:04 AM, Vassil Vassilev
>>> <v.g.vassilev at gmail.com <mailto:v.g.vassilev at gmail.com>> wrote:
>>>
>>> Good point. Do you have in mind calling
>>> 'clang::Sema::MergeVarDeclTypes' or we to just "duplicate"
>>> the logic in clang::ASTDeclReader::mergeRedeclarable?
>>>
>>>
>>> It's not safe to call into Sema while we're in a
>>> partially-deserialized state. We know the only interesting case
>>> here is complete versus incomplete array types, so we don't need
>>> anything like the complexity of MergeVarDeclTypes -- something
>>> as easy as "if (new type is incomplete and old type is not) set
>>> new type to old type" should work.
>>>
>>> On 05/02/16 02:50, Richard Smith via cfe-commits wrote:
>>>> I suspect we'll need to do a little more than this: when we
>>>> rebuild the redeclaration chain, we should probably
>>>> propagate the complete type to later redeclarations in the
>>>> same scope. Otherwise, if we import a module that has a
>>>> complete type and one that has an incomplete type, the
>>>> declaration found by name lookup might be the one with the
>>>> incomplete type, possibly resulting in rejects-valid on
>>>> code like this:
>>>>
>>>> a.h:
>>>> extern int a[5];
>>>>
>>>> b.h:
>>>> extern int a[];
>>>>
>>>> x.cc:
>>>> #include "a.h"
>>>> #include "b.h"
>>>> int k = sizeof(a);
>>>>
>>>> On Fri, Jan 22, 2016 at 11:03 AM, Yaron Keren via
>>>> cfe-commits <cfe-commits at lists.llvm.org> wrote:
>>>>
>>>> Author: yrnkrn
>>>> Date: Fri Jan 22 13:03:27 2016
>>>> New Revision: 258524
>>>>
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project?rev=258524&view=rev
>>>> Log:
>>>> Merge templated static member variables, fixes
>>>> http://llvm.org/pr26179.
>>>>
>>>> Patch by Vassil Vassilev!
>>>> Reviewed by Richard Smith.
>>>>
>>>>
>>>> Added:
>>>> cfe/trunk/test/Modules/Inputs/PR26179/
>>>> cfe/trunk/test/Modules/Inputs/PR26179/A.h
>>>> cfe/trunk/test/Modules/Inputs/PR26179/B.h
>>>> cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h
>>>> cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap
>>>> cfe/trunk/test/Modules/pr26179.cpp
>>>> Modified:
>>>> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>>>>
>>>> Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=258524&r1=258523&r2=258524&view=diff
>>>> ==============================================================================
>>>> --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>>>> (original)
>>>> +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Fri
>>>> Jan 22 13:03:27 2016
>>>> @@ -2595,8 +2595,24 @@ static bool
>>>> isSameEntity(NamedDecl *X, N
>>>> // Variables with the same type and linkage match.
>>>> if (VarDecl *VarX = dyn_cast<VarDecl>(X)) {
>>>> VarDecl *VarY = cast<VarDecl>(Y);
>>>> - return (VarX->getLinkageInternal() ==
>>>> VarY->getLinkageInternal()) &&
>>>> - VarX->getASTContext().hasSameType(VarX->getType(),
>>>> VarY->getType());
>>>> + if (VarX->getLinkageInternal() ==
>>>> VarY->getLinkageInternal()) {
>>>> + ASTContext &C = VarX->getASTContext();
>>>> + if (C.hasSameType(VarX->getType(), VarY->getType()))
>>>> + return true;
>>>> +
>>>> + // We can get decls with different types on the
>>>> redecl chain. Eg.
>>>> + // template <typename T> struct S { static T
>>>> Var[]; }; // #1
>>>> + // template <typename T> T S<T>::Var[sizeof(T)];
>>>> // #2
>>>> + // Only? happens when completing an incomplete
>>>> array type. In this case
>>>> + // when comparing #1 and #2 we should go through
>>>> their elements types.
>>>> + const ArrayType *VarXTy =
>>>> C.getAsArrayType(VarX->getType());
>>>> + const ArrayType *VarYTy =
>>>> C.getAsArrayType(VarY->getType());
>>>> + if (!VarXTy || !VarYTy)
>>>> + return false;
>>>> + if (VarXTy->isIncompleteArrayType() ||
>>>> VarYTy->isIncompleteArrayType())
>>>> + return C.hasSameType(VarXTy->getElementType(),
>>>> VarYTy->getElementType());
>>>> + }
>>>> + return false;
>>>> }
>>>>
>>>> // Namespaces with the same name and inlinedness match.
>>>>
>>>> Added: cfe/trunk/test/Modules/Inputs/PR26179/A.h
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/A.h?rev=258524&view=auto
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Modules/Inputs/PR26179/A.h (added)
>>>> +++ cfe/trunk/test/Modules/Inputs/PR26179/A.h Fri Jan
>>>> 22 13:03:27 2016
>>>> @@ -0,0 +1,4 @@
>>>> +#include "basic_string.h"
>>>> +#include "B.h"
>>>> +
>>>> +int *p = a;
>>>>
>>>> Added: cfe/trunk/test/Modules/Inputs/PR26179/B.h
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/B.h?rev=258524&view=auto
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Modules/Inputs/PR26179/B.h (added)
>>>> +++ cfe/trunk/test/Modules/Inputs/PR26179/B.h Fri Jan
>>>> 22 13:03:27 2016
>>>> @@ -0,0 +1,2 @@
>>>> +#include "basic_string.h"
>>>> +extern int a[5];
>>>>
>>>> Added: cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h?rev=258524&view=auto
>>>> ==============================================================================
>>>> ---
>>>> cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h
>>>> (added)
>>>> +++
>>>> cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h
>>>> Fri Jan 22 13:03:27 2016
>>>> @@ -0,0 +1,14 @@
>>>> +#ifndef _GLIBCXX_STRING
>>>> +#define _GLIBCXX_STRING 1
>>>> +
>>>> +template<typename T>
>>>> +struct basic_string {
>>>> + static T _S_empty_rep_storage[];
>>>> +};
>>>> +
>>>> +template<typename T>
>>>> +T basic_string<T>::_S_empty_rep_storage[sizeof(T)];
>>>> +
>>>> +extern int a[];
>>>> +
>>>> +#endif
>>>>
>>>> Added:
>>>> cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap?rev=258524&view=auto
>>>> ==============================================================================
>>>> ---
>>>> cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap
>>>> (added)
>>>> +++
>>>> cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap
>>>> Fri Jan 22 13:03:27 2016
>>>> @@ -0,0 +1,9 @@
>>>> +module A {
>>>> + header "A.h"
>>>> + export *
>>>> +}
>>>> +
>>>> +module B {
>>>> + header "B.h"
>>>> + export *
>>>> +}
>>>>
>>>> Added: cfe/trunk/test/Modules/pr26179.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/pr26179.cpp?rev=258524&view=auto
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Modules/pr26179.cpp (added)
>>>> +++ cfe/trunk/test/Modules/pr26179.cpp Fri Jan 22
>>>> 13:03:27 2016
>>>> @@ -0,0 +1,7 @@
>>>> +// RUN: rm -rf %t
>>>> +// RUN: %clang_cc1 -I%S/Inputs/PR26179 -verify %s
>>>> +// RUN: %clang_cc1 -fmodules
>>>> -fmodule-map-file=%S/Inputs/PR26179/module.modulemap
>>>> -fmodules-cache-path=%t -I%S/Inputs/PR26179 -verify %s
>>>> +
>>>> +#include "A.h"
>>>> +
>>>> +// expected-no-diagnostics
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160207/da9ee574/attachment-0001.html>
-------------- next part --------------
From 93c94e46e42dd68597e964a20e17948287e5849d Mon Sep 17 00:00:00 2001
From: Vassil Vassilev <v.g.vassilev at gmail.com>
Date: Fri, 5 Feb 2016 21:15:05 +0100
Subject: [PATCH] [modules] Prefer more complete variable types.
---
lib/Sema/SemaLookup.cpp | 19 +++++++++++++++++++
lib/Serialization/ASTReaderDecl.cpp | 2 +-
test/Modules/Inputs/PR26179/A.h | 2 --
test/Modules/Inputs/PR26179/B.h | 1 -
test/Modules/Inputs/PR26179/basic_string.h | 2 --
.../Modules/Inputs/merge-incomplete-array-vars/a.h | 4 ++++
.../Modules/Inputs/merge-incomplete-array-vars/b.h | 4 ++++
.../Inputs/merge-incomplete-array-vars/c1.h | 1 +
.../Inputs/merge-incomplete-array-vars/c2.h | 1 +
.../Modules/Inputs/merge-incomplete-array-vars/d.h | 7 +++++++
.../merge-incomplete-array-vars/module.modulemap | 5 +++++
test/Modules/merge-incomplete-array-vars.cpp | 22 ++++++++++++++++++++++
12 files changed, 64 insertions(+), 6 deletions(-)
create mode 100644 test/Modules/Inputs/merge-incomplete-array-vars/a.h
create mode 100644 test/Modules/Inputs/merge-incomplete-array-vars/b.h
create mode 100644 test/Modules/Inputs/merge-incomplete-array-vars/c1.h
create mode 100644 test/Modules/Inputs/merge-incomplete-array-vars/c2.h
create mode 100644 test/Modules/Inputs/merge-incomplete-array-vars/d.h
create mode 100644 test/Modules/Inputs/merge-incomplete-array-vars/module.modulemap
create mode 100644 test/Modules/merge-incomplete-array-vars.cpp
diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp
index 1d7b15c..0d92b62 100644
--- a/lib/Sema/SemaLookup.cpp
+++ b/lib/Sema/SemaLookup.cpp
@@ -419,6 +419,25 @@ static bool isPreferredLookupResult(Sema &S, Sema::LookupNameKind Kind,
}
}
+ // VarDecl can have incomplete array types, prefer the one with more complete
+ // array type.
+ if (VarDecl *DVD = dyn_cast<VarDecl>(DUnderlying)) {
+ VarDecl *EVD = cast<VarDecl>(EUnderlying);
+ // Skip local VarDecls with incomplete array type.
+ if ((!DVD->isLocalVarDecl() && DVD->hasExternalStorage()) ||
+ (!EVD->isLocalVarDecl() && EVD->hasExternalStorage())) {
+
+ ASTContext &C = S.getASTContext();
+
+ if (const ArrayType *DVDTy = C.getAsArrayType(DVD->getType())) {
+ const ArrayType *EVDTy = C.getAsArrayType(EVD->getType());
+ // Prefer the decl with more complete type if visible.
+ return !DVDTy->isIncompleteArrayType() && EVDTy->isIncompleteArrayType()
+ && S.isVisible(D);
+ }
+ }
+ }
+
// For most kinds of declaration, it doesn't really matter which one we pick.
if (!isa<FunctionDecl>(DUnderlying) && !isa<VarDecl>(DUnderlying)) {
// If the existing declaration is hidden, prefer the new one. Otherwise,
diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp
index 9bcd9a0..feb9d12 100644
--- a/lib/Serialization/ASTReaderDecl.cpp
+++ b/lib/Serialization/ASTReaderDecl.cpp
@@ -2604,7 +2604,7 @@ static bool isSameEntity(NamedDecl *X, NamedDecl *Y) {
// template <typename T> struct S { static T Var[]; }; // #1
// template <typename T> T S<T>::Var[sizeof(T)]; // #2
// Only? happens when completing an incomplete array type. In this case
- // when comparing #1 and #2 we should go through their elements types.
+ // when comparing #1 and #2 we should go through their element type.
const ArrayType *VarXTy = C.getAsArrayType(VarX->getType());
const ArrayType *VarYTy = C.getAsArrayType(VarY->getType());
if (!VarXTy || !VarYTy)
diff --git a/test/Modules/Inputs/PR26179/A.h b/test/Modules/Inputs/PR26179/A.h
index ce95faf..c264f4c 100644
--- a/test/Modules/Inputs/PR26179/A.h
+++ b/test/Modules/Inputs/PR26179/A.h
@@ -1,4 +1,2 @@
#include "basic_string.h"
#include "B.h"
-
-int *p = a;
diff --git a/test/Modules/Inputs/PR26179/B.h b/test/Modules/Inputs/PR26179/B.h
index eb8d1c2..46a109e 100644
--- a/test/Modules/Inputs/PR26179/B.h
+++ b/test/Modules/Inputs/PR26179/B.h
@@ -1,2 +1 @@
#include "basic_string.h"
-extern int a[5];
diff --git a/test/Modules/Inputs/PR26179/basic_string.h b/test/Modules/Inputs/PR26179/basic_string.h
index afd1e0d..653ce07 100644
--- a/test/Modules/Inputs/PR26179/basic_string.h
+++ b/test/Modules/Inputs/PR26179/basic_string.h
@@ -9,6 +9,4 @@ struct basic_string {
template<typename T>
T basic_string<T>::_S_empty_rep_storage[sizeof(T)];
-extern int a[];
-
#endif
diff --git a/test/Modules/Inputs/merge-incomplete-array-vars/a.h b/test/Modules/Inputs/merge-incomplete-array-vars/a.h
new file mode 100644
index 0000000..4dcf7de
--- /dev/null
+++ b/test/Modules/Inputs/merge-incomplete-array-vars/a.h
@@ -0,0 +1,4 @@
+extern int a[5];
+extern int aa[55];
+extern int b[];
+extern int bb[];
diff --git a/test/Modules/Inputs/merge-incomplete-array-vars/b.h b/test/Modules/Inputs/merge-incomplete-array-vars/b.h
new file mode 100644
index 0000000..8486136
--- /dev/null
+++ b/test/Modules/Inputs/merge-incomplete-array-vars/b.h
@@ -0,0 +1,4 @@
+extern int a[];
+extern int aa[];
+extern int b[6];
+extern int bb[65];
diff --git a/test/Modules/Inputs/merge-incomplete-array-vars/c1.h b/test/Modules/Inputs/merge-incomplete-array-vars/c1.h
new file mode 100644
index 0000000..99be230
--- /dev/null
+++ b/test/Modules/Inputs/merge-incomplete-array-vars/c1.h
@@ -0,0 +1 @@
+extern int c[];
diff --git a/test/Modules/Inputs/merge-incomplete-array-vars/c2.h b/test/Modules/Inputs/merge-incomplete-array-vars/c2.h
new file mode 100644
index 0000000..3fee8c7
--- /dev/null
+++ b/test/Modules/Inputs/merge-incomplete-array-vars/c2.h
@@ -0,0 +1 @@
+extern int c[1];
diff --git a/test/Modules/Inputs/merge-incomplete-array-vars/d.h b/test/Modules/Inputs/merge-incomplete-array-vars/d.h
new file mode 100644
index 0000000..e24cf76
--- /dev/null
+++ b/test/Modules/Inputs/merge-incomplete-array-vars/d.h
@@ -0,0 +1,7 @@
+extern int c[];
+void g(int(*)[], int);
+void g(int(*)[1], int) = delete;
+template<typename T> void f(T t) {
+ extern int c[];
+ g(&c, t);
+}
diff --git a/test/Modules/Inputs/merge-incomplete-array-vars/module.modulemap b/test/Modules/Inputs/merge-incomplete-array-vars/module.modulemap
new file mode 100644
index 0000000..d2b467d
--- /dev/null
+++ b/test/Modules/Inputs/merge-incomplete-array-vars/module.modulemap
@@ -0,0 +1,5 @@
+module A { header "a.h" export * }
+module B { header "b.h" export * }
+
+module C { module C1 { header "c1.h" } module C2 { header "c2.h" } }
+module D { header "d.h" }
diff --git a/test/Modules/merge-incomplete-array-vars.cpp b/test/Modules/merge-incomplete-array-vars.cpp
new file mode 100644
index 0000000..4b0c208
--- /dev/null
+++ b/test/Modules/merge-incomplete-array-vars.cpp
@@ -0,0 +1,22 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++11 -I%S/Inputs/merge-incomplete-array-vars -verify %s
+// RUN: %clang_cc1 -std=c++11 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I%S/Inputs/merge-incomplete-array-vars -verify %s
+
+
+extern int aa[];
+extern int bb[];
+
+#include "a.h"
+#include "b.h"
+
+// Import the complete type first and the incomplete second, forcing the
+// most recent decl to have an incomplete type.
+int k = sizeof(a);
+int l = sizeof(aa);
+int m = sizeof(b);
+int n = sizeof(bb);
+
+#include "c1.h"
+#include "d.h"
+int size = sizeof(c); // expected-error{{invalid application of 'sizeof' to an incomplete type 'int []'}}
+void h() { f(0); } // should not produce an error; type of 'c' within 'f' should not have been updated
--
2.3.8 (Apple Git-58)
More information about the cfe-commits
mailing list