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

jrose-apple · 2018-02-20T00:17:12Z

(both C enums and Swift enums declared @objc), because of the "feature" in C of treating a value not declared as a case as a valid value of an enum. No more undefined behavior here!

This bit can go in separately from all the work on exhaustive/frozen enums, which is still being discussed and will come later.

rdar://problem/20420436

jrose-apple · 2018-02-20T00:19:08Z

lib/SILGen/SILGenPattern.cpp

-  SILBasicBlock *defaultBB = nullptr;
+/// Create destination blocks for switching over the cases in an enum defined
+/// by \p rows.
+static void generateEnumCaseBlocks(


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.

Two things:

Can you put the refactoring in a separate commit? That will make the diff even /easier/ to read than whitespace-insensitive diff.

I purposely duplicated the code so I could easily visualize the difference in between the two.

Fair, will do.

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

@jrose-apple on 2:

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.

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.

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

jrose-apple · 2018-02-20T00:20:38Z

lib/SILGen/SILGenPattern.cpp

  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);


@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?

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

jrose-apple · 2018-02-20T00:21:27Z

lib/SILGen/SILGenPattern.cpp

+    // 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.


@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?

I think it's OK to do it piecemeal.

jrose-apple · 2018-02-20T00:22:10Z

@swift-ci Please test

jckarter · 2018-02-20T00:40:33Z

Looks good. Do you have any tests for trapping on @objc enums in nested pattern positions?

jrose-apple · 2018-02-20T00:59:45Z

Ooh, good call. I'll add one now.

jrose-apple · 2018-02-20T01:13:26Z

@swift-ci Please test

swift-ci · 2018-02-20T01:13:54Z

Build failed
Swift Test OS X Platform
Git Sha - 05a092f891815708efab04af43967b20aa7b9d0a

swift-ci · 2018-02-20T01:14:59Z

Build failed
Swift Test Linux Platform
Git Sha - 05a092f891815708efab04af43967b20aa7b9d0a

jrose-apple · 2018-02-20T01:18:26Z

Simple code size test:

@swift-ci Please test compiler performance

jrose-apple · 2018-02-20T01:23:13Z

@swift-ci Please test compiler performance

gottesmm

Can you move the refactoring into a separate commit/pr? Also a quick comment about a DenseMap.

gottesmm · 2018-02-20T18:35:35Z

lib/SILGen/SILGenPattern.cpp

-  SILBasicBlock *defaultBB = nullptr;
+/// Create destination blocks for switching over the cases in an enum defined
+/// by \p rows.
+static void generateEnumCaseBlocks(


Two things:

Can you put the refactoring in a separate commit? That will make the diff even /easier/ to read than whitespace-insensitive diff.

I purposely duplicated the code so I could easily visualize the difference in between the two.

gottesmm · 2018-02-20T18:37:06Z

lib/SILGen/SILGenPattern.cpp

-      } else {
-        specRow.Patterns.push_back(nullptr);
-      }
+  llvm::DenseMap<EnumElementDecl*, unsigned> caseToIndex;


Can you change this to be a SmallDenseMap? DenseMap seems really heavy weight here.

This isn't a change from what was there before, but sure. (Makes sense, most enums are small.)

Yes. You are correct.

swift-ci · 2018-02-20T20:20:25Z

Build comment file:

Summary for master full

Unexpected test results, stats may be off for FAIL_Kronos-Kronos.xcodeproj_3.0_BuildXcodeProjectTarget_Kronos_generic-platform-tvOS.log, FAIL_RxDataSources-Pods-Pods.xcodeproj_3.0_BuildXcodeProjectTarget_Pods-RxDataSources_generic-platform-iOS.log, FAIL_Kronos-Kronos.xcodeproj_3.0_BuildXcodeProjectTarget_Kronos_generic-platform-iOS.log, FAIL_JSQDataSourcesKit-JSQDataSourcesKit.xcodeproj_4.0_BuildXcodeProjectTarget_JSQDataSourcesKit-iOS_generic-platform-iOS.log, FAIL_Dollar-Dollar.xcodeproj_3.0_BuildXcodeProjectScheme_Dollar_generic-platform-macOS.log, 3, FAIL_Kronos-Kronos.xcodeproj_3.0_BuildXcodeProjectTarget_Kronos_generic-platform-macOS.log, FAIL_RxDataSources-Pods-Pods.xcodeproj_3.0_BuildXcodeProjectTarget_Pods-Example_generic-platform-iOS.log

Regressions found (see below)

Debug
- debug brief
- debug detailed
Debug-opt
- debug-opt brief
- debug-opt detailed
Wmo-onone
- wmo-onone brief
- wmo-onone detailed
Release
- release brief
- release detailed

Debug

debug brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (1)

name	old	new	delta	delta_pct
time.swift-driver.wall	2044.9s	2013.7s	-31.2s	-1.53% ✅

Unchanged (delta < 1.0% or delta < 100.0ms) (1)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	1,067,627,162	1,067,518,646	-108,516	-0.01%

debug 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	1,670,241	1,669,684	-557	-0.03%
AST.NumLoadedModules	334,291	334,247	-44	-0.01%
AST.NumTotalClangImportedEntities	5,198,400	5,197,068	-1,332	-0.03%
AST.NumUsedConformances	152,080	152,036	-44	-0.03%
IRModule.NumIRBasicBlocks	3,301,365	3,300,829	-536	-0.02%
IRModule.NumIRFunctions	1,673,880	1,673,840	-40	-0.0%
IRModule.NumIRGlobals	1,575,413	1,575,390	-23	-0.0%
IRModule.NumIRInsts	34,747,709	34,742,717	-4,992	-0.01%
IRModule.NumIRValueSymbols	2,890,372	2,890,286	-86	-0.0%
LLVM.NumLLVMBytesOutput	1,067,627,162	1,067,518,646	-108,516	-0.01%
SILModule.NumSILGenFunctions	1,020,139	1,020,069	-70	-0.01%
SILModule.NumSILOptFunctions	1,511,713	1,511,866	153	0.01%
Sema.NumConformancesDeserialized	6,082,529	6,082,927	398	0.01%
Sema.NumConstraintScopes	11,013,658	11,010,814	-2,844	-0.03%
Sema.NumDeclsDeserialized	48,946,146	48,942,941	-3,205	-0.01%
Sema.NumDeclsValidated	2,107,153	2,105,110	-2,043	-0.1%
Sema.NumFunctionsTypechecked	1,014,629	1,014,164	-465	-0.05%
Sema.NumGenericSignatureBuilders	1,671,067	1,670,574	-493	-0.03%
Sema.NumLazyGenericEnvironments	9,550,932	9,549,935	-997	-0.01%
Sema.NumLazyGenericEnvironmentsLoaded	908,794	908,659	-135	-0.01%
Sema.NumLazyIterableDeclContexts	7,641,764	7,640,570	-1,194	-0.02%
Sema.NumTypesDeserialized	51,191,247	51,187,138	-4,109	-0.01%
Sema.NumTypesValidated	5,268,221	5,265,255	-2,966	-0.06%

Debug-opt

debug-opt brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (1)

name	old	new	delta	delta_pct
time.swift-driver.wall	3569.0s	3427.0s	-142.0s	-3.98% ✅

Unchanged (delta < 1.0% or delta < 100.0ms) (1)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	1,041,481,389	1,041,614,154	132,765	0.01%

debug-opt 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	1,661,549	1,661,576	27	0.0%
AST.NumLoadedModules	320,311	320,321	10	0.0%
AST.NumTotalClangImportedEntities	5,354,286	5,354,473	187	0.0%
AST.NumUsedConformances	151,323	151,326	3	0.0%
IRModule.NumIRBasicBlocks	3,579,613	3,581,010	1,397	0.04%
IRModule.NumIRFunctions	1,331,039	1,331,143	104	0.01%
IRModule.NumIRGlobals	1,325,703	1,325,747	44	0.0%
IRModule.NumIRInsts	28,088,090	28,092,518	4,428	0.02%
IRModule.NumIRValueSymbols	2,424,944	2,425,085	141	0.01%
LLVM.NumLLVMBytesOutput	1,041,481,389	1,041,614,154	132,765	0.01%
SILModule.NumSILGenFunctions	1,018,167	1,018,187	20	0.0%
SILModule.NumSILOptFunctions	2,068,631	2,068,818	187	0.01%
Sema.NumConformancesDeserialized	12,810,652	12,811,606	954	0.01%
Sema.NumConstraintScopes	10,981,795	10,982,228	433	0.0%
Sema.NumDeclsDeserialized	55,141,401	55,143,072	1,671	0.0%
Sema.NumDeclsValidated	2,089,642	2,089,428	-214	-0.01%
Sema.NumFunctionsTypechecked	1,008,713	1,008,746	33	0.0%
Sema.NumGenericSignatureBuilders	1,716,693	1,716,720	27	0.0%
Sema.NumLazyGenericEnvironments	10,627,235	10,627,502	267	0.0%
Sema.NumLazyGenericEnvironmentsLoaded	927,284	927,310	26	0.0%
Sema.NumLazyIterableDeclContexts	8,004,058	8,004,247	189	0.0%
Sema.NumTypesDeserialized	59,618,512	59,620,395	1,883	0.0%
Sema.NumTypesValidated	5,242,432	5,242,366	-66	-0.0%

Wmo-onone

wmo-onone 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) (2)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	965,042,997	965,045,889	2,892	0.0%
time.swift-driver.wall	1576.1s	1571.7s	-4.4s	-0.28%

wmo-onone 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	200,610	200,610	0	0.0%
AST.NumLoadedModules	11,705	11,705	0	0.0%
AST.NumTotalClangImportedEntities	633,987	633,987	0	0.0%
AST.NumUsedConformances	155,477	155,477	0	0.0%
IRModule.NumIRBasicBlocks	2,655,965	2,655,965	0	0.0%
IRModule.NumIRFunctions	1,414,234	1,414,308	74	0.01%
IRModule.NumIRGlobals	1,296,668	1,296,668	0	0.0%
IRModule.NumIRInsts	30,830,068	30,830,334	266	0.0%
IRModule.NumIRValueSymbols	2,433,085	2,433,159	74	0.0%
LLVM.NumLLVMBytesOutput	965,042,997	965,045,889	2,892	0.0%
SILModule.NumSILGenFunctions	556,887	556,887	0	0.0%
SILModule.NumSILOptFunctions	620,399	620,399	0	0.0%
Sema.NumConformancesDeserialized	1,422,843	1,422,843	0	0.0%
Sema.NumConstraintScopes	10,328,296	10,328,296	0	0.0%
Sema.NumDeclsDeserialized	4,893,403	4,893,403	0	0.0%
Sema.NumDeclsValidated	987,195	987,195	0	0.0%
Sema.NumFunctionsTypechecked	313,083	313,083	0	0.0%
Sema.NumGenericSignatureBuilders	177,808	177,808	0	0.0%
Sema.NumLazyGenericEnvironments	837,776	837,776	0	0.0%
Sema.NumLazyGenericEnvironmentsLoaded	102,690	102,690	0	0.0%
Sema.NumLazyIterableDeclContexts	529,776	529,776	0	0.0%
Sema.NumTypesDeserialized	5,038,204	5,038,204	0	0.0%
Sema.NumTypesValidated	1,303,834	1,303,834	0	0.0%

Release

release brief

None

release detailed

None

jrose-apple · 2018-02-21T00:56:49Z

Already passed full-testing previously, so just doing a smoke test now.

@swift-ci Please smoke test

jrose-apple · 2018-02-21T01:15:49Z

Filed rdar://problem/37728359 to improve the error message and assigned it to myself.

jrose-apple · 2018-02-21T03:13:02Z

@swift-ci Please smoke test

This code is shared between emitEnumElementDispatch and emitEnumElementDispatchWithOwnership. Eventually the former will go away, but for now there's no need to have both copies (which really are the same). No intended functionality change.

@objc

(both C enums and Swift enums declared @objc), because of the "feature" in C of treating a value not declared as a case as a valid value of an enum. No more undefined behavior here! This bit can go in separately from all the work on exhaustive/frozen enums, which is still being discussed and will come later. rdar://problem/20420436

Noticed by MichaelG while reviewing my changes in the previous commit. No intended functionality change.

jrose-apple · 2018-02-21T18:37:29Z

@swift-ci Please smoke test

jrose-apple requested review from jckarter and gottesmm February 20, 2018 00:17

jrose-apple commented Feb 20, 2018

View reviewed changes

jrose-apple force-pushed the obligatory-its-a-trap-joke branch from 05a092f to 963fbe5 Compare February 20, 2018 01:13

gottesmm reviewed Feb 20, 2018

View reviewed changes

jrose-apple force-pushed the obligatory-its-a-trap-joke branch from 963fbe5 to c2cb9d1 Compare February 21, 2018 00:56

jrose-apple added 3 commits February 21, 2018 10:34

[SILGen] Minor improvements to generateEnumCaseBlocks

49e71c9

Noticed by MichaelG while reviewing my changes in the previous commit. No intended functionality change.

jrose-apple force-pushed the obligatory-its-a-trap-joke branch from c2cb9d1 to 49e71c9 Compare February 21, 2018 18:35

jrose-apple merged commit 3f944d0 into swiftlang:master Feb 21, 2018

jrose-apple deleted the obligatory-its-a-trap-joke branch February 21, 2018 20:46

This was referenced Mar 20, 2018

Implementation for /most/ of SE-0192 (frozen and non-frozen enums) #14945

Merged

SILGenPattern cleanup #15554

Merged

jrose-apple mentioned this pull request Mar 30, 2018

[SILGen] Show a message when an unexpected enum value is switched on #15614

Merged

jrose-apple mentioned this pull request Jul 31, 2018

[SIL] Don't drop a default when switching on a non-exhaustive enum #18408

Merged

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

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

Uh oh!

Conversation

jrose-apple commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Feb 20, 2018

Uh oh!

jckarter commented Feb 20, 2018

Uh oh!

jrose-apple commented Feb 20, 2018

Uh oh!

jrose-apple commented Feb 20, 2018

Uh oh!

swift-ci commented Feb 20, 2018

Uh oh!

swift-ci commented Feb 20, 2018

Uh oh!

jrose-apple commented Feb 20, 2018

Uh oh!

jrose-apple commented Feb 20, 2018

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Feb 20, 2018

Build comment file:

Summary for master full

Debug

debug brief

debug detailed

Debug-opt

debug-opt brief

debug-opt detailed

Wmo-onone

wmo-onone brief

wmo-onone detailed

Release

release brief

release detailed

Uh oh!

jrose-apple commented Feb 21, 2018

Uh oh!

jrose-apple commented Feb 21, 2018

Uh oh!

jrose-apple commented Feb 21, 2018

Uh oh!

jrose-apple commented Feb 21, 2018

Uh oh!

Uh oh!

jrose-apple Feb 20, 2018 •

edited

Loading