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.
You also have some tabs in your patch (on the Specs.push_back lines);
please fix those prior to commit. With those changes, this LGTM.
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
>>>>
>>>
>>
>
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
