[PATCH][modules][PR26237]

[Richard Smith via cfe-commits]

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.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
>>>>
>>>
>>
>
>
>