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

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

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>

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>