[PATCH][modules][PR26237]

[Richard Smith via cfe-commits]

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.

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

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