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 11:48:16 PST 2016
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 <mailto: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
>>> <mailto: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
>>> <mailto: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/ac9e37cb/attachment-0001.html>
-------------- next part --------------
From 4883ff35cdd196d56b5cd5d2ad40103d69063eac 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..69505b4 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);
+ // Only work on Avoid local VarDecls (function scope) incomplete ty
+ 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