[PATCH][modules][PR26237]

[Richard Smith via cfe-commits]

On Wed, Feb 24, 2016 at 2:01 PM, Vassil Vassilev <v.g.vassilev at gmail.com> wrote:
> 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?

r261781. Thanks!

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