On 24/02/16 02:05, Richard Smith wrote:
Calling getMostRecentDecl seems like a slightly fragile way to avoid
iterator invalidation here. Instead...
@@ -214,6 +212,19 @@ namespace clang {
unsigned I = Record.size();
Record.push_back(0);
+ auto &Specializations = Common->Specializations;
+ auto &&PartialSpecializations = getPartialSpecializations(Common);
+
+ // AddFirstDeclFromEachModule might trigger deserialization, invalidating
+ // *Specializations iterators. Force the deserialization in advance.
+ llvm::SmallVector<const Decl*, 16> Specs;
+ for (auto &Entry : Specializations)
+ Specs.push_back(getSpecializationDecl(Entry));
+ for (auto &Entry : PartialSpecializations)
+ Specs.push_back(getSpecializationDecl(Entry));
+ for (auto *D : Specs)
+ D->getMostRecentDecl();
... delete these two lines, and...
+
for (auto &Entry : Specializations) {
... iterate over "Specs" here....
auto *D = getSpecializationDecl(Entry);
... and you don't need this line any more.
assert(D->isCanonicalDecl() && "non-canonical decl in set");
You can then remove the following, identical, PartialSpecializations loop.
Done.
You also have some tabs in your patch (on the Specs.push_back lines);
Sorry about the tabs, they came from a brand new VM and unconfigured emacs.
please fix those prior to commit. With those changes, this LGTM.
Would it make sense to ask for commit perms?
On Mon, Feb 22, 2016 at 7:11 AM, Vassil Vassilev <v.g.vassi...@gmail.com> wrote:
ping...
On 30/01/16 21:13, Vassil Vassilev wrote:
On 30/01/16 18:36, David Blaikie wrote:
On Sat, Jan 30, 2016 at 9:00 AM, Vassil Vassilev <v.g.vassi...@gmail.com>
wrote:
AFAICT the making a test case independent on STL is the hard part. I think
it will be always failing due to the relocation of the memory region of the
underlying SmallVector.
Sorry, I think I didn't explain myself well. What I mean is - due to the
instability of test cases for UB (especially library UB), we don't usually
commit test cases for them - we just fix them without tests. About the only
time I think committing a test is helpful for UB (especially library UB) is
if we have a reliable way to test/catch the UB (eg: the sanitizers or a
checking STL that fails fast on validation violations). So, while it would
be good to provide/demonstrate your test case, I would not commit the test
case unless it's a minimal reproduction in the presence of one of those
tools (sanitizers or checking STL) - and would just commit the fix without a
test case, usually.
(I'm not actually reviewing this patch - I don't know much about the modules
code, but just providing a bit of context and first-pass-review)
Got it. Thanks!
--Vassil
On 30/01/16 17:37, David Blaikie wrote:
Yeah, it's good to have a reproduction, but I wouldn't actually commit one
- unless you have a checked stl to test against that always fails on
possibly-invalidating sequences of operations, then you'd have a fairly
simple reproduction
On Jan 30, 2016 8:23 AM, "Vassil Vassilev" <v.g.vassi...@gmail.com> wrote:
Sorry.
Our module builds choke on merging several modules, containing
declarations from STL (we are using libc++, no modulemaps).
When writing a new module containing the definition of a STL
reverse_iterator, it collects all its template specializations. Some of the
specializations need to be deserialized from a third module. This triggers
an iterator invalidation bug because of the deserialization happening when
iterating the specializations container itself. This happens only when the
container's capacity is exceeded and the relocation invalidates the
iterators and at best causes a crash (see valgrind reports in the bug
report). Unfortunately I haven't been able to make the reproducer
independent on STL.
--Vassil
On 30/01/16 17:08, David Blaikie wrote:
It might be handy to give an overview of the issue in the review (&
certainly in the commit) message rather than only citing the bug number
On Jan 30, 2016 6:49 AM, "Vassil Vassilev via cfe-commits"
<cfe-commits@lists.llvm.org> wrote:
Attaching a fix to https://llvm.org/bugs/show_bug.cgi?id=26237
Please review.
Many thanks!
--Vassil
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
From a90e8ec1bd8eeb7d35e43d29a2aab47fc444e12e Mon Sep 17 00:00:00 2001
From: Vassil Vassilev <v.g.vassi...@gmail.com>
Date: Sat, 30 Jan 2016 14:50:06 +0100
Subject: [PATCH] Writing out template specializations might trigger
deserialization.
Rebuilding a redecl chain of template (partial) specialization might be trigger
deserialization. If the container's capacity is exceeded the relocation
invalidates the iterators and at best causes a crash. Force deserialization by
copying the collections before iterating.
Fixes PR26237 (https://llvm.org/bugs/show_bug.cgi?id=26237)
I haven't been successful in reducing a reasonable testcase. It should contain
multimodule setup and at least 8 specializations being deserializaed in a very
specific way.
---
lib/Serialization/ASTWriterDecl.cpp | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/lib/Serialization/ASTWriterDecl.cpp
b/lib/Serialization/ASTWriterDecl.cpp
index f2f4a02..7e75a29 100644
--- a/lib/Serialization/ASTWriterDecl.cpp
+++ b/lib/Serialization/ASTWriterDecl.cpp
@@ -192,8 +192,8 @@ namespace clang {
return None;
}
- template<typename Decl>
- void AddTemplateSpecializations(Decl *D) {
+ template<typename DeclTy>
+ void AddTemplateSpecializations(DeclTy *D) {
auto *Common = D->getCommonPtr();
// If we have any lazy specializations, and the external AST source is
@@ -205,8 +205,6 @@ namespace clang {
assert(!Common->LazySpecializations);
}
- auto &Specializations = Common->Specializations;
- auto &&PartialSpecializations = getPartialSpecializations(Common);
ArrayRef<DeclID> LazySpecializations;
if (auto *LS = Common->LazySpecializations)
LazySpecializations = llvm::makeArrayRef(LS + 1, LS[0]);
@@ -215,13 +213,15 @@ namespace clang {
unsigned I = Record.size();
Record.push_back(0);
- for (auto &Entry : Specializations) {
- auto *D = getSpecializationDecl(Entry);
- assert(D->isCanonicalDecl() && "non-canonical decl in set");
- AddFirstDeclFromEachModule(D, /*IncludeLocal*/true);
- }
- for (auto &Entry : PartialSpecializations) {
- auto *D = getSpecializationDecl(Entry);
+ // AddFirstDeclFromEachModule might trigger deserialization, invalidating
+ // *Specializations iterators. Force the deserialization in advance.
+ llvm::SmallVector<const Decl*, 16> Specs;
+ for (auto &Entry : Common->Specializations)
+ Specs.push_back(getSpecializationDecl(Entry));
+ for (auto &Entry : getPartialSpecializations(Common))
+ Specs.push_back(getSpecializationDecl(Entry));
+
+ for (auto *D : Specs) {
assert(D->isCanonicalDecl() && "non-canonical decl in set");
AddFirstDeclFromEachModule(D, /*IncludeLocal*/true);
}
--
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
