[LLVMdev] Addressing const reference in ArrayRef

[David Blaikie]

On Wed, Aug 20, 2014 at 8:33 PM, Joey Ye <joey.ye.cc at gmail.com> wrote:
> Just applied and it didn't work. I'm fresh to LLVM so not sure if it
> is built correct.

Did it build successfully without changes?

> Anyway, if the issue is known I'll leave people from
> this community to solve it.

Sorry I've seemed a bit blunt, I didn't mean to discourage you from
engaging in the community. It's a useful thing to consider, it's just
difficult rehashing previous conversations without the context there.

I assume the original bug(s) have been fixed? The question is just how
to avoid introducing more instances of this bug?

- David

>
> Thanks,
> Joey
>
> On Wed, Aug 20, 2014 at 11:28 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> 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.
>>
>> 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