[LLVMdev] Addressing const reference in ArrayRef

[Justin Bogner]

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