[PATCH][modules][PR26237]

Wed Feb 24 14:05:27 PST 2016

On 24/02/16 23:03, Richard Smith wrote:
> 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!
Thanks a lot!
>
>>>>> 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
>>>>>>>>>