DI fixes for resilience #12386

slavapestov · 2017-10-11T06:36:36Z

Fixes https://bugs.swift.org/browse/SR-5649, rdar://problem/33767516, rdar://problem/33137910.

gottesmm

Metacomment: Please do not commit the less specific error messages (I realize this is WIP).

gottesmm · 2017-10-11T17:13:27Z

lib/SILOptimizer/Mandatory/DIMemoryUseCollectorOwnership.cpp

-  for (auto UI : MUI->getUses()) {
+void DelegatingInitElementUseCollector::collectValueTypeInitSelfUses(
+    SILInstruction *I) {
+  for (auto UI : I->getResults()[0]->getUses()) {


Please do not do this! If you truly want a SingleValueInstruction, dyn_cast to it. That being said, here you should be handling multiple results.

slavapestov · 2017-10-11T20:43:53Z

Thanks for the suggestion, I'll definitely fix up the multiple values usages. And of course the PR will get proper commit messages and a lot of cleanup before I merge.

Ok. I want to review this when you are done.

slavapestov · 2017-10-13T01:15:13Z

Still needs tests, and I have more diagnostics improvements pending.

slavapestov · 2017-10-13T01:17:57Z

@swift-ci Please smoke test

slavapestov · 2017-10-13T01:18:06Z

@swift-ci Please test source compatibility

slavapestov · 2017-10-13T01:18:24Z

@swift-ci Please test source compatibility

slavapestov · 2017-10-13T01:18:28Z

@swift-ci Please smoke test

slavapestov · 2017-10-13T22:09:14Z

Hey @jrose-apple @gottesmm this is ready for review.

There is a regression in diagnostics where sometimes we now point to the end location of a function instead of the 'return' in question when you return without initializing self. Technically this isn't a new problem, it was there all along, because the old logic was only happening with root initializers. Now that more initializers are delegating it's happening more.

Do you mind if I fix it in a follow-up PR? @jrose-apple needs the changes here for some resilience work he's doing.

slavapestov · 2017-10-13T22:09:28Z

@swift-ci Please test

slavapestov · 2017-10-13T22:09:33Z

@swift-ci Please test

slavapestov · 2017-10-13T22:09:39Z

@swift-ci Please test

slavapestov · 2017-10-13T22:09:47Z

@swift-ci Please test source compatibility

slavapestov · 2017-10-13T22:10:10Z

@swift-ci Please test Linux

jrose-apple

Thanks, Slava!

I'd like someone more familiar with DI to review the DI changes. I'm also unhappy with the bad diagnostics not being put in the file; we should put them in in their bad state and change them later.

jrose-apple · 2017-10-13T22:19:52Z

include/swift/AST/DiagnosticsSIL.def

+ERROR(self_before_selfinit,none,
+      "'self' used before 'self.init' call", ())
+ERROR(self_before_selfinit_value_type,none,
+      "'self' used before 'self.init' call or assignment to 'self'", ())


Nitpick: why have three separate messages here, when it's still a choice above?

Because the choice was keyed purely on whether its a delegating init or not, but I also want to distinguish the value type case so that diagnostics say that an assignment to self is OK too. I'll probably rework the other diagnostics when I do the fix for the return location analysis.

If you think they should still be a single diagnostic with a %select, I don't mind doing that. It seemed a bit silly to define a new enum that's only used in one place, but if I see other diagnostics get too repetitive I can do it.

jrose-apple · 2017-10-13T22:23:05Z

test/Interpreter/protocol_extensions.swift

@@ -1,6 +1,11 @@
-// RUN: %target-run-simple-swift | %FileCheck %s
+// RUN: %target-run-simple-swift


Once upon a time these were deliberately minimal tests, where if something broke in StdlibUnittest it wouldn't automatically break in these tests. Maybe that's not so relevant now, though.

Yeah, I understand the reasoning here, but I find StdlibUnittest easier to work with when a test fails, and also CHECK: lines are not totally reliable because it's easy to accidentally match the wrong thing. At one point Dmitri told me that new executable tests should use StdlibUnittest so I've been slowly converting them when I touch them.

jrose-apple · 2017-10-13T22:24:01Z

test/decl/init/resilience.swift

    self.w = ww
-    self.h = hh
+    self.h = hh // expected-error {{}}


What message do we get at the moment? It's worth including just so we can see when it changes.

We now get a Sema error saying that 'self.h' is a let property. The initializer is now delegating since it's inlinable, so we don't even get as far as DI here, instead we hit the restriction that delegating inits cannot assign to let properties at all.

This isn't the "bad diagnostic" case I was talking about; that's in the DI tests (look at the patch that makes all enum constructors delegating).

That should still be included, though. We can fix it later.

The enum thing does seem like something we can fix in a follow-up PR. We just need to not ship it!

…alling self.init

…ice versa This allows, for example, an initializer to conditionally assign to self or call self.init, along different control flow paths. It also means that it is legal to call self.init() multiple times in a value type initializer, but this... is fine. The old 'self' is destroyed. Fixes <rdar://problem/33137910>.

Previously protocol extension initializers which called 'self.init' were considered 'delegating', and ones that assign to 'self' were considered 'root'. Both have the same SIL lowering so the distinction is not useful, and removing it simplifies some code.

Again, since there's no distinction between an enum initializer that delegates to 'self.init' from one that assigns to 'self', we can remove the special handling of enum initializers in the 'root self' case. Now, 'root self' is only used for designated initializers in classes with no superclass, and struct initializers that perform memberwise initialization of stored properties. This regresses some diagnostics, because the logic for delegating init diagnostics is missing some heuristics present in the root self case. I will fix this in a subsequent patch.

Initializers for non-fixed-layout structs that are inlinable or are defined in a different module are treated as delegating initializers. Previously, only initializers containing a 'self.init' call were delegating; initializers that assigned to 'self' were not, which resulted in DI treating them as a root initializer where the stored 'self' value was exploded into a series of stores to each stored property member. They were not resilient as a result. Fixes <https://bugs.swift.org/browse/SR-5649>, <rdar://problem/33767516>.

…s-bound protocols

…n argument to self/super.init This fixes a regression from a previous patch, but it doesn't fully address <https://bugs.swift.org/browse/SR-5682>, because we now hit a SILGen crasher with the original test case.

slavapestov · 2017-10-14T06:52:16Z

@swift-ci Please test

slavapestov · 2017-10-14T06:52:22Z

@swift-ci Please test

slavapestov · 2017-10-14T06:52:37Z

@swift-ci Please test source compatibility

slavapestov · 2017-10-14T06:53:21Z

@swift-ci Please test source compatibility

slavapestov force-pushed the most-definitely-self branch from caf8aaa to ad9fa15 Compare October 11, 2017 06:36

gottesmm previously requested changes Oct 11, 2017

View reviewed changes

slavapestov force-pushed the most-definitely-self branch from ad9fa15 to ace1419 Compare October 13, 2017 01:14

slavapestov force-pushed the most-definitely-self branch from ace1419 to 201c08c Compare October 13, 2017 07:39

slavapestov changed the title ~~DI WIP~~ DI fixes for resilience Oct 13, 2017

jrose-apple reviewed Oct 13, 2017

View reviewed changes

slavapestov force-pushed the most-definitely-self branch 2 times, most recently from c021c14 to 79e7fcb Compare October 14, 2017 03:21

slavapestov added 9 commits October 13, 2017 23:51

DI: Consolidate and clean up some diagnostics

e515563

DI: Fix diagnostic for return from address-only initializer without c…

5ffbd28

…alling self.init

DI: Look through begin_access/end_access for assignment to self

9a7a1de

DI: Small cleanup now that SelfInit is only used for classes and clas…

94d4b5a

…s-bound protocols

DI: Remove shouldIgnoreClassMethodUseError()

ff34f3b

slavapestov added 4 commits October 13, 2017 23:51

DI: Fold handleSuperInitUse() into handleSelfInitUse()

421797c

DI: Kill off collectBorrowedSuperUses()

1bcfc50

DI: Kill off checkClassSelfUpcastUsedBySuperInit()

1feaf9e

slavapestov force-pushed the most-definitely-self branch from b9b4a78 to d8153b3 Compare October 14, 2017 06:51

slavapestov merged commit 1f84641 into swiftlang:master Oct 14, 2017

slavapestov mentioned this pull request Oct 14, 2017

[SR-5649] Value type initializers that assign to 'self' should be resilient #48219

Closed

		@@ -1,6 +1,11 @@
		// RUN: %target-run-simple-swift \| %FileCheck %s
		// RUN: %target-run-simple-swift

DI fixes for resilience #12386

DI fixes for resilience #12386

Uh oh!

Conversation

slavapestov commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Oct 11, 2017

Uh oh!

slavapestov commented Oct 13, 2017

Uh oh!

slavapestov commented Oct 13, 2017

Uh oh!

slavapestov commented Oct 13, 2017

Uh oh!

slavapestov commented Oct 13, 2017

Uh oh!

slavapestov commented Oct 13, 2017

Uh oh!

slavapestov commented Oct 13, 2017

Uh oh!

slavapestov commented Oct 13, 2017

Uh oh!

slavapestov commented Oct 13, 2017

Uh oh!

slavapestov commented Oct 13, 2017

Uh oh!

slavapestov commented Oct 13, 2017

Uh oh!

slavapestov commented Oct 13, 2017

Uh oh!

jrose-apple 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!

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!

slavapestov commented Oct 14, 2017

Uh oh!

slavapestov commented Oct 14, 2017

Uh oh!

slavapestov commented Oct 14, 2017

Uh oh!

slavapestov commented Oct 14, 2017

Uh oh!

Uh oh!

slavapestov commented Oct 11, 2017 •

edited

Loading