<div dir="ltr"><div dir="ltr">On Wed, Jun 10, 2020 at 2:32 PM Richard Smith via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Mon, 8 Jun 2020 at 19:52, John McCall via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><u></u>




<div>
<div style="font-family:sans-serif"><div style="white-space:normal">
<p dir="auto">On 8 Jun 2020, at 21:13, Richard Smith wrote:</p>

</div>
<div style="white-space:normal"><blockquote style="border-left:2px solid rgb(119,119,119);color:rgb(119,119,119);margin:0px 0px 5px;padding-left:5px"><p dir="auto">On Mon, 8 Jun 2020 at 00:22, John McCall via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>><br>
wrote:</p>
<blockquote style="border-left:2px solid rgb(153,153,153);color:rgb(153,153,153);margin:0px 0px 5px;padding-left:5px"><p dir="auto">You wouldn’t be the first person to be surprised by the result of this sort<br>
of analysis, but I’m afraid it’s what we’re working with.<br>
<br>
Unfortunately, there’s really no way to eliminate this one without either<br>
interprocedural information or language changes. trivial_abi eliminates<br>
the other one because it changes the convention for passing by value, but<br>
to<br>
pass an “immutably borrowed” value in C++ we have to pass by reference,<br>
which<br>
allows the reference to be escaped and accessed (and even mutated, if the<br>
original object wasn’t declared const) as long as those accesses happen<br>
before destruction.<br>
</p>
</blockquote><p dir="auto">Perhaps we should expose LLVM's nocapture attribute to the source level?</p>
</blockquote></div>
<div style="white-space:normal">

<p dir="auto">I think we have with <code style="background-color:rgb(247,247,247);border-radius:3px;margin:0px;padding:0px 0.4em" bgcolor="#F7F7F7">__attribute__((noescape))</code>.  Of course, adopting it<br>
systematically would be hugely invasive.</p>

</div>
<div style="white-space:normal"><blockquote style="border-left:2px solid rgb(119,119,119);color:rgb(119,119,119);margin:0px 0px 5px;padding-left:5px"><blockquote style="border-left:2px solid rgb(153,153,153);color:rgb(153,153,153);margin:0px 0px 5px;padding-left:5px"><p dir="auto">*> Probably more importantly, though, we could allow unstable-ness to be<br>
detected with a type trait, and that would allow the standard library to<br>
take advantage of it. *<br>
<br>
<br>
We could actually do this for trivial_abi types too. If we added a builtin<br>
type trait to check if a type has the trivial_abi attribute, libc++ could<br>
conditionally give unique_ptr the trivial_abi attribute if its base type<br>
also had the attribute. Additionally, we could add a config macro that<br>
would do this globally when libc++ is in unstable ABI mode.<br>
<br>
Hmm. That doesn’t just fall out from any analysis I see. trivial_abi<br>
is an existing, ABI-stable attribute, so changing the ABI of<br>
std::unique_ptr<br>
for types that are already trivial_abi is just as much of an ABI break<br>
as changing it in general would be. You could try to justify it by saying<br>
that there just aren’t very many trivial_abi types yet, or that<br>
trivial_abi<br>
is a vendor-specific attribute that’s unlikely to be used on a type with a<br>
stable ABI because non-Clang implementations wouldn’t be able to compile<br>
it compatibly, but those aren’t terribly convincing arguments to me.<br>
</p>
</blockquote><p dir="auto">I guess I should finish <a href="https://reviews.llvm.org/D63748" style="color:rgb(119,119,119)" target="_blank">https://reviews.llvm.org/D63748</a> at some point.<br>
(Though I think we probably shouldn't enable it in libc++ unstable ABI<br>
configurations by default, since it also changes observable program<br>
semantics due to altering destruction order, and is arguably non-conforming<br>
for the same reason.)</p>
</blockquote></div>
<div style="white-space:normal">

<p dir="auto">It definitely changes observable semantics, but it’s not <em>obviously</em><br>
non-conforming; [expr.call]p7 gives us a lot of flexibility here:</p>

<p dir="auto">It is implementation-defined whether the lifetime of a parameter<br>
  ends when the function in which it is defined returns or at the<br>
  end of the enclosing full-expression.</p></div></div></div></blockquote><div>This is the non-conformance I'm referring to: <a href="https://godbolt.org/z/cgf5_2" target="_blank">https://godbolt.org/z/cgf5_2</a> </div><div><br></div><div>Even given [expr.call]p7, we are still required to destroy automatic-storage-duration objects in reverse construction order by [stmt.jump]p2:</div><div><br></div><div>"On exit from a scope (however accomplished), objects with automatic storage duration (6.7.5.3) that have been constructed in that scope are destroyed in the reverse order of their construction."</div></div></div></blockquote><div><br></div><div>Even with trivial_abi, we are still destroying objects in the proper order of construction. Although it may not be immediately clear, modifying the example to print addresses as well (<a href="https://godbolt.org/z/DKLqE4" target="_blank">https://godbolt.org/z/DKLqE4</a>) will make this a bit more obvious. You will see the following:</div><div>1. A addr1<br>2. B addr2<br>3. C addr3<br>4. ~B addr4<br>5. ~C addr3<br>6. ~A addr1<br></div><div><br></div><div>Note that what is "wrong" here is <i>not</i> a misordering of construction vs destruction of objects. Rather, what's weird is that there is a silent move construction of "B addr4" between lines 3 and 4, and we're missing "~B addr2", which should've occurred between lines 5 and 6. But -- omitting move construction and the subsequent destruction of the moved-from value is exactly what the class author asked for by specifying [[clang::trivial_abi]]. This is nonstandard only in a way internal to the type's implementation.</div><div><br></div><div>Until C++17, the program nominally creates a temporary A/B/C objects, then subsequently moves/copies those temporaries into the call parameter. However, the implementation may decide to elide copies, if it wants to. So, in the above example, as far as anyone who can't see the implementation of type B can tell, the implementation simply decided to elide the copies of A and C, but not to elide the move-construction of B. And it decided to (nominally) move-construct B after finishing evaluation of all of the argument expressions. AFAICT, this was all permissible behavior, until C++17.</div><div><br></div><div>A couple weeks ago, I'd convinced myself that the trivial_abi behavior was therefore conforming.</div><div><br></div><div>However, Richard Smith has pointed out to me elsewhere, that as of C++17, we have mandatory copy-elision. It is no longer allowable for the compiler to choose to create the object as a temporary, and then copy/move it to the argument -- it <i>must</i> now be created in place in the argument. There is an existing exception to that requirement, in <a href="http://wg21.link/class.temporary#3">[class.temporary#3]</a> which allow types which have trivial destructors and copy/move constructors to construct extra temporaries -- noting "This latitude is granted to allow objects of class type to be
passed to or returned from functions in registers." That latitude to pass or return in a register is exactly what we want here. So, in C++17, we need to extend this clause to allow the creation of a temporary object for a parameter of a type which has the trivial_abi attribute.</div><div><br></div><div>There's also the issue that we aren't allowed to evaluate arguments interleaved with each-other anymore. The above clause does not make it clear <i>when</i> the temporary is allowed to be constructed -- which doesn't really matter, given the triviality requirements. But, for the trivial_abi attribute, it does matter, because we now can have a destructor. But if the temporary was allowed to be created, nominally, in the callee, then everything would work out.</div><div><br></div></div></div>