llvm dev - Aug 2016 - Pointer to temporary issue in ArrayRefTest.InitializerList

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.
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.
3- Others?  

Thanks,
Erich Keane

> 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.
> 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.

— 
Mehdi

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