Skip to content

[SILGen] Generate a trap for unexpected cases in all @objc enums #14724

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

Merged
merged 3 commits into from
Feb 21, 2018
Merged

[SILGen] Generate a trap for unexpected cases in all @objc enums #14724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d782f46
[SILGen] Factor out the generation of blocks for switching on an enum
jrose-apple Feb 21, 2018
36eae9d
[SILGen] Generate a trap for unexpected cases in all @objc enums
jrose-apple Feb 20, 2018
49e71c9
[SILGen] Minor improvements to generateEnumCaseBlocks
jrose-apple Feb 21, 2018
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
289 changes: 120 additions & 169 deletions lib/SILGen/SILGenPattern.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1679,113 +1679,141 @@ void PatternMatchEmission::emitIsDispatch(ArrayRef<RowToSpecialize> rows,
[&] { (*innerFailure)(loc); }, rows[0].Count);
}

/// Perform specialized dispatch for a sequence of EnumElementPattern or an
/// OptionalSomePattern.
void PatternMatchEmission::emitEnumElementDispatchWithOwnership(
ArrayRef<RowToSpecialize> rows, ConsumableManagedValue src,
const SpecializationHandler &handleCase,
const FailureHandler &outerFailure) {
assert(src.getFinalConsumption() != CastConsumptionKind::TakeOnSuccess &&
"SIL ownership does not support TakeOnSuccess");

CanType sourceType = rows[0].Pattern->getType()->getCanonicalType();

namespace {
struct CaseInfo {
EnumElementDecl *FormalElement;
Pattern *FirstMatcher;
bool Irrefutable = false;
SmallVector<SpecializedRow, 2> SpecializedRows;
};
} // end anonymous namespace

SILBasicBlock *curBB = SGF.B.getInsertionBB();

// Collect the cases and specialized rows.
//
// These vectors are completely parallel, but the switch
// instructions want only the first information, so we split them up.
SmallVector<std::pair<EnumElementDecl *, SILBasicBlock *>, 4> caseBBs;
SmallVector<CaseInfo, 4> caseInfos;
SILBasicBlock *defaultBB = nullptr;
/// Create destination blocks for switching over the cases in an enum defined
/// by \p rows.
static void generateEnumCaseBlocks(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I factored this out from its duplication across the "ownership" and "non-ownership" versions of the pattern-match generation (*shakes fist at @gottesmm*), so it might be easier to review the whitespace-insensitive diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Two things:

  1. Can you put the refactoring in a separate commit? That will make the diff even /easier/ to read than whitespace-insensitive diff.
  2. I purposely duplicated the code so I could easily visualize the difference in between the two.

Copy link
Contributor Author

@jrose-apple jrose-apple Feb 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Fair, will do.
  2. Please don't do this in the future. You saw below that there was already an inconsistency in code coverage.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jrose-apple on 2:

  1. At the time, it was going to be temporary, but other things took priority. When you are trying to make a large transition in a code base (like putting ownership into SILGen), it is "cleaner" to duplicate old -> new/fix new/remove the old. I was just interrupted by the +0 work. Once +0 is done, this is next on my list to fix.

  2. I disagree strongly with your characterization that this code duplication directly resulted in the code coverage inconsistency. What resulted in the code coverage inconsistency is a lack of enough test coverage of our emission of code coverage. If we had test coverage of such paths, then the error would have never happened. That is like blaming a night time driver for hitting a hole dug by road construction crews, when the road construction crews did not put up any signs with warnings.

NOTE I am taking your word for the error. I have not looked at it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That being said, I do not think arguing about blame/etc about the coverage is productive, so lets drop that for now? ; ).

SILGenFunction &SGF,
ArrayRef<RowToSpecialize> rows,
CanType sourceType,
SILBasicBlock *curBB,
SmallVectorImpl<std::pair<EnumElementDecl *, SILBasicBlock *>> &caseBBs,
SmallVectorImpl<ProfileCounter> &caseCounts,
SmallVectorImpl<CaseInfo> &caseInfos,
SILBasicBlock *&defaultBB) {

assert(caseBBs.empty());
assert(caseCounts.empty());
assert(caseInfos.empty());
assert(defaultBB == nullptr);

caseBBs.reserve(rows.size());
caseInfos.reserve(rows.size());

{
// Create destination blocks for all the cases.
llvm::DenseMap<EnumElementDecl *, unsigned> caseToIndex;
for (auto &row : rows) {
EnumElementDecl *formalElt;
Pattern *subPattern = nullptr;
if (auto eep = dyn_cast<EnumElementPattern>(row.Pattern)) {
formalElt = eep->getElementDecl();
subPattern = eep->getSubPattern();
} else {
auto *osp = cast<OptionalSomePattern>(row.Pattern);
formalElt = osp->getElementDecl();
subPattern = osp->getSubPattern();
}
auto elt = SGF.SGM.getLoweredEnumElementDecl(formalElt);
auto enumDecl = sourceType.getEnumOrBoundGenericEnum();

unsigned index = caseInfos.size();
auto insertionResult = caseToIndex.insert({elt, index});
if (!insertionResult.second) {
index = insertionResult.first->second;
} else {
curBB = SGF.createBasicBlock(curBB);
caseBBs.push_back({elt, curBB});
caseInfos.resize(caseInfos.size() + 1);
caseInfos.back().FormalElement = formalElt;
caseInfos.back().FirstMatcher = row.Pattern;
}
assert(caseToIndex[elt] == index);
assert(caseBBs[index].first == elt);

auto &info = caseInfos[index];
info.Irrefutable = (info.Irrefutable || row.Irrefutable);
info.SpecializedRows.resize(info.SpecializedRows.size() + 1);
auto &specRow = info.SpecializedRows.back();
specRow.RowIndex = row.RowIndex;

// Use the row pattern, if it has one.
if (subPattern) {
specRow.Patterns.push_back(subPattern);
// It's also legal to write:
// case .Some { ... }
// which is an implicit wildcard.
} else {
specRow.Patterns.push_back(nullptr);
}
llvm::SmallDenseMap<EnumElementDecl *, unsigned, 16> caseToIndex;
for (auto &row : rows) {
EnumElementDecl *formalElt;
Pattern *subPattern = nullptr;
if (auto eep = dyn_cast<EnumElementPattern>(row.Pattern)) {
formalElt = eep->getElementDecl();
subPattern = eep->getSubPattern();
} else {
auto *osp = cast<OptionalSomePattern>(row.Pattern);
formalElt = osp->getElementDecl();
subPattern = osp->getSubPattern();
}
auto elt = SGF.SGM.getLoweredEnumElementDecl(formalElt);
assert(elt->getParentEnum() == enumDecl);

// We always need a default block if the enum is resilient.
// If the enum is @_fixed_layout, we only need one if the
// switch is not exhaustive.
bool exhaustive = false;
auto enumDecl = sourceType.getEnumOrBoundGenericEnum();
unsigned index = caseInfos.size();
auto insertionResult = caseToIndex.insert({elt, index});
if (!insertionResult.second) {
index = insertionResult.first->second;
} else {
curBB = SGF.createBasicBlock(curBB);
caseBBs.push_back({elt, curBB});
caseInfos.push_back(CaseInfo());
caseInfos.back().FormalElement = formalElt;
caseInfos.back().FirstMatcher = row.Pattern;
caseCounts.push_back(row.Count);
}
assert(caseToIndex[elt] == index);
assert(caseBBs[index].first == elt);

if (!enumDecl->isResilient(SGF.SGM.M.getSwiftModule(),
SGF.F.getResilienceExpansion())) {
exhaustive = true;
auto &info = caseInfos[index];
info.Irrefutable = (info.Irrefutable || row.Irrefutable);
info.SpecializedRows.push_back(SpecializedRow());
auto &specRow = info.SpecializedRows.back();
specRow.RowIndex = row.RowIndex;

for (auto elt : enumDecl->getAllElements()) {
if (!caseToIndex.count(elt)) {
exhaustive = false;
break;
}
}
// Use the row pattern, if it has one.
if (subPattern) {
specRow.Patterns.push_back(subPattern);
// It's also legal to write:
// case .Some { ... }
// which is an implicit wildcard.
} else {
specRow.Patterns.push_back(nullptr);
}

if (!exhaustive)
defaultBB = SGF.createBasicBlock(curBB);
}

assert(caseBBs.size() == caseInfos.size());

// Check to see if the enum may have values beyond the cases we can see
// at compile-time. This includes future cases (for resilient enums) and
// random values crammed into C enums.
//
// Note: This relies on the fact that there are no "non-resilient" enums that
// are still non-exhaustive, except for @objc enums.
bool canAssumeExhaustive = !enumDecl->isObjC();
if (canAssumeExhaustive) {
canAssumeExhaustive =
!enumDecl->isResilient(SGF.SGM.SwiftModule,
SGF.F.getResilienceExpansion());
}
if (canAssumeExhaustive) {
// Check that Sema didn't let any cases slip through. (This can happen
// with @_downgrade_exhaustivity_check.)
canAssumeExhaustive = llvm::all_of(enumDecl->getAllElements(),
[&](const EnumElementDecl *elt) {
return caseToIndex.count(elt);
});
}

if (!canAssumeExhaustive)
defaultBB = SGF.createBasicBlock(curBB);
}

/// Perform specialized dispatch for a sequence of EnumElementPattern or an
/// OptionalSomePattern.
void PatternMatchEmission::emitEnumElementDispatchWithOwnership(
ArrayRef<RowToSpecialize> rows, ConsumableManagedValue src,
const SpecializationHandler &handleCase,
const FailureHandler &outerFailure) {
assert(src.getFinalConsumption() != CastConsumptionKind::TakeOnSuccess &&
"SIL ownership does not support TakeOnSuccess");

CanType sourceType = rows[0].Pattern->getType()->getCanonicalType();

// Collect the cases and specialized rows.
//
// These vectors are completely parallel, but the switch
// instructions want only the first information, so we split them up.
SmallVector<std::pair<EnumElementDecl *, SILBasicBlock *>, 4> caseBBs;
SmallVector<ProfileCounter, 4> caseCounts;
SmallVector<CaseInfo, 4> caseInfos;
SILBasicBlock *defaultBB = nullptr;

generateEnumCaseBlocks(SGF, rows, sourceType, SGF.B.getInsertionBB(),
caseBBs, caseCounts, caseInfos, defaultBB);

SILLocation loc = PatternMatchStmt;
loc.setDebugLoc(rows[0].Pattern);
// SEMANTIC SIL TODO: Once we have the representation of a switch_enum that
// can take a +0 value, this extra copy should be a borrow.
SILValue srcValue = src.getFinalManagedValue().copy(SGF, loc).forward(SGF);
// FIXME: Pass caseCounts in here as well, as it is in
// emitEnumElementDispatch.
SGF.B.createSwitchEnum(loc, srcValue, defaultBB, caseBBs);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vedantk / @gottesmm, this looks like a place where we just weren't generating code coverage counters. Is it correct to have the caseCounts generated in the same way as the non-ownership implementation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Switch and case statements are handled for coverage reporting purposes. This might be more up @shajrawi's alley.


// Okay, now emit all the cases.
Expand Down Expand Up @@ -1910,15 +1938,6 @@ void PatternMatchEmission::emitEnumElementDispatch(

CanType sourceType = rows[0].Pattern->getType()->getCanonicalType();

struct CaseInfo {
EnumElementDecl *FormalElement;
Pattern *FirstMatcher;
bool Irrefutable = false;
SmallVector<SpecializedRow, 2> SpecializedRows;
};

SILBasicBlock *curBB = SGF.B.getInsertionBB();

// Collect the cases and specialized rows.
//
// These vectors are completely parallel, but the switch
Expand All @@ -1928,80 +1947,8 @@ void PatternMatchEmission::emitEnumElementDispatch(
SmallVector<ProfileCounter, 4> caseCounts;
SILBasicBlock *defaultBB = nullptr;

caseBBs.reserve(rows.size());
caseInfos.reserve(rows.size());

{
// Create destination blocks for all the cases.
llvm::DenseMap<EnumElementDecl*, unsigned> caseToIndex;
for (auto &row : rows) {
EnumElementDecl *formalElt;
Pattern *subPattern = nullptr;
if (auto eep = dyn_cast<EnumElementPattern>(row.Pattern)) {
formalElt = eep->getElementDecl();
subPattern = eep->getSubPattern();
} else {
auto *osp = cast<OptionalSomePattern>(row.Pattern);
formalElt = osp->getElementDecl();
subPattern = osp->getSubPattern();
}
auto elt = SGF.SGM.getLoweredEnumElementDecl(formalElt);

unsigned index = caseInfos.size();
auto insertionResult = caseToIndex.insert({elt, index});
if (!insertionResult.second) {
index = insertionResult.first->second;
} else {
curBB = SGF.createBasicBlock(curBB);
caseBBs.push_back({elt, curBB});
caseInfos.resize(caseInfos.size() + 1);
caseInfos.back().FormalElement = formalElt;
caseInfos.back().FirstMatcher = row.Pattern;
caseCounts.push_back(row.Count);
}
assert(caseToIndex[elt] == index);
assert(caseBBs[index].first == elt);

auto &info = caseInfos[index];
info.Irrefutable = (info.Irrefutable || row.Irrefutable);
info.SpecializedRows.resize(info.SpecializedRows.size() + 1);
auto &specRow = info.SpecializedRows.back();
specRow.RowIndex = row.RowIndex;

// Use the row pattern, if it has one.
if (subPattern) {
specRow.Patterns.push_back(subPattern);
// It's also legal to write:
// case .Some { ... }
// which is an implicit wildcard.
} else {
specRow.Patterns.push_back(nullptr);
}
}

// We always need a default block if the enum is resilient.
// If the enum is @_fixed_layout, we only need one if the
// switch is not exhaustive.
bool exhaustive = false;
auto enumDecl = sourceType.getEnumOrBoundGenericEnum();

if (!enumDecl->isResilient(SGF.SGM.M.getSwiftModule(),
SGF.F.getResilienceExpansion())) {
exhaustive = true;

for (auto elt : enumDecl->getAllElements()) {
if (!caseToIndex.count(elt)) {
exhaustive = false;
break;
}
}
}

if (!exhaustive)
defaultBB = SGF.createBasicBlock(curBB);
}

assert(caseBBs.size() == caseInfos.size());
generateEnumCaseBlocks(SGF, rows, sourceType, SGF.B.getInsertionBB(),
caseBBs, caseCounts, caseInfos, defaultBB);

// Emit the switch_enum{_addr} instruction.
bool addressOnlyEnum = src.getType().isAddress();
Expand Down Expand Up @@ -2600,17 +2547,21 @@ void SILGenFunction::emitSwitchStmt(SwitchStmt *S) {
DEBUG(llvm::dbgs() << "emitting switch stmt\n";
S->print(llvm::dbgs());
llvm::dbgs() << '\n');
auto failure = [&](SILLocation location) {
// If we fail to match anything, we can just emit unreachable.
// This will be a dataflow error if we can reach here.
B.createUnreachable(S);
auto failure = [this](SILLocation location) {
// If we fail to match anything, we trap. This can happen with a switch
// over an @objc enum, which may contain any value of its underlying type.
// FIXME: Even if we can't say what the invalid value was, we should at
// least mention that this was because of a non-exhaustive enum.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jckarter, do you think it's okay to commit this part now and file a follow-up bug to provide a message? Or do you think that should go in at the same time?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's OK to do it piecemeal.

B.createBuiltinTrap(location);
B.createUnreachable(location);
};

// If the subject expression is uninhabited, we're already dead.
// Emit an unreachable in place of the switch statement.
if (S->getSubjectExpr()->getType()->isStructurallyUninhabited()) {
emitIgnoredExpr(S->getSubjectExpr());
return failure(SILLocation(S));
B.createUnreachable(S);
return;
}

auto completionHandler = [&](PatternMatchEmission &emission,
Expand Down
14 changes: 8 additions & 6 deletions test/IRGen/objc_enum_multi_file.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@ func useFoo(_ x: Foo) -> Int32 {
// CHECK-DAG: i32 0, label %[[CASE_A:.+]]
// CHECK: ]

// CHECK: <label>:[[DEFAULT]]
// CHECK-NEXT: unreachable

switch x {
// CHECK: <label>:[[CASE_B]]
// CHECK-NEXT: br label %[[FINAL:.+]]
Expand All @@ -36,6 +33,10 @@ func useFoo(_ x: Foo) -> Int32 {
return 10
}

// CHECK: <label>:[[DEFAULT]]
// CHECK-NEXT: call void @llvm.trap()
// CHECK-NEXT: unreachable

// CHECK: <label>:[[FINAL]]
// CHECK: %[[RETVAL:.+]] = phi i32 [ 10, %[[CASE_A]] ], [ 15, %[[CASE_C]] ], [ 11, %[[CASE_B]] ]
// CHECK: ret i32 %[[RETVAL]]
Expand All @@ -49,9 +50,6 @@ func useBar(_ x: Bar) -> Int32 {
// CHECK-DAG: i32 5, label %[[CASE_A:.+]]
// CHECK: ]

// CHECK: <label>:[[DEFAULT]]
// CHECK-NEXT: unreachable

switch x {
// CHECK: <label>:[[CASE_B]]
// CHECK-NEXT: br label %[[FINAL:.+]]
Expand All @@ -69,6 +67,10 @@ func useBar(_ x: Bar) -> Int32 {
return 10
}

// CHECK: <label>:[[DEFAULT]]
// CHECK-NEXT: call void @llvm.trap()
// CHECK-NEXT: unreachable

// CHECK: <label>:[[FINAL]]
// CHECK: %[[RETVAL:.+]] = phi i32 [ 10, %[[CASE_A]] ], [ 15, %[[CASE_C]] ], [ 11, %[[CASE_B]] ]
// CHECK: ret i32 %[[RETVAL]]
Expand Down
Loading