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 > >
Duncan P. N. Exon Smith
2014-Aug-22 17:16 UTC
[LLVMdev] Addressing const reference in ArrayRef
> On 2014-Aug-21, at 10:39, David Blaikie <dblaikie at gmail.com> wrote: > > 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. >>>>>>> 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.A variation of these solutions makes sense to me. I strongly dislike this constructor since it leads to other subtle bugs. E.g., suppose you have this API: void foo(ArrayRef<const char *> Strings); and some caller uses `foo("string")`. If you change the API to add an optional `bool`: void foo(ArrayRef<const char *> Strings, bool B = false); void foo(bool B = false); the caller silently switches to `foo(bool("string"))` instead of `foo(ArrayRef<const char *>("string"))`. If we remove the `ArrayRef` constructor AND the helper, and add a new helper like: template <class T> ArrayRef<T> makeTempArrayRef(const T &OneElt) { return ArrayRef<T>(&OneElt, &OneElt + 1); } then both types of bugs will be more rare and obvious. For the OP's case, the name "Temp" makes it more clear that the ArrayRef shouldn't be named. For my case, since the caller is forced to use `foo(makeTempArrayRef("string"))`, it's easier to make safe API changes. This would cause a lot of churn though -- ArrayRefs are *usually* temporaries. Although this makes sense to me, disallowing named ArrayRefs might be more practical.
On Fri, Aug 22, 2014 at 10:16 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:>> On 2014-Aug-21, at 10:39, David Blaikie <dblaikie at gmail.com> wrote: >> >> 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. >> > > >>>>>>> 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. > > A variation of these solutions makes sense to me. I strongly dislike > this constructor since it leads to other subtle bugs. E.g., suppose you > have this API: > > void foo(ArrayRef<const char *> Strings); > > and some caller uses `foo("string")`. > > If you change the API to add an optional `bool`: > > void foo(ArrayRef<const char *> Strings, bool B = false); > void foo(bool B = false); > > the caller silently switches to `foo(bool("string"))` instead of > `foo(ArrayRef<const char *>("string"))`.My perspective on this /\ is that this kind of API refactoring problem is just one instance of a broader problem (replace ArrayRef with int, T*, etc in the above and you're back to square one). We could catch some instances (such as the one you've shown) with a broader -Wliteral-conversion warning (any literal other than "true" or "false" should warn if converted to bool - modulo the usual false positive suppressions... (RHS of an && for assertions, etc)). Of course that would be thwarted by moving the string into a local variable. Nothing perfect here.> If we remove the `ArrayRef` constructor AND the helper, and add a new > helper like: > > template <class T> ArrayRef<T> makeTempArrayRef(const T &OneElt) { > return ArrayRef<T>(&OneElt, &OneElt + 1); > } > > then both types of bugs will be more rare and obvious. > > For the OP's case, the name "Temp" makes it more clear that the ArrayRef > shouldn't be named. For my case, since the caller is forced to use > `foo(makeTempArrayRef("string"))`, it's easier to make safe API changes.Yep - the convenience of one-element->ArrayRef is "cute" at best, I think. Having to wrap it doesn't seem detrimental. Would have to look at some numbers, though (if we could easily gather the number of instances of this - I'm not sure of the easiest way to do that, given the build system likes to stop after it sees an error). I'm not sure if the "Temp" in the name would be a sufficient deterrent, but maybe.> This would cause a lot of churn though -- ArrayRefs are *usually* > temporaries. Although this makes sense to me, disallowing named > ArrayRefs might be more practical.Yeah, I'm not sure how practical that is either - we have (guessing) quite a few named ArrayRefs... but it might be tractible to remove them. Not sure how I feel about that.