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.

[Aaron Ballman]

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.  :-/

~Aaron