This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Emit destructor calls for non-trivial C structs returned by function calls and loaded from volatile objects
ClosedPublic

Authored by ahatanak on Aug 12 2019, 10:14 AM.

Details

Summary

This is the patch I split out of https://reviews.llvm.org/D64464.

The cleanup is pushed in EmitCallExpr and EmitObjCMessageExpr so that the destructor is called to destruct function call and ObjC message returns. I also added test cases for block function calls since the patch in https://reviews.llvm.org/D64464 wasn't handling that case correctly.

rdar://problem/51867864

Diff Detail

Event Timeline

ahatanak created this revision.Aug 12 2019, 10:14 AM
ahatanak updated this revision to Diff 214660.Aug 12 2019, 10:26 AM

Revert changes I made to llvm that are unrelated to this patch.

rjmccall added inline comments.Aug 13 2019, 4:10 PM
clang/lib/CodeGen/CGCall.h
367–368

Does llvm::Value* actually guarantee three bits? Presumably on 64-bit platforms, but we do support 32-bit hosts.

Fortunately, I don't actually think there's any reason to be space-conscious in this class; all the values are short-lived. Might as well pull all the fields out into a bit-field or something.

371

I think the "is externally destructed" semantics make more sense — that is, the *default* should be that a destructor needs to get pushed, and maybe specific contexts should suppress that. That will also let you avoid all the ugly code to compute whether destruction is required in a bunch of generic places.

Actually, "externally destructed" is a really problematic idea; we need to come up with a better solution for contexts that need to be careful about destructor ordering like this, and it should probably be based on passing around and deactivating destructors. But that's for later.

ahatanak updated this revision to Diff 248363.Mar 4 2020, 5:57 PM
ahatanak marked 2 inline comments as done.
ahatanak removed a project: Restricted Project.
ahatanak removed a subscriber: llvm-commits.

Address review comments.

  • Declare the flags in ReturnValueSlot as bitfields.
  • Replace flag RequiresDestruction with flag IsExternallyDestructed and move the pushDestroy call to EmitCall. I audited all the places where ReturnValueSlot is constructed and there were only a few places where IsExternallyDestructed had to be set to true.
rjmccall added inline comments.Mar 7 2020, 2:39 PM
clang/lib/CodeGen/CGExprAgg.cpp
841

I don't think setExternallyDestructed can be used to communicate outwards like this; the code isn't set up to just do modifications on a single AggValueSlot that's passed around by reference. Instead, the flags are used to communicate downwards to the callees, and the expectation needs to be that callees will push a destructor when they're done initializing unless isExternallyDestructed is set on the dest slot they receive.

ahatanak updated this revision to Diff 249190.Mar 9 2020, 12:29 PM
ahatanak marked an inline comment as done.

Fix the way IsExternallyDestructed is used.

rjmccall added inline comments.Mar 16 2020, 8:54 PM
clang/lib/Sema/SemaExpr.cpp
678

Why only when the l-value is volatile?

5735

You should do this in MaybeBindToTemporary, and then you probably don't need it in most of these other places.

ahatanak updated this revision to Diff 250934.Mar 17 2020, 3:50 PM
ahatanak marked 2 inline comments as done.

Address review comments.

clang/lib/Sema/SemaExpr.cpp
678

I was trying to avoid emitting ExprWithCleanups when it wasn't needed and it seemed that it wasn't needed for non-volatile types. I'm not sure it's important, so I've removed the check for volatile. Also, ExprWithCleanups shouldn't' be emitted when this is in file scope, so I fixed that too.

rjmccall added inline comments.Mar 17 2020, 7:13 PM
clang/lib/Sema/SemaExpr.cpp
678

Hmm, not sure about this file-scope thing, since the combination of C++ dynamic initializers and statement-expressions means we can have pretty unrestricted code there.

ahatanak marked an inline comment as done.Mar 17 2020, 8:58 PM
ahatanak added inline comments.
clang/lib/Sema/SemaExpr.cpp
678

I should have explained why this was needed, but I wanted to prevent emitting ExprWithCleanups in the following example:

struct A {
  id f0;
};

typedef struct A A;

A g = (A){ .f0 = 0 };

The l-value to r-value conversion happens here because compound literals are l-values. Since g is a global of a non-trivial C struct type, we shouldn't try to push a cleanup and destruct the object.

We don't have to think about the C++ case since the line below checks the type is a non-trivial C type. I didn't think about statement expressions, but they aren't allowed in file scope, so I guess that's not a problem either.

rjmccall added inline comments.Mar 18 2020, 10:25 AM
clang/lib/Sema/SemaExpr.cpp
678

I would hope that the constant-evaluator here might be smart enough to ignore some elidable temporaries.

ahatanak marked an inline comment as done.Mar 19 2020, 12:00 AM
ahatanak added inline comments.
clang/lib/Sema/SemaExpr.cpp
678

Do you mean the constant-evaluator should evaluate initializers that are ExprWithCleanups to constants in a case like this? It's possible to do so by seeing whether the sub-expression of ExprWithCleanups is constant, but it looks like we also have to set the CleanupInfo::CleanupsHaveSideEffects flag to false when it's a file scope expression so that ConstExprEmitter::VisitExprWithCleanups can constant-fold the ExprWithCleanups initializer.

rjmccall added inline comments.Mar 19 2020, 10:26 AM
clang/lib/Sema/SemaExpr.cpp
678

In this case, what we're doing is eliding a "temporary" (really an object with wider lifetime, but if the object isn't referenceable, we can compile as-if the object is really temporary) and therefore bypassing the need for a cleanup entirely. I wouldn't expect the AST to change to reflect that this is possible, just the constant evaluator. Basically, it needs to look through ExprWithCleanups; we probably already peephole the LValueToRValue(CompoundLiteralExpr) combination.

We do try to do constant-evaluation on local initializers as well as an optimization, and abstractly that should also work with these constructs.

ahatanak updated this revision to Diff 251463.Mar 19 2020, 1:36 PM
ahatanak marked an inline comment as done.

Teach Expr::isConstantInitializer that ExprWithCleanups can be a constant.

ahatanak added inline comments.Mar 19 2020, 1:38 PM
clang/lib/Sema/SemaExpr.cpp
678

I taught Expr::isConstantInitializer to look through ExprWithCleanups and ConstExprEmitter::VisitExprWithCleanups to ignore whether the expression has side effects. For the latter, I'm assuming we don't have to care about the side effect of the cleanup if the need for a cleanup is elided. This change causes CodeGenFunction::EmitAutoVarInit to emit the initializer as a constant too.

rjmccall accepted this revision.Mar 19 2020, 8:03 PM

Thank, this looks great.

This revision is now accepted and ready to land.Mar 19 2020, 8:03 PM
This revision was automatically updated to reflect the committed changes.