[PATCH][modules][PR26237]
Vassil Vassilev via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 24 14:01:05 PST 2016
On 24/02/16 22:50, Richard Smith wrote:
> On Wed, Feb 24, 2016 at 8:10 AM, Vassil Vassilev <v.g.vassilev at 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.vassilev at 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.vassilev at 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.vassilev at 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 at 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 at lists.llvm.org
>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>>
>>>>
-------------- next part --------------
From 22c67c14843f119f0cdb64c3fa0ab3ef272f7b5f Mon Sep 17 00:00:00 2001
From: Vassil Vassilev <v.g.vassilev at 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)
More information about the cfe-commits
mailing list