Skip to content
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

Jump to bottom

Fix the evil fallthrough bug #23687

Merged
merged 3 commits into from
Apr 4, 2019
Merged

Fix the evil fallthrough bug #23687

merged 3 commits into from
Apr 4, 2019

Conversation

gottesmm
Copy link
Contributor

This PR contains two helper PRs that refactor the code slightly and a final last PR that wires up the case body var decl code to SILGenPattern.

rdar://47467128

@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

@swift-ci test compiler performance

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 1725d7a761f5bd96e0170345d32ec7b342858c3e

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 1725d7a761f5bd96e0170345d32ec7b342858c3e

@swift-ci
Copy link
Contributor

Compilation-performance test failed

@gottesmm gottesmm force-pushed the pr-aa1beb8a78e3e8b7772bb6e273d51f214e0aa30d branch from 1725d7a to 55492f1 Compare March 31, 2019 17:46
@gottesmm
Copy link
Contributor Author

@swift-ci test

2 similar comments
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

@swift-ci test compiler performance

2 similar comments
@gottesmm
Copy link
Contributor Author

@swift-ci test compiler performance

@gottesmm
Copy link
Contributor Author

@swift-ci test compiler performance

@gottesmm
Copy link
Contributor Author

@swift-ci test source compatibility

1 similar comment
@gottesmm
Copy link
Contributor Author

@swift-ci test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 1725d7a761f5bd96e0170345d32ec7b342858c3e

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 1725d7a761f5bd96e0170345d32ec7b342858c3e

@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 1, 2019

For some reason the performance job is not posting the comment AFAIKT. I got it directly from the bot. It is going to be my next post.

@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 1, 2019

Summary for master full

Unexpected test results, excluded stats for CoreStore, Tagged, Wordy, Deferred, ProcedureKit

Regressions found (see below)

Debug-batch

debug-batch brief

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (3)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 27,391,005,897,762 27,308,254,571,690 -82,751,326,072 -0.3%
LLVM.NumLLVMBytesOutput 972,300,152 972,298,662 -1,490 -0.0%
time.swift-driver.wall 2619.7s 2599.0s -20.7s -0.79%

debug-batch detailed

Regressed (1)
name old new delta delta_pct
Driver.NumDriverPipeReads 176,218 178,788 2,570 1.46% ⛔
Improved (1)
name old new delta delta_pct
Sema.USRGenerationRequest 12,604,081 12,456,240 -147,841 -1.17% ✅
Unchanged (delta < 1.0% or delta < 100.0ms) (98)
name old new delta delta_pct
AST.NumASTBytesAllocated 67,543,297,810 67,269,627,945 -273,669,865 -0.41%
AST.NumDecls 80,786 80,786 0 0.0%
AST.NumDependencies 205,072 205,072 0 0.0%
AST.NumImportedExternalDefinitions 1,092,562 1,092,562 0 0.0%
AST.NumInfixOperators 31,080 31,080 0 0.0%
AST.NumLinkLibraries 0 0 0 0.0%
AST.NumLoadedModules 244,881 244,881 0 0.0%
AST.NumLocalTypeDecls 108 108 0 0.0%
AST.NumObjCMethods 9,724 9,724 0 0.0%
AST.NumPostfixOperators 18 18 0 0.0%
AST.NumPrecedenceGroups 14,514 14,514 0 0.0%
AST.NumPrefixOperators 70 70 0 0.0%
AST.NumReferencedDynamicNames 92 92 0 0.0%
AST.NumReferencedMemberNames 3,511,542 3,511,542 0 0.0%
AST.NumReferencedTopLevelNames 271,022 271,022 0 0.0%
AST.NumSourceBuffers 331,664 331,664 0 0.0%
AST.NumSourceLines 2,432,547 2,432,547 0 0.0%
AST.NumSourceLinesPerSecond 2,160,326 2,159,532 -794 -0.04%
AST.NumTotalClangImportedEntities 4,126,996 4,117,811 -9,185 -0.22%
AST.NumUsedConformances 226,385 226,385 0 0.0%
Driver.ChildrenMaxRSS 106,809,978,880 106,860,933,120 50,954,240 0.05%
Driver.DriverDepCascadingDynamic 0 0 0 0.0%
Driver.DriverDepCascadingExternal 0 0 0 0.0%
Driver.DriverDepCascadingMember 0 0 0 0.0%
Driver.DriverDepCascadingNominal 0 0 0 0.0%
Driver.DriverDepCascadingTopLevel 0 0 0 0.0%
Driver.DriverDepDynamic 0 0 0 0.0%
Driver.DriverDepExternal 0 0 0 0.0%
Driver.DriverDepMember 0 0 0 0.0%
Driver.DriverDepNominal 0 0 0 0.0%
Driver.DriverDepTopLevel 0 0 0 0.0%
Driver.NumDriverJobsRun 16,205 16,205 0 0.0%
Driver.NumDriverJobsSkipped 0 0 0 0.0%
Driver.NumDriverPipePolls 159,800 161,170 1,370 0.86%
Driver.NumProcessFailures 0 0 0 0.0%
Frontend.MaxMallocUsage 691,782,496,840 690,548,053,896 -1,234,442,944 -0.18%
Frontend.NumInstructionsExecuted 27,391,005,897,762 27,308,254,571,690 -82,751,326,072 -0.3%
Frontend.NumProcessFailures 0 0 0 0.0%
IRModule.NumIRAliases 105,540 105,540 0 0.0%
IRModule.NumIRBasicBlocks 3,849,136 3,849,136 0 0.0%
IRModule.NumIRComdatSymbols 0 0 0 0.0%
IRModule.NumIRFunctions 1,804,555 1,804,555 0 0.0%
IRModule.NumIRGlobals 1,857,056 1,857,056 0 0.0%
IRModule.NumIRIFuncs 0 0 0 0.0%
IRModule.NumIRInsts 47,456,421 47,456,421 0 0.0%
IRModule.NumIRNamedMetaData 78,402 78,402 0 0.0%
IRModule.NumIRValueSymbols 3,302,061 3,302,061 0 0.0%
LLVM.NumLLVMBytesOutput 972,300,152 972,298,662 -1,490 -0.0%
Parse.NumFunctionsParsed 142,585 142,585 0 0.0%
Parse.NumIterableDeclContextParsed 1,004,447 1,004,447 0 0.0%
SILModule.NumSILGenDefaultWitnessTables 0 0 0 0.0%
SILModule.NumSILGenFunctions 932,961 932,961 0 0.0%
SILModule.NumSILGenGlobalVariables 37,189 37,189 0 0.0%
SILModule.NumSILGenVtables 10,694 10,694 0 0.0%
SILModule.NumSILGenWitnessTables 39,362 39,362 0 0.0%
SILModule.NumSILOptDefaultWitnessTables 0 0 0 0.0%
SILModule.NumSILOptFunctions 1,326,972 1,326,972 0 0.0%
SILModule.NumSILOptGlobalVariables 37,947 37,947 0 0.0%
SILModule.NumSILOptVtables 17,207 17,207 0 0.0%
SILModule.NumSILOptWitnessTables 86,613 86,613 0 0.0%
Sema.AccessLevelRequest 2,337,053 2,335,562 -1,491 -0.06%
Sema.CustomAttrNominalRequest 0 0 0 0.0%
Sema.DefaultAndMaxAccessLevelRequest 54,591 54,581 -10 -0.02%
Sema.DefaultTypeRequest 318,780 318,780 0 0.0%
Sema.EnumRawTypeRequest 17,024 17,024 0 0.0%
Sema.ExtendedNominalRequest 3,736,770 3,732,306 -4,464 -0.12%
Sema.InheritedDeclsReferencedRequest 4,285,044 4,264,978 -20,066 -0.47%
Sema.InheritedTypeRequest 528,333 528,188 -145 -0.03%
Sema.IsDynamicRequest 1,828,610 1,828,610 0 0.0%
Sema.IsObjCRequest 1,565,510 1,565,195 -315 -0.02%
Sema.MangleLocalTypeDeclRequest 216 216 0 0.0%
Sema.NamedLazyMemberLoadFailureCount 18,897 18,821 -76 -0.4%
Sema.NamedLazyMemberLoadSuccessCount 17,452,639 17,453,228 589 0.0%
Sema.NominalTypeLookupDirectCount 29,198,089 29,161,026 -37,063 -0.13%
Sema.NumConformancesDeserialized 6,533,956 6,503,177 -30,779 -0.47%
Sema.NumConstraintScopes 15,052,515 15,048,890 -3,625 -0.02%
Sema.NumConstraintsConsideredForEdgeContraction 41,335,760 41,334,392 -1,368 -0.0%
Sema.NumDeclsDeserialized 48,020,922 47,812,921 -208,001 -0.43%
Sema.NumDeclsFinalized 1,644,178 1,644,178 0 0.0%
Sema.NumDeclsTypechecked 895,635 895,635 0 0.0%
Sema.NumDeclsValidated 1,945,073 1,945,069 -4 -0.0%
Sema.NumFunctionsTypechecked 949,206 949,206 0 0.0%
Sema.NumGenericSignatureBuilders 1,146,242 1,144,494 -1,748 -0.15%
Sema.NumLazyGenericEnvironments 9,705,972 9,693,103 -12,869 -0.13%
Sema.NumLazyGenericEnvironmentsLoaded 195,445 195,401 -44 -0.02%
Sema.NumLazyIterableDeclContexts 6,708,618 6,701,238 -7,380 -0.11%
Sema.NumLeafScopes 9,802,953 9,799,953 -3,000 -0.03%
Sema.NumTypesDeserialized 16,142,805 16,112,754 -30,051 -0.19%
Sema.NumTypesValidated 1,316,471 1,316,466 -5 -0.0%
Sema.NumUnloadedLazyIterableDeclContexts 4,406,287 4,409,363 3,076 0.07%
Sema.OverriddenDeclsRequest 7,538,555 7,474,221 -64,334 -0.85%
Sema.RequirementRequest 62,179 62,180 1 0.0%
Sema.SelfBoundsFromWhereClauseRequest 6,242,555 6,217,495 -25,060 -0.4%
Sema.SetterAccessLevelRequest 137,533 137,533 0 0.0%
Sema.SuperclassDeclRequest 66,860 66,796 -64 -0.1%
Sema.SuperclassTypeRequest 30,796 30,796 0 0.0%
Sema.TypeDeclsFromWhereClauseRequest 29,939 29,929 -10 -0.03%
Sema.UnderlyingTypeDeclsReferencedRequest 145,957 145,575 -382 -0.26%

Release

release brief

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (3)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 25,573,686,262,485 25,584,346,383,395 10,660,120,910 0.04%
LLVM.NumLLVMBytesOutput 779,850,458 779,851,418 960 0.0%
time.swift-driver.wall 4600.7s 4609.8s 9.0s 0.2%

release detailed

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
name old new delta delta_pct
AST.NumImportedExternalDefinitions 216,559 216,559 0 0.0%
AST.NumLoadedModules 16,492 16,492 0 0.0%
AST.NumTotalClangImportedEntities 736,667 736,667 0 0.0%
AST.NumUsedConformances 227,331 227,331 0 0.0%
IRModule.NumIRBasicBlocks 3,234,122 3,234,122 0 0.0%
IRModule.NumIRFunctions 1,486,895 1,486,895 0 0.0%
IRModule.NumIRGlobals 1,622,639 1,622,639 0 0.0%
IRModule.NumIRInsts 29,381,150 29,381,150 0 0.0%
IRModule.NumIRValueSymbols 2,896,376 2,896,376 0 0.0%
LLVM.NumLLVMBytesOutput 779,850,458 779,851,418 960 0.0%
SILModule.NumSILGenFunctions 653,152 653,152 0 0.0%
SILModule.NumSILOptFunctions 889,962 889,962 0 0.0%
Sema.NumConformancesDeserialized 2,233,184 2,233,184 0 0.0%
Sema.NumConstraintScopes 13,401,956 13,401,956 0 0.0%
Sema.NumDeclsDeserialized 6,020,091 6,020,091 0 0.0%
Sema.NumDeclsValidated 1,038,655 1,038,655 0 0.0%
Sema.NumFunctionsTypechecked 428,737 428,737 0 0.0%
Sema.NumGenericSignatureBuilders 190,265 190,265 0 0.0%
Sema.NumLazyGenericEnvironments 1,244,822 1,244,822 0 0.0%
Sema.NumLazyGenericEnvironmentsLoaded 21,173 21,173 0 0.0%
Sema.NumLazyIterableDeclContexts 765,629 765,629 0 0.0%
Sema.NumTypesDeserialized 3,185,724 3,185,724 0 0.0%
Sema.NumTypesValidated 616,421 616,421 0 0.0%

slavapestov
slavapestov reviewed Apr 3, 2019
View reviewed changes
lib/AST/Decl.cpp Show resolved Hide resolved
lib/Parse/ParseStmt.cpp
// If we had any bad redefinitions, we already diagnosed them against the
// first case label item.
if (v->hasName())
addToScope(v, false /*diagnoseRedefinitions*/);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always going to be a redefinition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a new entry point, like addRedefinitionToScope(), that changes the logic around and asserts if its not a redefinition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM

Copy link
Contributor Author

@gottesmm gottesmm Apr 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Actually that is not what we want. We actually just do not want
to emit the error here. The reason why is that we are not always
going to have a redefinition But in the case where we do have the
redefinition, we only want to emit a single error (the one from
the case label). That is why for the case body decls I have wired
through the flag to ignore the error /if/ it occurs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@slavapestov ping?

lib/Sema/MiscDiagnostics.cpp Outdated Show resolved Hide resolved
@gottesmm gottesmm force-pushed the pr-aa1beb8a78e3e8b7772bb6e273d51f214e0aa30d branch from 55492f1 to 6f30bc0 Compare April 3, 2019 21:10
@gottesmm gottesmm requested a review from slavapestov April 4, 2019 00:35
slavapestov
slavapestov approved these changes Apr 4, 2019
View reviewed changes
lib/SILGen/SILGenPattern.cpp Outdated Show resolved Hide resolved
@gottesmm gottesmm force-pushed the pr-aa1beb8a78e3e8b7772bb6e273d51f214e0aa30d branch from 6f30bc0 to a307db0 Compare April 4, 2019 06:39
@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 4, 2019

@swift-ci smoke test and merge

2 similar comments
@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 4, 2019

@swift-ci smoke test and merge

@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 4, 2019

@swift-ci smoke test and merge

@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 4, 2019

@swift-ci smoke test and merge

@gottesmm
[ast][silgen] Wire up the case body var decls and use them in SILGenP…
7b0d845 
…attern emission to fix the evil fallthrough bug.

rdar://47467128
@gottesmm gottesmm force-pushed the pr-aa1beb8a78e3e8b7772bb6e273d51f214e0aa30d branch from a307db0 to 7b0d845 Compare April 4, 2019 06:51
@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 4, 2019

@swift-ci smoke test and merge

4 similar comments
@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 4, 2019

@swift-ci smoke test and merge

@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 4, 2019

@swift-ci smoke test and merge

@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 4, 2019

@swift-ci smoke test and merge

@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 4, 2019

@swift-ci smoke test and merge

@swift-ci swift-ci merged commit 0205150 into swiftlang:master Apr 4, 2019
@gottesmm gottesmm deleted the pr-aa1beb8a78e3e8b7772bb6e273d51f214e0aa30d branch April 4, 2019 13:35
@rintaro
Copy link
Member

rintaro commented Apr 4, 2019

@gottesmm This is causing heap-use-after-free in internal ASAN bot. rdar://problem/49609717

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slavapestov slavapestov slavapestov approved these changes

@jckarter jckarter Awaiting requested review from jckarter

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gottesmm @swift-ci @rintaro @slavapestov