Skip to content

[codegen] Emit missing cleanups when an expression contains a branch #80698

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 15 commits into from
Closed

[codegen] Emit missing cleanups when an expression contains a branch #80698

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
28d9071
[misc-coroutine-hostile-raii] Use getOperand instead of getCommonExpr
usx95 Jan 23, 2024
442809c
[codegen] Emit cleanups for lifetime-extended temporaries when statme…
usx95 Feb 5, 2024
eeabfa2
Merge branch 'llvm:main' into coro-destruct-temporary
usx95 Feb 5, 2024
4125808
format
usx95 Feb 5, 2024
3543711
Emit not-all but only necessary lifetime-extended cleanups for each b…
usx95 Feb 6, 2024
d23cdb3
format
usx95 Feb 6, 2024
dc3659a
format
usx95 Feb 6, 2024
ddf7c24
format
usx95 Feb 6, 2024
a58284b
Add BranchInExpr cleanups kind to handle more missing dtor calls
usx95 Feb 9, 2024
cf5024c
remove dead code
usx95 Feb 9, 2024
db43f02
format
usx95 Feb 9, 2024
034ebda
add branch-in-expr cleanup for lambda init
usx95 Feb 12, 2024
b902c61
fix var name
usx95 Feb 12, 2024
12706f2
fix pre-existing bug
usx95 Feb 12, 2024
ed016d6
fix EmitNewArrayInitializer
usx95 Feb 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 28 additions & 19 deletions clang/lib/CodeGen/CGCleanup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

#include "CGCleanup.h"
#include "CodeGenFunction.h"
#include "EHScopeStack.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/SaveAndRestore.h"

using namespace clang;
Expand Down Expand Up @@ -488,37 +490,37 @@ void CodeGenFunction::PopCleanupBlocks(
}
}

/// Pops cleanup blocks until the given savepoint is reached, then add the
/// cleanups from the given savepoint in the lifetime-extended cleanups stack.
void CodeGenFunction::PopCleanupBlocks(
EHScopeStack::stable_iterator Old, size_t OldLifetimeExtendedSize,
std::initializer_list<llvm::Value **> ValuesToReload) {
PopCleanupBlocks(Old, ValuesToReload);

// Move our deferred cleanups onto the EH stack.
for (size_t I = OldLifetimeExtendedSize,
E = LifetimeExtendedCleanupStack.size(); I != E; /**/) {
void CodeGenFunction::AddDeferredCleanups(DeferredCleanupStack &Cleanups,
size_t OldSize) {
for (size_t I = OldSize, E = Cleanups.size(); I != E;) {
// Alignment should be guaranteed by the vptrs in the individual cleanups.
assert((I % alignof(LifetimeExtendedCleanupHeader) == 0) &&
assert((I % alignof(DeferredCleanupHeader) == 0) &&
"misaligned cleanup stack entry");

LifetimeExtendedCleanupHeader &Header =
reinterpret_cast<LifetimeExtendedCleanupHeader&>(
LifetimeExtendedCleanupStack[I]);
DeferredCleanupHeader &Header =
reinterpret_cast<DeferredCleanupHeader &>(Cleanups[I]);
I += sizeof(Header);

EHStack.pushCopyOfCleanup(Header.getKind(),
&LifetimeExtendedCleanupStack[I],
Header.getSize());
EHStack.pushCopyOfCleanup(Header.getKind(), &Cleanups[I], Header.getSize());
I += Header.getSize();

if (Header.isConditional()) {
Address ActiveFlag =
reinterpret_cast<Address &>(LifetimeExtendedCleanupStack[I]);
Address ActiveFlag = reinterpret_cast<Address &>(Cleanups[I]);
initFullExprCleanupWithFlag(ActiveFlag);
I += sizeof(ActiveFlag);
}
}
}

/// Pops cleanup blocks until the given savepoint is reached, then add the
/// cleanups from the given savepoint in the lifetime-extended cleanups stack.
void CodeGenFunction::PopCleanupBlocks(
EHScopeStack::stable_iterator Old, size_t OldLifetimeExtendedSize,
std::initializer_list<llvm::Value **> ValuesToReload) {
PopCleanupBlocks(Old, ValuesToReload);

// Move our deferred lifetime-extended cleanups onto the EH stack.
AddDeferredCleanups(LifetimeExtendedCleanupStack, OldLifetimeExtendedSize);
LifetimeExtendedCleanupStack.resize(OldLifetimeExtendedSize);
}

Expand Down Expand Up @@ -1102,6 +1104,13 @@ void CodeGenFunction::EmitBranchThroughCleanup(JumpDest Dest) {
if (!HaveInsertPoint())
return;

// If we are emitting a branch in a partial expression, add deferred cleanups
// to EHStack, which would otherwise have only been emitted after the full
// expression.
RunCleanupsScope BranchInExprCleanups(*this);
if (Dest.isValid())
AddDeferredCleanups(BranchInExprCleanupStack, Dest.getBranchInExprDepth());

// Create the branch.
llvm::BranchInst *BI = Builder.CreateBr(Dest.getBlock());

Expand Down
15 changes: 10 additions & 5 deletions clang/lib/CodeGen/CGDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
//
//===----------------------------------------------------------------------===//

#include "Address.h"
#include "CGBlocks.h"
#include "CGCXXABI.h"
#include "CGCleanup.h"
Expand All @@ -19,6 +20,7 @@
#include "CodeGenFunction.h"
#include "CodeGenModule.h"
#include "ConstantEmitter.h"
#include "EHScopeStack.h"
#include "PatternInit.h"
#include "TargetInfo.h"
#include "clang/AST/ASTContext.h"
Expand Down Expand Up @@ -2209,8 +2211,11 @@ void CodeGenFunction::pushLifetimeExtendedDestroy(CleanupKind cleanupKind,
static_cast<CleanupKind>(cleanupKind & ~NormalCleanup), addr, type,
destroyer, useEHCleanupForArray);

return pushCleanupAfterFullExprWithActiveFlag<DestroyObject>(
cleanupKind, Address::invalid(), addr, type, destroyer, useEHCleanupForArray);
pushDestroy(BranchInExprCleanup, addr, type, destroyer,
useEHCleanupForArray);
return pushDeferredCleanup<DestroyObject>(
LifetimeExtendedCleanupStack, cleanupKind, Address::invalid(), addr,
type, destroyer, useEHCleanupForArray);
}

// Otherwise, we should only destroy the object if it's been initialized.
Expand All @@ -2232,9 +2237,9 @@ void CodeGenFunction::pushLifetimeExtendedDestroy(CleanupKind cleanupKind,
initFullExprCleanupWithFlag(ActiveFlag);
}

pushCleanupAfterFullExprWithActiveFlag<ConditionalCleanupType>(
cleanupKind, ActiveFlag, SavedAddr, type, destroyer,
useEHCleanupForArray);
pushDeferredCleanup<ConditionalCleanupType>(
LifetimeExtendedCleanupStack, cleanupKind, ActiveFlag, SavedAddr, type,
destroyer, useEHCleanupForArray);
}

/// emitDestroy - Immediately perform the destruction of the given
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/CodeGen/CGExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4961,7 +4961,7 @@ LValue CodeGenFunction::EmitCompoundLiteralLValue(const CompoundLiteralExpr *E){
if (QualType::DestructionKind DtorKind = E->getType().isDestructedType())
pushLifetimeExtendedDestroy(getCleanupKind(DtorKind), DeclPtr,
E->getType(), getDestroyer(DtorKind),
DtorKind & EHCleanup);
getCleanupKind(DtorKind) & EHCleanup);

return Result;
}
Expand Down
31 changes: 21 additions & 10 deletions clang/lib/CodeGen/CGExprAgg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "CodeGenFunction.h"
#include "CodeGenModule.h"
#include "ConstantEmitter.h"
#include "EHScopeStack.h"
#include "TargetInfo.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/Attr.h"
Expand Down Expand Up @@ -565,10 +566,6 @@ void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType,
elementAlign,
CGF.getDestroyer(dtorKind));
cleanup = CGF.EHStack.stable_begin();

// Otherwise, remember that we didn't need a cleanup.
} else {
dtorKind = QualType::DK_none;
}

llvm::Value *one = llvm::ConstantInt::get(CGF.SizeTy, 1);
Expand All @@ -580,6 +577,7 @@ void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType,
// elements have been initialized.
llvm::Value *element = begin;

CodeGenFunction::RestoreBranchInExprRAII branchInExpr(CGF);
// Emit the explicit initializers.
for (uint64_t i = 0; i != NumInitElements; ++i) {
// Advance to the next element.
Expand All @@ -592,10 +590,14 @@ void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType,
// observed to be unnecessary.
if (endOfInit.isValid()) Builder.CreateStore(element, endOfInit);
}

LValue elementLV = CGF.MakeAddrLValue(
Address(element, llvmElementType, elementAlign), elementType);
Address address = Address(element, llvmElementType, elementAlign);
LValue elementLV = CGF.MakeAddrLValue(address, elementType);
EmitInitializationToLValue(Args[i], elementLV);
// Schedule to emit element cleanup if we see a branch in the array
// initialisation expression.
if (CGF.needsBranchCleanup(dtorKind))
CGF.pushDestroy(BranchInExprCleanup, address, elementType,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts on pushing a cleanup for each array element, vs. using a single cleanup which iterates over the elements, like we do for exceptions?

Is there any chance an ArrayFiller needs a branch cleanup? Off the top of my head, I don't think it's possible because branches exits can only happen in code explicitly written in the current function, but that's a subtle invariant.

Copy link
Contributor Author

@usx95 usx95 Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Element wise cleanup Vs iteration-style cleanup.

I had considered using IrregularPartialArrayDestroy instead of element-wise destroy. The catch is that this cleanup expects allocated array.init.end to store the current end pointer. This is updated as we iterate over and create the elements.
I do not think we should add these "stores" overhead, especially with -fno-exceptions when there are no branches in expr (the simple cases).

What we could be doing instead is just considering the current array index and using that for the array end address.
But that seems to be warned against in the comments:

      // In principle we could tell the Cleanup where we are more
      // directly, but the control flow can get so varied here that it
      // would actually be quite complex.  Therefore we go through an
      // alloca.

I do not understand the "complex" flows where this could go wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The complexity here is basically that we'd have to reimplement mem2reg to compute the correct number of cleanups along every control-flow path. Instead, we store to an alloca, and just let the mem2reg optimization clean it up.

On a similar note, because mem2reg cleans up the stores, their overhead is minimal.

Copy link
Contributor Author

@usx95 usx95 Feb 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please tell me how to verify that mem2reg will clear these stores.
If we can use this cleanup as-is, then we should be able to reuse the EH-only cleanups without introducing another cleanup stack (as also pointed out by @jyknight).

(I was not able to remove the stores by bin/opt -S -passes=mem2reg. I am new to Codegen so I might be missing something.)

It is important that we do not introduce these stores with exceptions disabled. But from the looks of it, it should be a easy optimisation pass as the store would never be read in normal circumstances.

This is the only blocker in the reusing EH-only cleanups.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you generated the IR correctly? Note that clang marks everything optnone at -O0 by default.

That said, I guess -O0 codegen is a potential issue; I don't think we run any passes that would clean up the dead stores in that case. I'd only be concerned about the case where we don't actually emit the cleanup; trying to minimize the codesize of the cleanup code isn't important at -O0.

To address the case of no cleanups, maybe we could generate the stores lazily? Basically, save where the stores should be, but don't generate them until we know the cleanup is actually used. Or equivalently, emit the stores, then erase them if the cleanup isn't emitted.

CGF.getDestroyer(dtorKind), false);
}

// Check whether there's a non-trivial array-fill expression.
Expand Down Expand Up @@ -666,7 +668,8 @@ void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType,
}

// Leave the partial-array cleanup if we entered one.
if (dtorKind) CGF.DeactivateCleanupBlock(cleanup, cleanupDominator);
if (cleanupDominator)
CGF.DeactivateCleanupBlock(cleanup, cleanupDominator);
}

//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -710,7 +713,7 @@ AggExprEmitter::VisitCompoundLiteralExpr(CompoundLiteralExpr *E) {
if (QualType::DestructionKind DtorKind = E->getType().isDestructedType())
CGF.pushLifetimeExtendedDestroy(
CGF.getCleanupKind(DtorKind), Slot.getAddress(), E->getType(),
CGF.getDestroyer(DtorKind), DtorKind & EHCleanup);
CGF.getDestroyer(DtorKind), CGF.getCleanupKind(DtorKind) & EHCleanup);
}

/// Attempt to look through various unimportant expressions to find a
Expand Down Expand Up @@ -1360,6 +1363,7 @@ AggExprEmitter::VisitLambdaExpr(LambdaExpr *E) {
SmallVector<EHScopeStack::stable_iterator, 16> Cleanups;
llvm::Instruction *CleanupDominator = nullptr;

CodeGenFunction::RestoreBranchInExprRAII RestoreBranchInExpr(CGF);
CXXRecordDecl::field_iterator CurField = E->getLambdaClass()->field_begin();
for (LambdaExpr::const_capture_init_iterator i = E->capture_init_begin(),
e = E->capture_init_end();
Expand All @@ -1377,6 +1381,9 @@ AggExprEmitter::VisitLambdaExpr(LambdaExpr *E) {
if (QualType::DestructionKind DtorKind =
CurField->getType().isDestructedType()) {
assert(LV.isSimple());
if (CGF.needsBranchCleanup(DtorKind))
CGF.pushDestroy(BranchInExprCleanup, LV.getAddress(CGF),
CurField->getType(), CGF.getDestroyer(DtorKind), false);
if (CGF.needsEHCleanup(DtorKind)) {
if (!CleanupDominator)
CleanupDominator = CGF.Builder.CreateAlignedLoad(
Expand Down Expand Up @@ -1754,7 +1761,7 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(

return;
}

CodeGenFunction::RestoreBranchInExprRAII branchInExpr(CGF);
// Here we iterate over the fields; this makes it simpler to both
// default-initialize fields and skip over unnamed fields.
for (const auto *field : record->fields()) {
Expand Down Expand Up @@ -1793,6 +1800,10 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(
if (QualType::DestructionKind dtorKind
= field->getType().isDestructedType()) {
assert(LV.isSimple());
if (CGF.needsBranchCleanup(dtorKind)) {
CGF.pushDestroy(BranchInExprCleanup, LV.getAddress(CGF),
field->getType(), CGF.getDestroyer(dtorKind), false);
}
if (CGF.needsEHCleanup(dtorKind)) {
CGF.pushDestroy(EHCleanup, LV.getAddress(CGF), field->getType(),
CGF.getDestroyer(dtorKind), false);
Expand Down
4 changes: 3 additions & 1 deletion clang/lib/CodeGen/CGStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,9 +627,11 @@ CodeGenFunction::getJumpDestForLabel(const LabelDecl *D) {
if (Dest.isValid()) return Dest;

// Create, but don't insert, the new block.
// FIXME: We do not know `BranchInExprDepth` for the destination and currently
// emit *all* the BranchInExpr cleanups.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you briefly describe what you expect the fix for this to look like, if you have some idea?

Copy link
Contributor Author

@usx95 usx95 Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remember/copy the entire BranchInExpr stack in a branch-fix-up and then emit the cleanups from this stored BranchInExpr stack up until the now-known destination depth.
This is because we simply pop the BranchInExpr stack on leaving an expression, but in this case, we need to remember which cleanups were popped earlier.

(This would not work if the goto destination is inside a different stmt-expr. In this case, the code is invalid anyways.)

Probably need to rethink how BranchInExpr is emitted to avoid copying the complete stack each time.

Dest = JumpDest(createBasicBlock(D->getName()),
EHScopeStack::stable_iterator::invalid(),
NextCleanupDestIndex++);
/*BranchInExprDepth=*/0, NextCleanupDestIndex++);
return Dest;
}

Expand Down
3 changes: 3 additions & 0 deletions clang/lib/CodeGen/CodeGenFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,9 @@ static void EmitIfUsed(CodeGenFunction &CGF, llvm::BasicBlock *BB) {
void CodeGenFunction::FinishFunction(SourceLocation EndLoc) {
assert(BreakContinueStack.empty() &&
"mismatched push/pop in break/continue stack!");
assert(LifetimeExtendedCleanupStack.empty() &&
"mismatched push/pop in stack!");
assert(BranchInExprCleanupStack.empty() && "mismatched push/pop in stack!");

bool OnlySimpleReturnStmts = NumSimpleReturnExprs > 0
&& NumSimpleReturnExprs == NumReturnExprs
Expand Down
Loading