r203299 - In my tests, I'm finding that declaring iterators in terms of ranges can sometimes have dangerous side-effects where the range temporary is destroyed, taking the underlying iterators out with it.

[David Blaikie]

On Fri, Mar 7, 2014 at 4:18 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Fri, Mar 7, 2014 at 6:35 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> On Fri, Mar 7, 2014 at 2:43 PM, Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>>>
>>> On Fri, Mar 7, 2014 at 5:28 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>> > On Fri, Mar 7, 2014 at 2:17 PM, Aaron Ballman <aaron at aaronballman.com>
>>> > wrote:
>>> >> Author: aaronballman
>>> >> Date: Fri Mar  7 16:17:20 2014
>>> >> New Revision: 203299
>>> >>
>>> >> URL: http://llvm.org/viewvc/llvm-project?rev=203299&view=rev
>>> >> Log:
>>> >> In my tests, I'm finding that declaring iterators in terms of ranges
>>> >> can sometimes have dangerous side-effects where the range temporary is
>>> >> destroyed, taking the underlying iterators out with it.
>>> >
>>> > Um... what? do you have a concrete example because that doesn't sound
>>> > right at all, at least not for iterator_range<T> which just holds
>>> > copies of the iterator - are some of our iterators non-copyable (or
>>> > not safely/sanely copyable in some way)? All the begin/end's return by
>>> > value, so I don't see how they could be related to the copy that
>>> > happens to be in the range temporary...
>>>
>>> I've found this to be the case anywhere the iterator doesn't wind up
>>> being a simple pointer. attributes (which I had to roll back),
>>
>>
>> I thought the problem in the attributes case was that an accidental copy of
>> the collection of attributes was being made? Or does this still break with
>> that fixed?
>
> It was still breaking with that fixed, once upon a time. Now, not as confident.
>
>>
>>> enums
>>> (which I've yet to commit while trying to figure this out). In all
>>> cases, the commonality is that it compiles fine, but exhibits
>>> use-after-free behavior on test cases in some circumstances.
>>>
>>> Regarding the attribute patch (which was committed this morning as
>>> r203236 and reverted as r203237), Takumi found that these specific
>>> changes were causing test cases to fail:
>>> https://gist.github.com/chapuni/9412576
>>>
>>> So I agree, I find is really surprising that it behaves this way, but
>>> prefer to err on the side of caution at least while doing this initial
>>> push. Once things are pretty well range-ified, then I intend to
>>> revisit the problem (unless someone beats me to it).
>>
>>
>> Does this happen with multiple compilers, or is it an MSVC-specific
>> miscompile?
>
> I'm starting to doubt my sanity. I just locally reverted this commit,
> applied my enums patch, and everything passes whereas it was
> consistently failing before.
>
> Let's pretend I've got a goblin (I should probably reboot this
> machine, it's been quite a while and I've been taxing it lately). Do
> you (or David) see any problems with the code as it was written prior
> to this commit? I originally thought the code was fine, and only
> rolled it back to the state it's currently in because of local worries
> that don't seem so valid now. I'd like to go with a second set of eyes
> to reduce any further churn.
>
> Assuming that you and/or David don't see anything wrong with the code
> I reverted, I'll re-revert this as being a valid pattern and chalk
> this up to solar flares or some such.  :-/

Yeah, I certainly can't see any fault with the implementation as it
was before you reverted it - hence my surprise at the revert.

- David