<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">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.<o:p></o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></a></p>
<p class="MsoNormal"><a name="_____replyseparator"></a><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> David Blaikie [mailto:dblaikie@gmail.com]
<br>
<b>Sent:</b> Thursday, August 25, 2016 11:54 AM<br>
<b>To:</b> Keane, Erich <erich.keane@intel.com>; mehdi.amini@apple.com<br>
<b>Cc:</b> llvm-dev@lists.llvm.org<br>
<b>Subject:</b> Re: [llvm-dev] Pointer to temporary issue in ArrayRefTest.InitializerList<o:p></o:p></span></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">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).<br>
<br>
which is to say: fix the test case, it's buggy, but that's about it<o:p></o:p></p>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Wed, Aug 24, 2016 at 11:46 AM Keane, Erich via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">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.<br>
<br>
-----Original Message-----<br>
From: <a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a> [mailto:<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>]<br>
Sent: Wednesday, August 24, 2016 10:55 AM<br>
To: Keane, Erich <<a href="mailto:erich.keane@intel.com" target="_blank">erich.keane@intel.com</a>><br>
Cc: <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
Subject: Re: [llvm-dev] Pointer to temporary issue in ArrayRefTest.InitializerList<br>
<br>
<br>
> On Aug 24, 2016, at 10:48 AM, Keane, Erich via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
><br>
> Hi all-<br>
> 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.<br>
><br>
> 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:<br>
> ArrayRef<int> A = { 0, 1, 2, 3, 4 };<br>
> for (int i = 0; i < 5; ++i)<br>
> EXPECT_EQ(i, A[i]);<br>
><br>
> 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.<br>
><br>
> 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.<br>
><br>
> 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.<br>
><br>
> 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.<br>
<br>
How would we do with ArrayRef as function argument, built from an R-value? Sounds like a valid use-case to me.<br>
[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.<br>
<br>
> 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.<br>
<br>
Right.<br>
<br>
Note that I believe the same issue exists with Twine and StringRef.<br>
[Keane, Erich] Interesting... I'm surprised to see that StringRef isn't implemented in terms of ArrayRef (with inheritance).<br>
<br>
—<br>
Mehdi<br>
<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><o:p></o:p></p>
</blockquote>
</div>
</div>
</body>
</html>