[PATCH][modules][PR26237]

[Vassil Vassilev via cfe-commits]

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 <mailto: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 <mailto: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
>>>         <mailto: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
>>>             <mailto:cfe-commits at lists.llvm.org>
>>>             http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160130/8238af6d/attachment.html>