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

[Keane, Erich via llvm-dev]

Ok, thanks for the guidance.  I’ll submit a review this afternoon to fix the test case.  I spent most of the morning to try to find some violations of lifetimes around the code, but didn’t see any smoking guns.  For the most part, ArrayRef is either used properly, or the issue is deep enough call-wise I didn’t see it easily enough.

From: David Blaikie [mailto:dblaikie at gmail.com]
Sent: Thursday, August 25, 2016 11:54 AM
To: Keane, Erich <erich.keane at intel.com>; mehdi.amini at apple.com
Cc: llvm-dev at lists.llvm.org
Subject: Re: [llvm-dev] Pointer to temporary issue in ArrayRefTest.InitializerList

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<mailto: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> [mailto: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<mailto:erich.keane at intel.com>>
Cc: llvm-dev at lists.llvm.org<mailto: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<mailto: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<mailto: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/845ca973/attachment.html>