On 02/02/16 02:52, Richard Smith via cfe-commits wrote:
On Thu, Jan 28, 2016 at 8:23 AM, Vassil Vassilev <vvasi...@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 <vvasi...@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@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
From 4efa41e3cba19d3dc2ac76c60a135a01fbd13380 Mon Sep 17 00:00:00 2001
From: Vassil Vassilev <v.g.vassi...@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)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
