On Wed, Feb 24, 2016 at 2:01 PM, Vassil Vassilev <v.g.vassi...@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.vassi...@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.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