[Lifetime, Temporary] Generate ExprWithCleanups for trivially destructible tempoararies

Clang Frontend
1

Hi, I’m working on bug 27451 (https://llvm.org/bugs/show_bug.cgi?id=27451).

If the lifetime-extended temporary object is not trivially destructible, there will be a ExprWithCleanups generated for the VarDecl, so calling CodeGenFunction::pushCleanupAfterFullExpr to push a llvm.lifetime.end() call as a cleanup (as clang currently does in non-temporary cases) just works.

However, I learned that ExprWithCleanups will not be generated by Sema::MaybeBindToTemporary, if the destructor is trivial, according to <https://github.com/llvm-mirror/clang/blob/4a65931dcba82b23856d654eb77d133d0a3c59f2/lib/Sema/SemaExprCXX.cpp#L5593>; and pushCleanupAfterFullExpr seems not working well without a ExprWithCleanups, due to the absence of RunCleanupsScope <https://github.com/llvm-mirror/clang/blob/717e5e20622b0f4ac3c16bb643ed6a36eef29603/lib/CodeGen/CGExpr.cpp#L1002>.

If there is a ExprWithCleanups generated even for a trivially destructible temporary, generating llvm.lifetime.end() is easy by calling CodeGenFunction::pushCleanupAfterFullExpr.

This requires moving L5593 to L5550 in SemaExprCXX.cpp. The two-lines change breaks 36 tests, most of which are checking for warnings, so I suppose I also need to fix some warning detections.

Is this - by adding more ExprWithCleanups and fix tests - the correct way to fix the temporary lifetime bug? I feel weird about the existence of ExprWithCleanups in the first place, but that’s off the topic.

Just for illustration, the attached patch fixes the bug, but doesn’t fix the tests though.

Thanks!

a.diff (5.14 KB)

2

As discussed off the list, ExprWithCleanups should be used for dtors only. So another solution can be creating a new LifetimeEndStack in CodeGenFunction, which is handled separately.

The problem of this solution is that, since LifetimeEnd marks and dtor calls are not in the same stack, they are not emitted interleaved. The result will be like:
call void dtor2();
call void dtor1();
llvm.lifetime.end(t2);
llvm.lifetime.end(t1);

instead of, ideally:

call void dtor2();
llvm.lifetime.end(t2);

call void dtor1();

llvm.lifetime.end(t1);

This approach is good enough to reduce the stack frame size, though.

3

You should be aware of C++ [basic.life]/1:

"The lifetime of an object of type T ends when:
— if T is a class type with a non-trivial destructor (12.4), the destructor call starts, or

— the storage which the object occupies is reused or released."

That is, for temporaries that have a trivial destructor or temporaries of a non-class type, the lifetime extends until the storage goes away (after all relevant destructors have run).

So, given:

struct A { ~A(); };

struct B { ~B(); };

void f(int&&);

( A(), f(0), B() );

… should clean up as follows:

  1. call ~B()
  2. end lifetime of B temporary
  3. call ~A()
  4. end lifetime of A temporary
  5. end lifetime of int temporary

That’s obviously stupid, but that’s what the language rules require today. (I’m trying to get this fixed but have not succeeded yet.) It’s valid for us to do this:

  1. call ~B()
  2. call ~A()
    3-5) end lifetime of temporaries

but not to do this:

4

Does this apply to non-temporaries? For:

struct A { ~A(); };
struct B { ~B(); };
void Bar(int&);
void Foo() {
B b;
int c;
A a;
Bar(c);
}

Today Clang generates:

call void @_ZN1AD1Ev(%struct.A* nonnull %a) #4
call void @llvm.lifetime.end(i64 1, i8* %2) #4
call void @llvm.lifetime.end(i64 4, i8* %1) #4
call void @_ZN1BD1Ev(%struct.B* nonnull %b) #4
call void @llvm.lifetime.end(i64 1, i8* %0) #4

which is exactly what we want to avoid for now.

5

but not to do this:

1) call ~B()
2) end lifetime of B temporary
3) end lifetime of int temporary
4) call ~A()
5) end lifetime of A temporary

Does this apply to non-temporaries?

Yes.

For:

struct A { ~A(); };
struct B { ~B(); };
void Bar(int&);
void Foo() {
  B b;
  int c;
  A a;
  Bar(c);
}

Today Clang generates:
  call void @_ZN1AD1Ev(%struct.A* nonnull %a) #4
  call void @llvm.lifetime.end(i64 1, i8* %2) #4
  call void @llvm.lifetime.end(i64 4, i8* %1) #4
  call void @_ZN1BD1Ev(%struct.B* nonnull %b) #4
  call void @llvm.lifetime.end(i64 1, i8* %0) #4

which is exactly what we want to avoid for now.

Yep, this is technically a bug. (As mentioned, I'm working on getting the
standard fixed to be a bit less ridiculous here. We'll see how that goes.)

6

Even if we change the C++ standard, people will probably still bug us about
C: https://llvm.org/bugs/show_bug.cgi?id=27604

7

It looks like the problem there is that we start the lifetime too late (in
C), not that we end it too early. But I suppose with our stack-based
approach to cleanups, these are closely-related issues :slight_smile: