<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Juneyong,</p>
<p>Sorry for not replying sooner. Point 3 came up in the context of
<a class="moz-txt-link-freetext" href="https://reviews.llvm.org/D99135">https://reviews.llvm.org/D99135</a>. I need to ask that you revert
that particular wording. More discussion is needed, and the
current semantics in the LangRef is problematic for
dereferenceability. (Both current and proposed semantics.)</p>
<p>Some detailed comments follow, but please don't get lost in the
discussion. The primary point of this email is the request in the
previous paragraph. <br>
</p>
<p>Philip<br>
</p>
<div class="moz-cite-prefix">On 3/4/21 7:46 PM, Juneyoung Lee wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAJOWn3gkbY=BCPJB25x2OgYWErOEJQsLR3J39zRJ-kzPrmSi-A@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div dir="ltr">
<div>Hello Philip,</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Fri, Mar 5, 2021 at
2:48 AM Philip Reames <<a
href="mailto:listmail@philipreames.com"
moz-do-not-send="true">listmail@philipreames.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">I
see a couple of problems with the drafting on this that I
think <br>
warrant tweaking.<br>
<br>
1) The original wording for lifetime.start and
lifetime.end applied <br>
equally to all memory objects, not just stack objects. I
think it's a <br>
mistake to weaken them so much for non-stack objects.
Glancing through <br>
the optimizer, I can't find a case where this actively
causes a problem, <br>
but the lack of symmetry in the definition seems
undesirable.<br>
</blockquote>
<div><br>
</div>
<div>There were discussions about allowing non-stack objects
in llvm-dev's lifetime thread and the old lifetime patch</div>
<div>(the threads are super long), and two issues were
found:</div>
<div><br>
</div>
<div>1. AddressSanitizer may turn allocas into special
allocator function calls without dropping their lifetime
uses.</div>
<div>2. Since LangRef had been allowing lifetime with
non-stack objects, changing it can turn programs that use</div>
<div>lifetime with non-stack objects into an invalid
program.</div>
</div>
</div>
</div>
</blockquote>
Why is this anything other than a bug in asan? <br>
<blockquote type="cite"
cite="mid:CAJOWn3gkbY=BCPJB25x2OgYWErOEJQsLR3J39zRJ-kzPrmSi-A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>To resolve 1, simply dropping the lifetime would work;
for 2, we asked llvm-dev whether there is any such program
language</div>
<div>that needs the feature, but didn't get any reply.</div>
</div>
</div>
</div>
</blockquote>
I agree, I think the current semantics of lifetime end are badly
specified and a result, largely unusable by any frontend. <br>
<blockquote type="cite"
cite="mid:CAJOWn3gkbY=BCPJB25x2OgYWErOEJQsLR3J39zRJ-kzPrmSi-A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div>But, we chose to be conservative here because this
direction still requires changes in the implementations
(either in LLVM</div>
<div>or frontends that we're not aware of).</div>
</div>
</div>
</div>
</blockquote>
We also need to be considerate of the "defacto" semantics of the
intrinsics. We've had an in tree implementation for years. What
the optimizer has done, is, defacto, the actual semantics. (See
point below for further expansion on this.)<br>
<blockquote type="cite"
cite="mid:CAJOWn3gkbY=BCPJB25x2OgYWErOEJQsLR3J39zRJ-kzPrmSi-A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div>Instead, the semantics w.r.t non-stack object is
clarified so that the object explicitly contains poison
bytes.</div>
<div><br>
</div>
<div>It would be great if something that definitely
requires excluding non-stack object lifetime uses is
reported; this will allow us to make</div>
<div>a further progress.</div>
<div> <br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">2)
In the wording for lifetime.start, "If the offset is
non-zero" is <br>
very unclear as to meaning. I think you mean that the
pointer passed to <br>
the object is not the base of the memory object. At a
minimum, this <br>
should be clarified. I think we can also make this case
immediate UB, <br>
but am not going to argue strongly for it.<br>
</blockquote>
<div><br>
</div>
<div>Thanks for the feedback, I'll clarify it. :)</div>
<div>I think lifetime.start/end shouldn't raise immediate UB
because it is common for the intrinsic calls to be</div>
<div>code-motioned.</div>
</div>
</div>
</div>
</blockquote>
Thanks for clarifying. The new wording is more clear. <br>
<blockquote type="cite"
cite="mid:CAJOWn3gkbY=BCPJB25x2OgYWErOEJQsLR3J39zRJ-kzPrmSi-A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">3)
The meaning of "dead" changed in this revision in a
problematic way. <br>
The original wording said that loads from dead objects
were undef. The <br>
new wording says that accessing a dead object is immediate
UB. From the <br>
commit comment, this doesn't appear to have been an
intentional change. <br>
I ask that this be reversed and reviewed separately if
actually <br>
desired. If nothing else, there's a backwards
compatibility concern here.<br>
</blockquote>
<div><br>
</div>
<div>The change was intentional because the comment at
StackColoring.cpp says</div>
<div>memory accesses to out-of-scope stack slots are UB.</div>
<div>Also, the original wording was unclear as well - it
says loads from dead objects can be *optimized to*</div>
<div>undef, implying it could be immediate UB. The new
wording is a clarification for this.</div>
<div><br>
</div>
<div>I think this change based on StackColoring.cpp's
comment is okay: it was the only place where</div>
<div>the meaning of lifetime was clearly mentioned, and also
found that frontend language developers</div>
<div>were also relying on the comment.</div>
</div>
</div>
</div>
</blockquote>
<p>I went and I read the StackColoring.cpp comments. I see where
you're coming from, but I think the definition used there is
problematic. I also suspect you're reading more specificity into
the wording than was intended by the patch author. :)</p>
<p>At a macro level, you need to consider and balance the
implementation of more than StackColoring. As an example,
consider LICM. Today, LICM does not consider lifetime end to
effect the speculation safety of reordering a load. Consider this
example:</p>
<p>(assume obj is an alloca)<br>
if (c) { lifetime_end(obj) }<br>
loop {<br>
if (!c) <br>
v = obj->f;<br>
.. body .. <br>
}<br>
if (!c) { lifetime_end(obj) }<br>
</p>
<p>LICM will convert this to:<br>
</p>
<p>if (c) { lifetime_end(obj) }<br>
v_1 = obj->f;<br>
loop {<br>
if (!c) <br>
v = v_1;<br>
.. body .. <br>
}<br>
if (!c) { lifetime_end(obj) }</p>
<p>This is a desirable transform. We do not wish to disallow this.<br>
</p>
<p>With the semantics you added to the LangRef, this is now
introducing UB into the program.</p>
<p>Please revert that particular sentence. The rest of your change
can stand, but that point is actively problematic and needs
removed. <br>
</p>
<p><br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CAJOWn3gkbY=BCPJB25x2OgYWErOEJQsLR3J39zRJ-kzPrmSi-A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div>But if it still requires further discussion, I'll
temporarily revert relevant sentences and open a
Phabricator patch for this.</div>
<div> </div>
<div>Juneyoung</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">On
3/3/21 4:58 PM, Juneyoung Lee via llvm-commits wrote:<br>
> Author: Juneyoung Lee<br>
> Date: 2021-03-04T09:58:06+09:00<br>
> New Revision:
c821ef451373849c403686042466d30a7bf8138a<br>
><br>
> URL: <a
href="https://github.com/llvm/llvm-project/commit/c821ef451373849c403686042466d30a7bf8138a"
rel="noreferrer" target="_blank" moz-do-not-send="true">https://github.com/llvm/llvm-project/commit/c821ef451373849c403686042466d30a7bf8138a</a><br>
> DIFF: <a
href="https://github.com/llvm/llvm-project/commit/c821ef451373849c403686042466d30a7bf8138a.diff"
rel="noreferrer" target="_blank" moz-do-not-send="true">https://github.com/llvm/llvm-project/commit/c821ef451373849c403686042466d30a7bf8138a.diff</a><br>
><br>
> LOG: [LangRef] Make lifetime intrinsic's semantics
consistent with StackColoring's comment<br>
><br>
> This patch is an update to LangRef by describing
lifetime intrinsics' behavior<br>
> by following the description of MIR's
LIFETIME_START/LIFETIME_END markers<br>
> at StackColoring.cpp (<a
href="https://github.com/llvm/llvm-project/blob/eb44682d671d66e422b02595a636050582a4d84a/llvm/lib/CodeGen/StackColoring.cpp#L163"
rel="noreferrer" target="_blank" moz-do-not-send="true">https://github.com/llvm/llvm-project/blob/eb44682d671d66e422b02595a636050582a4d84a/llvm/lib/CodeGen/StackColoring.cpp#L163</a>)
and the discussion in llvm-dev.<br>
><br>
> In order to explicitly define the meaning of an
object lifetime, I added 'Object Lifetime' subsection.<br>
><br>
> Reviewed By: nlopes<br>
><br>
> Differential Revision: <a
href="https://reviews.llvm.org/D94002" rel="noreferrer"
target="_blank" moz-do-not-send="true">https://reviews.llvm.org/D94002</a><br>
><br>
> Added:<br>
> <br>
><br>
> Modified:<br>
> llvm/docs/LangRef.rst<br>
><br>
> Removed:<br>
> <br>
><br>
><br>
>
################################################################################<br>
> diff --git a/llvm/docs/LangRef.rst
b/llvm/docs/LangRef.rst<br>
> index cc2fcef30831..6866cc362f57 100644<br>
> --- a/llvm/docs/LangRef.rst<br>
> +++ b/llvm/docs/LangRef.rst<br>
> @@ -2544,6 +2544,33 @@ This information is passed
along to the backend so that it generates<br>
> code for the proper architecture. It's possible to
override this on the<br>
> command line with the ``-mtriple`` command line
option.<br>
> <br>
> +.. _objectlifetime:<br>
> +<br>
> +Object Lifetime<br>
> +----------------------<br>
> +<br>
> +A memory object, or simply object, is a region of a
memory space that is<br>
> +reserved by a memory allocation such as :ref:`alloca
<i_alloca>`, heap<br>
> +allocation calls, and global variable definitions.<br>
> +Once it is allocated, the bytes stored in the region
can only be read or written<br>
> +through a pointer that is :ref:`based on
<_pointeraliasing>` the allocation<br>
> +value.<br>
> +If a pointer that is not based on the object tries
to read or write to the<br>
> +object, it is undefined behavior.<br>
> +<br>
> +A lifetime of a memory object is a property that
decides its accessibility.<br>
> +Unless stated otherwise, a memory object is alive
since its allocation, and<br>
> +dead after its deallocation.<br>
> +It is undefined behavior to access a memory object
that isn't alive, but<br>
> +operations that don't dereference it such as<br>
> +:ref:`getelementptr <i_getelementptr>`,
:ref:`ptrtoint <i_ptrtoint>` and<br>
> +:ref:`icmp <i_icmp>` return a valid result.<br>
> +This explains code motion of these instructions
across operations that<br>
> +impact the object's lifetime.<br>
> +A stack object's lifetime can be explicitly
specified using<br>
> +:ref:`llvm.lifetime.start <_int_lifestart>`
and<br>
> +:ref:`llvm.lifetime.end <_int_lifeend>`
intrinsic function calls.<br>
> +<br>
> .. _pointeraliasing:<br>
> <br>
> Pointer Aliasing Rules<br>
> @@ -17808,8 +17835,9 @@ Other targets may support
this intrinsic<br>
> diff erently, for example, by lowering i<br>
> Memory Use Markers<br>
> ------------------<br>
> <br>
> -This class of intrinsics provides information about
the lifetime of<br>
> -memory objects and ranges where variables are
immutable.<br>
> +This class of intrinsics provides information about
the<br>
> +:ref:`lifetime of memory objects
<_objectlifetime>` and ranges where variables<br>
> +are immutable.<br>
> <br>
> .. _int_lifestart:<br>
> <br>
> @@ -17826,8 +17854,8 @@ Syntax:<br>
> Overview:<br>
> """""""""<br>
> <br>
> -The '``llvm.lifetime.start``' intrinsic specifies
the start of a memory<br>
> -object's lifetime.<br>
> +The '``llvm.lifetime.start``' intrinsic specifies
the start of<br>
> +:ref:`a memory object's lifetime
<_objectlifetime>`.<br>
> <br>
> Arguments:<br>
> """"""""""<br>
> @@ -17839,10 +17867,23 @@ to the object.<br>
> Semantics:<br>
> """"""""""<br>
> <br>
> -This intrinsic indicates that before this point in
the code, the value<br>
> -of the memory pointed to by ``ptr`` is dead. This
means that it is known<br>
> -to never be used and has an undefined value. A load
from the pointer<br>
> -that precedes this intrinsic can be replaced with
``'undef'``.<br>
> +If ``ptr`` is a stack-allocated object and its
offset is zero, the object is<br>
> +initially marked as dead.<br>
> +After '``llvm.lifetime.start``', the stack object
that ``ptr`` points is marked<br>
> +as alive and has an uninitialized value.<br>
> +The stack object is marked as dead when either<br>
> +:ref:`llvm.lifetime.end <int_lifeend>` to the
alloca is executed or the<br>
> +function returns.<br>
> +<br>
> +After :ref:`llvm.lifetime.end <int_lifeend>`
is called,<br>
> +'``llvm.lifetime.start``' on the stack object can be
called again.<br>
> +The second '``llvm.lifetime.start``' call marks the
object as alive, but it<br>
> +does not change the address of the object.<br>
> +<br>
> +If ``ptr`` is a non-stack-allocated object, its
offset is non-zero or it is<br>
> +a stack object that is already alive, it simply
fills all bytes of the object<br>
> +with ``poison``.<br>
> +<br>
> <br>
> .. _int_lifeend:<br>
> <br>
> @@ -17859,8 +17900,8 @@ Syntax:<br>
> Overview:<br>
> """""""""<br>
> <br>
> -The '``llvm.lifetime.end``' intrinsic specifies the
end of a memory<br>
> -object's lifetime.<br>
> +The '``llvm.lifetime.end``' intrinsic specifies the
end of<br>
> +:ref:`a memory object's lifetime
<_objectlifetime>`.<br>
> <br>
> Arguments:<br>
> """"""""""<br>
> @@ -17872,10 +17913,13 @@ to the object.<br>
> Semantics:<br>
> """"""""""<br>
> <br>
> -This intrinsic indicates that after this point in
the code, the value of<br>
> -the memory pointed to by ``ptr`` is dead. This means
that it is known to<br>
> -never be used and has an undefined value. Any stores
into the memory<br>
> -object following this intrinsic may be removed as
dead.<br>
> +If ``ptr`` is a stack-allocated object and its
offset is zero, the object is<br>
> +dead.<br>
> +Calling ``llvm.lifetime.end`` on an already dead
alloca is no-op.<br>
> +<br>
> +If ``ptr`` is a non-stack-allocated object or its
offset is non-zero,<br>
> +it is equivalent to simply filling all bytes of the
object with ``poison``.<br>
> +<br>
> <br>
> '``llvm.invariant.start``' Intrinsic<br>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^<br>
><br>
><br>
> <br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org"
target="_blank" moz-do-not-send="true">llvm-commits@lists.llvm.org</a><br>
> <a
href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits"
rel="noreferrer" target="_blank" moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
</div>
</div>
</blockquote>
</body>
</html>