Support Linux on z as a Swift platform: Part 2 #3193
Conversation
…s and have guard branches.
| // RUN: %target-swift-frontend -emit-silgen -verify %s | ||
|
|
||
| enum testError: ErrorProtocol { | ||
| case C1 |
There was a problem hiding this comment.
We usually use 2 spaces of indentation in .swift files.
|
@slavapestov @rjmccall: could you review, please? |
| ArrayRef<CaseLabelItem> labelItems = stmt->getCaseLabelItems(); | ||
| bool hasMultipleItems = labelItems.size() > 1; | ||
| bool hasMultipleItems = false; | ||
| if (stmt->getKind() == StmtKind::Case) { |
There was a problem hiding this comment.
Calling getKind() is usually not the right thing to do. Here, you probably want if (isa<CaseStmt>(S)).
| auto stmt = clauses[row].getClientData<Stmt>(); | ||
| bool hasMultipleItems = false; | ||
| if (isa<CaseStmt>(stmt)) { | ||
| hasMultipleItems = dyn_cast<CaseStmt>(stmt)->getCaseLabelItems().size() > 1; |
There was a problem hiding this comment.
if (auto *CS = dyn_cast<CaseStmt>(stmt))
hasMultipleItems = CS->getCaseLabelItems().size() > 1;
|
@swift-ci Please test |
|
Thanks a lot for reviewing @gribozavr. |
| @@ -0,0 +1,26 @@ | |||
| // RUN: $target-swift-frontend -emit-silgen -verify %s | |||
There was a problem hiding this comment.
-verify is only there to check '// expected-error' that you see in Sema diagnostics tests. You don't need it here.
| }); | ||
| } else { | ||
| // For CatchStmt | ||
| emitGuardBranch(guardExpr, guardExpr, failure); |
There was a problem hiding this comment.
I'm still wondering why we need to branch—this is probably a hazard elsewhere in code that handles patterns too. I think it would be much more robust for SGF.usingImplicitVariablesForPattern to handle the caseStmt == nullptr case inside itself instead of delegating this responsibility to every call site, so this becomes:
SGF.usingImplicitVariablesForPattern(clauses[row].getCasePattern(), dyn_cast<CaseStmt>(stmt), [&]{
this->emitGuardBranch(guardExpr, guardExpr, failure);
});and SGF.usingImplicitVariablesForPattern has an early exit like:
if (!caseStmt)
// Call through to the closure directly.
return f();There was a problem hiding this comment.
Thanks @jckarter . Yes, the code is better with what you suggested and is added.
Fix new test case from commit fcaba99c81edcb8320856cb55367e1d8a9176675 Adding the review changes to swiftlang#3193
|
LGTM now, thanks! |
|
@swift-ci Please test and merge |
What's in this pull request?
Current code in emitWildcardDispatch() caters only for case statements, the catch statement in a do-catch block can also have wildcard patterns and it can get to the method.
In the case of a catch statement, emitWildcardDispatch() determines that clauses[row].getCaseGuardExpr() is non-null as well, and assumes that clauses[row] refers to a CaseStmt (which is not true; it refers to a CatchStmt instead), and eventually calls usingImplicitVariablesForPattern() to emit the guard branch.
usingImplicitVariablesForPattern() calls getCaseLabelItems() on the CatchStmt object, which returns garbage in a stack-allocated ArrayRef named labelItems. An assert is thrown when we try to read from labelItems[0]
This is a continuation effort from the previous pull request #2541. Our team encountered this issue during the build of swiftpm for s390x. However this issue is platform independent and the same assert is reproducible on x86_64 with the new test case added alongside with this commit.
Regression test has been run on both s390x and x86_64 with all the commits proposed here, no new issues found.
There is also a small s390x test enabling change on test/Prototypes/FloatingPoint.swift, test is passing in s390x after the change. (NOTE: This change is in #3240 already)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.