[PATCH][modules][PR26237]

[David Blaikie via cfe-commits]

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)


>
> 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
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160130/81d4352e/attachment.html>