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