Skip to content

Commit 69a8845

Browse files
atrickatrick
committed
Fix ownership of select_enum instruction
This instruction was given forwarding ownership in the original OSSA implementation. That will obviously lead to memory leaks. Remove ownership from this instruction and verify that it is never used for non-trivial types.
1 parent eccf94d commit 69a8845

File tree

16 files changed

+24
-124
lines changed

16 files changed

+24
-124
lines changed

‎SwiftCompilerSources/Sources/SIL/ForwardingInstruction.swift

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -134,12 +134,6 @@ extension TuplePackExtractInst {
134134
}
135135
}
136136

137-
extension SelectEnumInst {
138-
public var singleForwardedOperand: Operand {
139-
return enumOperand
140-
}
141-
}
142-
143137
//===----------------------------------------------------------------------===//
144138
// forwardedResults
145139
//===----------------------------------------------------------------------===//
@@ -246,10 +240,6 @@ extension MarkUninitializedInst {
246240
public var preservesRepresentation: Bool { true }
247241
}
248242

249-
extension SelectEnumInst {
250-
public var preservesRepresentation: Bool { true }
251-
}
252-
253243
extension StructExtractInst {
254244
public var preservesRepresentation: Bool { true }
255245
}

‎include/swift/SIL/InstWrappers.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -269,9 +269,6 @@ class ForwardingOperation {
269269
&forwardingInst->getOperandRef(RefToBridgeObjectInst::ConvertedOperand);
270270
case SILInstructionKind::TuplePackExtractInst:
271271
return &forwardingInst->getOperandRef(TuplePackExtractInst::TupleOperand);
272-
case SILInstructionKind::SelectEnumInst:
273-
// ignore trailing case operands
274-
return &forwardingInst->getOperandRef(0);
275272
default:
276273
int numRealOperands = forwardingInst->getNumRealOperands();
277274
if (numRealOperands == 0) {

‎include/swift/SIL/SILBuilder.h

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1754,20 +1754,10 @@ class SILBuilder {
17541754
ArrayRef<std::pair<EnumElementDecl *, SILValue>> CaseValues,
17551755
llvm::Optional<ArrayRef<ProfileCounter>> CaseCounts = llvm::None,
17561756
ProfileCounter DefaultCount = ProfileCounter()) {
1757-
return createSelectEnum(Loc, Operand, Ty, DefaultValue, CaseValues,
1758-
CaseCounts, DefaultCount,
1759-
Operand->getOwnershipKind());
1760-
}
1761-
1762-
SelectEnumInst *createSelectEnum(
1763-
SILLocation Loc, SILValue Operand, SILType Ty, SILValue DefaultValue,
1764-
ArrayRef<std::pair<EnumElementDecl *, SILValue>> CaseValues,
1765-
llvm::Optional<ArrayRef<ProfileCounter>> CaseCounts,
1766-
ProfileCounter DefaultCount, ValueOwnershipKind forwardingOwnershipKind) {
17671757
assert(isLoadableOrOpaque(Ty));
17681758
return insert(SelectEnumInst::create(
17691759
getSILDebugLocation(Loc), Operand, Ty, DefaultValue, CaseValues,
1770-
getModule(), CaseCounts, DefaultCount, forwardingOwnershipKind));
1760+
getModule(), CaseCounts, DefaultCount));
17711761
}
17721762

17731763
SelectEnumAddrInst *createSelectEnumAddr(

‎include/swift/SIL/SILCloner.h

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3289,10 +3289,7 @@ SILCloner<ImplClass>::visitSelectEnumInst(SelectEnumInst *Inst) {
32893289
Inst, getBuilder().createSelectEnum(
32903290
getOpLocation(Inst->getLoc()),
32913291
getOpValue(Inst->getEnumOperand()), getOpType(Inst->getType()),
3292-
DefaultResult, CaseResults, llvm::None, ProfileCounter(),
3293-
getBuilder().hasOwnership()
3294-
? Inst->getForwardingOwnershipKind()
3295-
: ValueOwnershipKind(OwnershipKind::None)));
3292+
DefaultResult, CaseResults, llvm::None, ProfileCounter()));
32963293
}
32973294

32983295
template<typename ImplClass>

‎include/swift/SIL/SILInstruction.h

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6687,23 +6687,20 @@ class SelectEnumInstBase : public BaseTy {
66876687
class SelectEnumInst final
66886688
: public InstructionBaseWithTrailingOperands<
66896689
SILInstructionKind::SelectEnumInst, SelectEnumInst,
6690-
SelectEnumInstBase<SelectEnumInst,
6691-
OwnershipForwardingSingleValueInstruction>,
6690+
SelectEnumInstBase<SelectEnumInst, SingleValueInstruction>,
66926691
EnumElementDecl *> {
66936692
friend SILBuilder;
6694-
friend SelectEnumInstBase<SelectEnumInst,
6695-
OwnershipForwardingSingleValueInstruction>;
6693+
friend SelectEnumInstBase<SelectEnumInst, SingleValueInstruction>;
66966694

66976695
public:
66986696
SelectEnumInst(SILDebugLocation DebugLoc, SILValue Operand, SILType Type,
66996697
bool DefaultValue, ArrayRef<SILValue> CaseValues,
67006698
ArrayRef<EnumElementDecl *> CaseDecls,
67016699
llvm::Optional<ArrayRef<ProfileCounter>> CaseCounts,
6702-
ProfileCounter DefaultCount,
6703-
ValueOwnershipKind forwardingOwnershipKind)
6700+
ProfileCounter DefaultCount)
67046701
: InstructionBaseWithTrailingOperands(
67056702
Operand, CaseValues, DebugLoc, Type, bool(DefaultValue), CaseCounts,
6706-
DefaultCount, forwardingOwnershipKind) {
6703+
DefaultCount) {
67076704
assert(CaseValues.size() - DefaultValue == CaseDecls.size());
67086705
std::uninitialized_copy(CaseDecls.begin(), CaseDecls.end(),
67096706
getTrailingObjects<EnumElementDecl *>());
@@ -6713,8 +6710,7 @@ class SelectEnumInst final
67136710
SILValue DefaultValue,
67146711
ArrayRef<std::pair<EnumElementDecl *, SILValue>> CaseValues,
67156712
SILModule &M, llvm::Optional<ArrayRef<ProfileCounter>> CaseCounts,
6716-
ProfileCounter DefaultCount,
6717-
ValueOwnershipKind forwardingOwnershipKind);
6713+
ProfileCounter DefaultCount);
67186714
};
67196715

67206716
/// Select one of a set of values based on the case of an enum.
@@ -10654,7 +10650,6 @@ OwnershipForwardingSingleValueInstruction::classof(SILInstructionKind kind) {
1065410650
case SILInstructionKind::BridgeObjectToRefInst:
1065510651
case SILInstructionKind::ThinToThickFunctionInst:
1065610652
case SILInstructionKind::UnconditionalCheckedCastInst:
10657-
case SILInstructionKind::SelectEnumInst:
1065810653
return true;
1065910654
default:
1066010655
return false;

‎lib/SIL/IR/OperandOwnership.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -432,8 +432,8 @@ OperandOwnershipClassifier::visitSelectEnumInst(SelectEnumInst *i) {
432432
if (getValue() == i->getEnumOperand()) {
433433
return OperandOwnership::InstantaneousUse;
434434
}
435-
return getOwnershipKind().getForwardingOperandOwnership(
436-
/*allowUnowned*/true);
435+
assert(i->getType().isTrivial(i->getFunction()));
436+
return OperandOwnership::TrivialUse;
437437
}
438438

439439
OperandOwnership OperandOwnershipClassifier::visitBranchInst(BranchInst *bi) {

‎lib/SIL/IR/SILInstructions.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2008,9 +2008,9 @@ SelectEnumInst *SelectEnumInst::create(
20082008
SILDebugLocation Loc, SILValue Operand, SILType Type, SILValue DefaultValue,
20092009
ArrayRef<std::pair<EnumElementDecl *, SILValue>> CaseValues, SILModule &M,
20102010
llvm::Optional<ArrayRef<ProfileCounter>> CaseCounts,
2011-
ProfileCounter DefaultCount, ValueOwnershipKind forwardingOwnership) {
2011+
ProfileCounter DefaultCount) {
20122012
return createSelectEnum(Loc, Operand, Type, DefaultValue, CaseValues, M,
2013-
CaseCounts, DefaultCount, forwardingOwnership);
2013+
CaseCounts, DefaultCount);
20142014
}
20152015

20162016
SelectEnumAddrInst *SelectEnumAddrInst::create(

‎lib/SIL/IR/SILPrinter.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2734,7 +2734,6 @@ class SILPrinter : public SILInstructionVisitor<SILPrinter> {
27342734

27352735
void visitSelectEnumInst(SelectEnumInst *SEI) {
27362736
printSelectEnumInst(SEI);
2737-
printForwardingOwnershipKind(SEI, SEI->getOperand());
27382737
}
27392738
void visitSelectEnumAddrInst(SelectEnumAddrInst *SEI) {
27402739
printSelectEnumInst(SEI);

‎lib/SIL/IR/ValueOwnership.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,7 @@ CONSTANT_OWNERSHIP_INST(None, ProjectExistentialBox)
140140
CONSTANT_OWNERSHIP_INST(None, RefElementAddr)
141141
CONSTANT_OWNERSHIP_INST(None, RefTailAddr)
142142
CONSTANT_OWNERSHIP_INST(None, RefToRawPointer)
143+
CONSTANT_OWNERSHIP_INST(None, SelectEnum)
143144
CONSTANT_OWNERSHIP_INST(None, SelectEnumAddr)
144145
CONSTANT_OWNERSHIP_INST(None, StringLiteral)
145146
CONSTANT_OWNERSHIP_INST(None, StructElementAddr)
@@ -274,7 +275,6 @@ FORWARDING_OWNERSHIP_INST(UnconditionalCheckedCast)
274275
FORWARDING_OWNERSHIP_INST(Upcast)
275276
FORWARDING_OWNERSHIP_INST(UncheckedValueCast)
276277
FORWARDING_OWNERSHIP_INST(UncheckedEnumData)
277-
FORWARDING_OWNERSHIP_INST(SelectEnum)
278278
FORWARDING_OWNERSHIP_INST(Enum)
279279
FORWARDING_OWNERSHIP_INST(MarkDependence)
280280
// NOTE: init_existential_ref from a reference counting perspective is not

‎lib/SIL/Parser/ParseSIL.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6040,8 +6040,8 @@ bool SILParser::parseSpecificSILInstruction(SILBuilder &B,
60406040

60416041
if (Opcode == SILInstructionKind::SelectEnumInst) {
60426042
ResultVal = B.createSelectEnum(InstLoc, Val, ResultType, DefaultValue,
6043-
CaseValues, llvm::None, ProfileCounter(),
6044-
forwardingOwnership);
6043+
CaseValues, llvm::None,
6044+
ProfileCounter());
60456045
} else
60466046
ResultVal = B.createSelectEnumAddr(InstLoc, Val, ResultType,
60476047
DefaultValue, CaseValues);

0 commit comments

Comments
 (0)