On 24/02/16 22:50, Richard Smith wrote:
On Wed, Feb 24, 2016 at 8:10 AM, Vassil Vassilev <v.g.vassi...@gmail.com> wrote:
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.
+ // *Specializations iterators. Force the deserialization in advance.
Drop the second sentence of this comment, since it's no longer accurate.
oops... thanks!
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?
Yes, please see the instructions here:
http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access
Will do. Could you check this patch in meanwhile, please?
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 22c67c14843f119f0cdb64c3fa0ab3ef272f7b5f 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..6ff3f95 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.
+ 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
