[modules] PR24954
Vassil Vassilev via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 22 12:51:57 PST 2016
On 02/02/16 02:52, Richard Smith via cfe-commits wrote:
> On Thu, Jan 28, 2016 at 8:23 AM, Vassil Vassilev <vvasilev at cern.ch> wrote:
>> Would this patch be more reasonable? It follows what
>> RegisterTemplateSpecialization (introduced in r245779) does. AFAICT this
>> adds an update record far less often.
> It's still adding redundant update records. We'll write the
> appropriate update record as part of serializing the declaration
> itself, so we only need to ensure that the declaration is emitted, not
> actually emit an update record for it. Perhaps you could store a list
> of such declarations on the ASTWriter, and call GetDeclRef on each of
> them once we start emitting the AST file (or maybe just push them into
> UpdatingVisibleDecls). Please also restrict this to the template
> friend corner case.
The attached patch fixes the issues more or less in the direction you
suggest. I am not sure if I expressed well enough the conditions of the
template friend corner case. Could you have a look?
>
>> --Vassil
>>
>> On 12/12/15 16:13, Vassil Vassilev wrote:
>>
>> I couldn't find GetDecl routine in the ASTWriter. Could you elaborate?
>>
>> Assuming you meant ASTWriter::GetDeclRef(D): It seems that the conditions
>> when calling GetDeclRef differ from the conditions of
>> AddedCXXTemplateSpecialization. Eg:
>>
>> ASTWriter::AddedCXXTemplateSpecialization {
>> assert(!WritingAST && "Already writing the AST!");
>> ...
>> }
>> ASTWriter::GetDeclRef {
>> assert(WritingAST && "Cannot request a declaration ID before AST
>> writing");
>> ..
>> }
>>
>> IIUC this particular instantiation happens *after* module B was built, thus
>> it needs to be retrospectively added to the serialized namespace. It looks
>> like even avoiding somehow the asserts of GetDeclRef it wouldn't help much.
>>
>> Alternatively I could try to reduce the redundant update records by
>> narrowing down to instantiations coming in the context of friends.
>>
>> --Vassil
>>
>> On 12/12/15 01:07, Richard Smith wrote:
>>
>> Instead of adding an update record directly in this case (which will emit
>> far more update records than necessary), how about just calling GetDecl(D)
>> from AddedCXXTemplateSpecialization to ensure that it gets emitted?
>>
>> On Fri, Dec 4, 2015 at 7:46 AM, Vassil Vassilev <vvasilev at cern.ch> wrote:
>>> Hi,
>>> Could you review my fix please.
>>> Many thanks,
>>> Vassil
>>>
>>> On 08/10/15 15:53, Vassil Vassilev wrote:
>>>> Hi Richard,
>>>> I started working on https://llvm.org/bugs/show_bug.cgi?id=24954
>>>>
>>>> IIUC r228485 introduces an abstraction to deal with
>>>> not-really-anonymous friend decls
>>>> (serialization::needsAnonymousDeclarationNumber in ASTCommon.cpp).
>>>>
>>>> A comment explicitly says:
>>>> "// This doesn't apply to friend tag decls; Sema makes those available
>>>> to name
>>>> // lookup in the surrounding context."
>>>>
>>>> In the bug reproducer, the friend function (wrt __iom_t10) is forward
>>>> declared in the same namespace, where Sema makes the friend available for a
>>>> name lookup.
>>>>
>>>> It seems that the friend operator<< in __iom_t10 (sorry about the names
>>>> they come from libcxx) doesn't get registered in the ASTWriter's DeclIDs but
>>>> it gets registered in outer namespace's lookup table. Thus, assert is
>>>> triggered when finalizing module A, since it rebuilds the lookups of the
>>>> updated contexts.
>>>>
>>>> The issue only appears when building module A deserializes/uses module
>>>> B.
>>>>
>>>> Currently I was assume that something wrong happens in either
>>>> needsAnonymousDeclarationNumber or I hit a predicted issue
>>>> ASTWriterDecl.cpp:1602
>>>> // FIXME: This is not correct; when we reach an imported declaration
>>>> we
>>>> // won't emit its previous declaration.
>>>> (void)Writer.GetDeclRef(D->getPreviousDecl());
>>>> (void)Writer.GetDeclRef(MostRecent);
>>>>
>>>> The issue seems a fairly complex one and I am a bit stuck.
>>>>
>>>> Any hints are very very welcome ;)
>>>> Many thanks,
>>>> Vassil
>>>>
>>>>
>>>>
>>
>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
-------------- next part --------------
From 4efa41e3cba19d3dc2ac76c60a135a01fbd13380 Mon Sep 17 00:00:00 2001
From: Vassil Vassilev <v.g.vassilev at gmail.com>
Date: Sun, 27 Sep 2015 21:12:39 +0200
Subject: [PATCH] [modules] Fix adding a templated friend functions to a
namespace from another module.
When clang adds argument dependent lookup candidates, it can perform template
instantiations. For example, it can instantiate a templated friend function and
register it in the enclosing namespace's lookup table.
Fixes https://llvm.org/bugs/show_bug.cgi?id=24954
---
lib/Serialization/ASTWriter.cpp | 7 +++++--
test/Modules/Inputs/PR24954/A.h | 10 ++++++++++
test/Modules/Inputs/PR24954/B.h | 30 ++++++++++++++++++++++++++++
test/Modules/Inputs/PR24954/module.modulemap | 9 +++++++++
test/Modules/pr24954.cpp | 7 +++++++
5 files changed, 61 insertions(+), 2 deletions(-)
create mode 100644 test/Modules/Inputs/PR24954/A.h
create mode 100644 test/Modules/Inputs/PR24954/B.h
create mode 100644 test/Modules/Inputs/PR24954/module.modulemap
create mode 100644 test/Modules/pr24954.cpp
diff --git a/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp
index 1d1c998..6c71e04 100644
--- a/lib/Serialization/ASTWriter.cpp
+++ b/lib/Serialization/ASTWriter.cpp
@@ -5723,8 +5723,11 @@ static bool isImportedDeclContext(ASTReader *Chain, const Decl *D) {
}
void ASTWriter::AddedVisibleDecl(const DeclContext *DC, const Decl *D) {
- // TU and namespaces are handled elsewhere.
- if (isa<TranslationUnitDecl>(DC) || isa<NamespaceDecl>(DC))
+ // TU and namespaces are handled elsewhere. Exclude template instantiations of
+ // FunctionTemplateDecls in namespaces. This can happen on adding ADL lookup
+ // candidates, for example templated friends.
+ if (isa<TranslationUnitDecl>(DC) ||
+ (isa<NamespaceDecl>(DC) && !isa<FunctionTemplateDecl>(D)))
return;
// We're only interested in cases where a local declaration is added to an
diff --git a/test/Modules/Inputs/PR24954/A.h b/test/Modules/Inputs/PR24954/A.h
new file mode 100644
index 0000000..5e5d5bf
--- /dev/null
+++ b/test/Modules/Inputs/PR24954/A.h
@@ -0,0 +1,10 @@
+#include "B.h"
+
+template <class T>
+class Expr {
+public:
+ void print(B::basic_ostream<char>& os) {
+ os << B::setw(42);
+ os << B::endl;
+ }
+};
diff --git a/test/Modules/Inputs/PR24954/B.h b/test/Modules/Inputs/PR24954/B.h
new file mode 100644
index 0000000..a8ddc71
--- /dev/null
+++ b/test/Modules/Inputs/PR24954/B.h
@@ -0,0 +1,30 @@
+namespace B {
+
+ template <class _CharT>
+ struct basic_ostream {
+ basic_ostream& operator<<(basic_ostream& (*__pf)());
+ };
+
+
+ template <class _CharT> basic_ostream<_CharT>&
+ endl();
+
+ struct S1 {
+ template <class _CharT> friend void
+ operator<<(basic_ostream<_CharT>& __os, const S1& __x);
+ };
+
+ S1 setw(int __n);
+
+ template <class _CharT> class S2;
+
+ template <class _CharT> void
+ operator<<(basic_ostream<_CharT>& __os, const S2<_CharT>& __x);
+
+ template <class _CharT>
+ struct S2 {
+ template <class _Cp> friend void
+ operator<<(basic_ostream<_Cp>& __os, const S2<_Cp>& __x);
+ };
+
+}
diff --git a/test/Modules/Inputs/PR24954/module.modulemap b/test/Modules/Inputs/PR24954/module.modulemap
new file mode 100644
index 0000000..4937418
--- /dev/null
+++ b/test/Modules/Inputs/PR24954/module.modulemap
@@ -0,0 +1,9 @@
+module A {
+ header "A.h"
+ export *
+}
+
+module B {
+ header "B.h"
+ export *
+}
diff --git a/test/Modules/pr24954.cpp b/test/Modules/pr24954.cpp
new file mode 100644
index 0000000..407ee06
--- /dev/null
+++ b/test/Modules/pr24954.cpp
@@ -0,0 +1,7 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -I%S/Inputs/PR24954 -verify %s
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I%S/Inputs/PR24954 -verify %s
+
+#include "A.h"
+
+// expected-no-diagnostics
--
2.3.8 (Apple Git-58)
More information about the cfe-commits
mailing list