[llvm-dev] Pointer to temporary issue in ArrayRefTest.InitializerList

[David Blaikie via llvm-dev]

Yeah, this general issue comes up across the board. Generally it's "don't
do that" and we could add some clang-tidy checks to help catch these cases
(perhaps even powered by an attribute on the type so it's extensible).

which is to say: fix the test case, it's buggy, but that's about it

On Wed, Aug 24, 2016 at 11:46 AM Keane, Erich via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

> Sorry for the inline-comment format being weird, I haven't figured out yet
> how to do '>' stuff in outlook yet :/  Hopefully this is clear enough.
>
> -----Original Message-----
> From: mehdi.amini at apple.com [mailto:mehdi.amini at apple.com]
> Sent: Wednesday, August 24, 2016 10:55 AM
> To: Keane, Erich <erich.keane at intel.com>
> Cc: llvm-dev at lists.llvm.org
> Subject: Re: [llvm-dev] Pointer to temporary issue in
> ArrayRefTest.InitializerList
>
>
> > On Aug 24, 2016, at 10:48 AM, Keane, Erich via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> >
> > Hi all-
> > I am mostly doing work in Clang (and am new there), so I apologize if
> this isn't the proper place to mention this.  I appreciate guidance in
> advance.
> >
> > I was looking into some of the unit tests, and noticed that the
> ArrayRefTest.InitializerList, and thus the InitializerList constructor of
> ArrayRef (under normal use-case) hit undefined behavior.  The test does the
> following:
> >  ArrayRef<int> A = { 0, 1, 2, 3, 4 };
> >  for (int i = 0; i < 5; ++i)
> >    EXPECT_EQ(i, A[i]);
> >
> > For those unfamiliar, ArrayRef is a T* Data/size_t Length pair-type with
> a std::initializer_list Ctor that simply copies the initializer_list::begin
> into Data.
> >
> > The issue is that after the assignment, the initializer-list temporary
> goes out of scope (since it is a temporary), creating a dangling pointer.
> This doesn't seem to be an issue for the most part, however compiling the
> test with -O2 and -fno-merge-all-constants causes this test to fail.
> >
> > I suspect that this should be fixed in 1 of the following ways.  I'm
> willing to contribute the patch, but would like some guidance as to which
> the community thinks is the proper solution.
> >
> > 1- "Delete" r-value ctors for ArrayRef.  I did a quick test just
> deleting r-value std;:initializer list, and discovered quite a few usages
> of construct-from-temporary (before the build gave up!) that would need to
> be fixed as well.
>
> How would we do with ArrayRef as function argument, built from an R-value?
> Sounds like a valid use-case to me.
> [Keane, Erich] Huh, I hadn't thought of that, but that definitely explains
> why the GSL version of "span" doesn't delete them.  Also explains why the
> array_view paper doesn't mention this as well.  I guess it is up to the
> user to beware of this case.    Perhaps the solution is to just audit our
> usages and see where we've messed up.
>
> > 2- Implement the r-value ctors to allocate.  This is likely going to
> require an additional member to capture the fact that this was allocated
> and thus needs to be free'd.  I suspect that this violates the purpose of
> the ArrayRef.
>
> Right.
>
> Note that I believe the same issue exists with Twine and StringRef.
> [Keane, Erich] Interesting... I'm surprised to see that StringRef isn't
> implemented in terms of ArrayRef (with inheritance).
>
> —
> Mehdi
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160825/fce0207d/attachment.html>