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 > >
Is there some way we can get lifetime extension of temporaries to kick in here? 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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140821/7f94e7fc/attachment.html>
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 > >