Would this patch be more reasonable? It follows what
RegisterTemplateSpecialization (introduced in r245779) does. AFAICT this
adds an update record far less often.
--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
From 2c12f1539c36548f7558e67a91e702c9233c0007 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] Fix adding a templated friend functions to a namespace from
another module.
This should be reproducible with chained PCHs too.
Fixes https://llvm.org/bugs/show_bug.cgi?id=24954
---
include/clang/Serialization/ASTWriter.h | 3 +++
lib/Serialization/ASTWriter.cpp | 20 +++++++++++++++++++
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 +++++++
6 files changed, 79 insertions(+)
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/include/clang/Serialization/ASTWriter.h
b/include/clang/Serialization/ASTWriter.h
index ed34547..0f2ce34 100644
--- a/include/clang/Serialization/ASTWriter.h
+++ b/include/clang/Serialization/ASTWriter.h
@@ -865,6 +865,9 @@ public:
void CompletedTagDefinition(const TagDecl *D) override;
void AddedVisibleDecl(const DeclContext *DC, const Decl *D) override;
void AddedCXXImplicitMember(const CXXRecordDecl *RD, const Decl *D) override;
+ using ASTMutationListener::AddedCXXTemplateSpecialization;
+ void AddedCXXTemplateSpecialization(const FunctionTemplateDecl *TD,
+ const FunctionDecl *D) override;
void ResolvedExceptionSpec(const FunctionDecl *FD) override;
void DeducedReturnType(const FunctionDecl *FD, QualType ReturnType) override;
void ResolvedOperatorDelete(const CXXDestructorDecl *DD,
diff --git a/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp
index 128935c..05a5331 100644
--- a/lib/Serialization/ASTWriter.cpp
+++ b/lib/Serialization/ASTWriter.cpp
@@ -5710,6 +5710,26 @@ void ASTWriter::AddedCXXImplicitMember(const
CXXRecordDecl *RD, const Decl *D) {
DeclUpdates[RD].push_back(DeclUpdate(UPD_CXX_ADDED_IMPLICIT_MEMBER, D));
}
+void ASTWriter::AddedCXXTemplateSpecialization(const FunctionTemplateDecl *TD,
+ const FunctionDecl *D) {
+ assert(!WritingAST && "Already writing the AST!");
+
+ // TODO: The checks are dups of RegisterTemplateSpecialization.
+ // Factor the checks out.
+
+ // A decl coming from PCH was modified.
+ if (!TD->isFromASTFile())
+ return;
+
+ // We only need to associate the first local declaration of the
+ // specialization. The other declarations will get pulled in by it.
+ if (getFirstLocalDecl(D) != D)
+ return;
+
+ DeclUpdates[TD].push_back(DeclUpdate(
+ UPD_CXX_ADDED_TEMPLATE_SPECIALIZATION, D));
+}
+
void ASTWriter::ResolvedExceptionSpec(const FunctionDecl *FD) {
assert(!DoneWritingDeclsAndTypes && "Already done writing updates!");
if (!Chain) return;
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
--
1.9.1
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
