[cfe-dev] Smart Pointer Lifetime Optimizations

[James Y Knight via cfe-dev]

On Wed, Jun 10, 2020 at 2:32 PM Richard Smith via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> On Mon, 8 Jun 2020 at 19:52, John McCall via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> On 8 Jun 2020, at 21:13, Richard Smith wrote:
>>
>> On Mon, 8 Jun 2020 at 00:22, John McCall via cfe-dev <
>> cfe-dev at lists.llvm.org>
>> wrote:
>>
>> You wouldn’t be the first person to be surprised by the result of this
>> sort
>> of analysis, but I’m afraid it’s what we’re working with.
>>
>> Unfortunately, there’s really no way to eliminate this one without either
>> interprocedural information or language changes. trivial_abi eliminates
>> the other one because it changes the convention for passing by value, but
>> to
>> pass an “immutably borrowed” value in C++ we have to pass by reference,
>> which
>> allows the reference to be escaped and accessed (and even mutated, if the
>> original object wasn’t declared const) as long as those accesses happen
>> before destruction.
>>
>> Perhaps we should expose LLVM's nocapture attribute to the source level?
>>
>> I think we have with __attribute__((noescape)). Of course, adopting it
>> systematically would be hugely invasive.
>>
>> *> Probably more importantly, though, we could allow unstable-ness to be
>> detected with a type trait, and that would allow the standard library to
>> take advantage of it. *
>>
>>
>> We could actually do this for trivial_abi types too. If we added a builtin
>> type trait to check if a type has the trivial_abi attribute, libc++ could
>> conditionally give unique_ptr the trivial_abi attribute if its base type
>> also had the attribute. Additionally, we could add a config macro that
>> would do this globally when libc++ is in unstable ABI mode.
>>
>> Hmm. That doesn’t just fall out from any analysis I see. trivial_abi
>> is an existing, ABI-stable attribute, so changing the ABI of
>> std::unique_ptr
>> for types that are already trivial_abi is just as much of an ABI break
>> as changing it in general would be. You could try to justify it by saying
>> that there just aren’t very many trivial_abi types yet, or that
>> trivial_abi
>> is a vendor-specific attribute that’s unlikely to be used on a type with a
>> stable ABI because non-Clang implementations wouldn’t be able to compile
>> it compatibly, but those aren’t terribly convincing arguments to me.
>>
>> I guess I should finish https://reviews.llvm.org/D63748 at some point.
>> (Though I think we probably shouldn't enable it in libc++ unstable ABI
>> configurations by default, since it also changes observable program
>> semantics due to altering destruction order, and is arguably
>> non-conforming
>> for the same reason.)
>>
>> It definitely changes observable semantics, but it’s not *obviously*
>> non-conforming; [expr.call]p7 gives us a lot of flexibility here:
>>
>> It is implementation-defined whether the lifetime of a parameter
>> ends when the function in which it is defined returns or at the
>> end of the enclosing full-expression.
>>
> This is the non-conformance I'm referring to: https://godbolt.org/z/cgf5_2
>
>
> Even given [expr.call]p7, we are still required to destroy
> automatic-storage-duration objects in reverse construction order by
> [stmt.jump]p2:
>
> "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."
>

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 (https://godbolt.org/z/DKLqE4) will make
this a bit more obvious. You will see the following:
1. A addr1
2. B addr2
3. C addr3
4. ~B addr4
5. ~C addr3
6. ~A addr1

Note that what is "wrong" here is *not* 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.

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.

A couple weeks ago, I'd convinced myself that the trivial_abi behavior was
therefore conforming.

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 *must* now be created in place in the argument. There is an
existing exception to that requirement, in [class.temporary#3]
<http://wg21.link/class.temporary#3> 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.

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 *when* 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200614/7ebe47b9/attachment-0001.html>