Page MenuHomePhabricator

[CodeGen] Emit destructor calls for non-trivial C structs
Needs ReviewPublic

Authored by ahatanak on Jul 9 2019, 6:40 PM.



This patch fixes a bug in IRGen where it wasn't calling the destructors for non-trivial C structs in the following cases:

  • member access of structs that are returned from a function.
  • compound literal.
  • load from volatile types.


Diff Detail

rC Clang

Event Timeline

ahatanak created this revision.Jul 9 2019, 6:40 PM
ahatanak marked an inline comment as done.Jul 9 2019, 6:42 PM
ahatanak added inline comments.

This change wasn't needed to fix the bugs, but I think isExternallyDestructed instead of isIgnored should be called to determine whether the returned value requires destruction.

rjmccall added inline comments.Jul 9 2019, 10:48 PM

Unfortunately, the lifetime of compound literals in C is not this simple; they're like blocks in that they're destroyed at the end of the enclosing scope rather than at the end of the current statement. (The cleanup here will be popped at the end of the full-expression if we've entered an ExprWithCleanups.) And the l-value case is exactly the case where this matters.

I think you need to do something like what we do with blocks, where we record all the blocks in the full-expression on the ExprWithCleanups so that we can push an inactive cleanup for them and then activate it when we emit the block.




The cleanup shouldn't be pushed until after you've finished evaluating the initializer.

ahatanak updated this revision to Diff 212942.Aug 1 2019, 5:54 PM
ahatanak marked 2 inline comments as done.
  • Emit member access, compound literal, and call expressions as subexpressions of ExprWithCleanups if the expressions are of C struct types that require non-trivial destruction. This fixes the bug in IRGen where it was destructing the function return at the end of the enclosing scope rather than at the end of the full expression (see the changes made in test/CodeGenObjC/arc.m).
  • Add compound literal expressions with automatic storage duration to the list of cleanup objects of ExprWithCleanups if the expressions have C struct types requiring non-trivial destruction. This enables IRGen to destruct the compound literals at the end of their enclosing scopes.
rjmccall added inline comments.Aug 2 2019, 8:45 AM

Might be worth spelling out and block-scoped compound literals here.


Ah, good catch.


I think the second sentence in this comment is no longer useful; you can just strike it.


This comment is no longer accurate. If you want to move the function, please do so in a separate commit, though.


Please include in the comment here that this doesn't apply in C++, where the compound literal has temporary lifetime.

Can we clarify that we're talking about block-scope literals in all the new method names?


It'd be nice to not shadow the enclosing variable.


Can we make the check here something like (1) this is a block-scope compound literal and (2) it has a non-trivially-destructed type (of any kind)? That way we're not conflating two potentially unrelated elements, the lifetime of the object and the kinds of types that can be constructed by the literal.

Oh, actually, there's a concrete reason to do this: C99 compound literals are not required to have struct type; they can have any object type, including arrays but also scalars. So we could, even without non-trivial C structs, have a block-scope compound of type __strong id[]; I guess we've always just gotten this wrong. Please add tests for this case. :)


Does EmitCallExpr not enter a cleanup when it returns an aggregate that's not into an externally-destructed slot? That seems wrong and dangerous.

ahatanak marked an inline comment as done.Aug 12 2019, 10:00 AM
ahatanak added inline comments.

I'm going to split this patch into two parts, one for compound literals and the other for everything else. The patch is getting too large and I also found another bug: a cleanup isn't pushed for ObjC message send.

martong added inline comments.Aug 16 2019, 4:58 AM

Perhaps then this patch depends on another patch which implements the import of a BlockExpr?
Or maybe the branch if (auto *BD = From.dyn_cast<BlockDecl *>()) should be left out from the ASTImporter code, and this way BlockExpr and BlockDecl would be implemented later in another patch.