[LLVMdev] Addressing const reference in ArrayRef

[David Blaikie]

On Thu, Aug 21, 2014 at 10:34 AM, Reid Kleckner <rnk at google.com> wrote:
> Is there some way we can get lifetime extension of temporaries to kick in
> here?

Nope - since the temporary is a subexpression - not the thing being declared.

>
>
> On Thu, Aug 21, 2014 at 8:05 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> Yeah - I suspect there are just too many cases where we use this ctor
>> correctly: where the ArrayRef is only a temporary to a function call.
>>
>> Perhaps this is a problem for which the best solution is a clang-tidy
>> checker - though I'm not sure if we have good integration there yet.
>>
>> (& yes, Andreas, that looks like the previous thread - thanks!)
>>
>> On Thu, Aug 21, 2014 at 5:09 AM, Andreas Weber
>> <andreas.c.weber at gmail.com> wrote:
>> > I think David is referring this thread:
>> > http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-July/074909.html
>> >
>> > I also tried building with the suggested patch (replacing/modifying the
>> > existing ctor (to take non-const ref) ) and this indeed results in
>> > plenty of
>> > build errors. I tried fixing them for a few hours but haven't seen light
>> > at
>> > the end of the tunnel.
>> >
>> > Best regards,
>> > Andy
>> >
>> >
>> > 2014-08-21 9:38 GMT+02:00 Justin Bogner <mail at justinbogner.com>:
>> >
>> >> David Blaikie <dblaikie at gmail.com> writes:
>> >> > I seem to recall discussing this before - is there an existing llvm
>> >> > bug filed, another email thread or something (or perhaps it was just
>> >> > an IRC conversation)? It would be good to keep all the discussion
>> >> > together or at least reference the prior (llvm community) discussion.
>> >>
>> >> I'm not sure if it's been discussed before, but it's led to issues as
>> >> recently as a couple of weeks ago:
>> >>
>> >>
>> >>
>> >> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140804/229557.html
>> >>
>> >> I certainly think it's worthwhile to make this API safer, or at least
>> >> to
>> >> document the caveats in how it can safely be used.
>> >>
>> >> > Have you tried applying your suggested patch? I assume you meant to
>> >> > suggest
>> >> > replacing/modifying the existing ctor (to take non-const ref) rather
>> >> > than
>> >> > adding another?
>> >> >
>> >> > I'd assume that causes a lot of build failures as we probably rely on
>> >> > binding
>> >> > temporaries to ArrayRef's in many places correctly (most ArrayRef's
>> >> > are
>> >> > temporaries, not local variables).
>> >> >
>> >> > I think in the previous discussion I suggested we might just want to
>> >> > make a
>> >> > practice of treating named/local (not parameter) ArrayRef's with
>> >> > greater
>> >> > suspicion, the same way we do for twine, but I'm not sure.
>> >> >
>> >> > We could move this ctor into a factory function (and/or just make the
>> >> > CTor
>> >> > private and friend the makeArrayRef helper for this case) to make it
>> >> > more
>> >> > obvious/easier to find bad call sites. But it would be helpful to
>> >> > have
>> >> > the
>> >> > context of the prior discussion to continue from there.
>> >> >
>> >> > On Aug 19, 2014 11:18 PM, "Joey Ye" <joey.ye.cc at gmail.com> wrote:
>> >> >
>> >> >     Analyzing why GCC failed to build LLVM recently, one root cause
>> >> > lies
>> >> >     in definition of ArrayRef:
>> >> >     // ArrayRef.h:
>> >> >     ArrayRef(const T &OneElt) : Data(&OneElt), Length(1) {}
>> >> >
>> >> >     Here address of const reference is taken and stored to an object.
>> >> > It
>> >> >     is believed that live range of const reference is only at the
>> >> > function
>> >> >     call site, escaping of its address to an object with a longer
>> >> > live
>> >> >     range is invalid. Referring to the case and discussion here:
>> >> >     https://gcc.gnu.org/ml/gcc/2014-08/msg00173.html
>> >> >
>> >> >     So I would suggest to fix ArrayRef. Adding a non-const version of
>> >> >     constructor should work, but it still leaves the vulnerability in
>> >> >     const version, which I'd expect on people in the community to
>> >> > work
>> >> > out
>> >> >     a solution.
>> >> >     ArrayRef(T &OneElt) : Data(&OneElt), Length(1) {}
>> >> >
>> >> >     Thanks,
>> >> >     Joey
>> >> >     _______________________________________________
>> >> >     LLVM Developers mailing list
>> >> >     LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> >> >     http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>> >> >
>> >> > _______________________________________________
>> >> > LLVM Developers mailing list
>> >> > LLVMdev at cs.uiuc.edu
>> >> >
>> >> > http://llvm.cs.uiuc.eduhttp://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>> >>
>> >> _______________________________________________
>> >> LLVM Developers mailing list
>> >> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>> >
>> >
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>