[PATCH][modules][PR26237]

[Vassil Vassilev via cfe-commits]

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 <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> 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/20160222/ef2fa424/attachment.html>