Fix SE-0025 edge case involving public member of protocol extension of private protocol #15735

slavapestov · 2018-04-04T09:20:03Z

The witness table for a conformance to a private protocol is private. For this reason any member of a protocol extension of such a protocol should also be private, since it expects to receive a witness table and therefore there's no way to call it from the outside.

However, the type checker was incorrectly allowing such members to be found by name lookup if a public type conformed to the private protocol. Calling such a member resulted in a linker error.

The type checker also allowed these members to witness protocol requirements of public protocols. Since we cannot break source compatibility, this behavior is preserved. When calling the protocol requirement from outside of the module with the concrete base type, we used to fail with a linker error; now we call the requirement virtually using a witness_method instruction.

Fixes rdar://problem/21380336, rdar://problem/29044612, let's go search JIRA for dupes... https://bugs.swift.org/browse/SR-2315, https://bugs.swift.org/browse/SR-2571, https://bugs.swift.org/browse/SR-2925, https://bugs.swift.org/browse/SR-3312, https://bugs.swift.org/browse/SR-6325, damn...

slavapestov · 2018-04-05T01:30:44Z

@swift-ci Please smoke test

slavapestov · 2018-04-05T01:33:15Z

@jrose-apple You might to take a look at this PR.

@gottesmm If you feel like it, take a look at the SILOptimizer change. This was a latent bug exposed by my changes.

gottesmm · 2018-04-05T04:46:42Z

lib/SILOptimizer/Transforms/Devirtualizer.cpp

-      cast<SingleValueInstruction>(AI)->replaceAllUsesWith(NewInstPair.first);
-
-    DeadApplies.push_back(AI);
+    replaceDeadApply(Apply, NewInstPair.first);


This is not code that I am that familiar with. But are you 200% sure that you are not going to run into any problems around iterator invalidation and or removing values still with uses. That is the reason why people usually follow this pattern.

Other parts of the optimizer use replaceDeadApply(), so if that was wrong they would be wrong too. Note that we don't have any active iterators in this loop. We're iterating over a vector of SILInstructions.

I am not saying that using replaceDeadArray is necessarily wrong. What I am saying is that you are changing the code in a functional way by moving the time the applies are deleted from after the transformation to during the transformation. I am trying to prove to myself that doing so is safe (the pattern you are removing is a common defensive coding pattern in optimization passes). When I look at the git history these lines were added in the initial pass. @rudkx do you remember why you did this here?

We'll see if the source compat suite passes :)

That doesn't necessarily mean anything.

gottesmm · 2018-04-05T04:47:09Z

test/SILOptimizer/mandatory_inlining_devirt.swift

@@ -30,3 +30,18 @@ public class PublicClass {
  }
 }

+public protocol Thrower {


This test needs FileCheck patterns.

It has them, see the function caller()?

I don't know what you are talking about. I see caller on line 45 without a FileCheck pattern showing that the callee was or was not devirtualized. Or am I misreading the test?

You're right, fixed

gottesmm · 2018-04-05T05:13:01Z

test/SILOptimizer/mandatory_inlining_devirt.swift

@@ -30,3 +30,18 @@ public class PublicClass {
  }
 }

+public protocol Thrower {


I don't know what you are talking about. I see caller on line 45 without a FileCheck pattern showing that the callee was or was not devirtualized. Or am I misreading the test?

gottesmm · 2018-04-05T05:23:10Z

lib/SILOptimizer/Transforms/Devirtualizer.cpp

-      cast<SingleValueInstruction>(AI)->replaceAllUsesWith(NewInstPair.first);
-
-    DeadApplies.push_back(AI);
+    replaceDeadApply(Apply, NewInstPair.first);


I am not saying that using replaceDeadArray is necessarily wrong. What I am saying is that you are changing the code in a functional way by moving the time the applies are deleted from after the transformation to during the transformation. I am trying to prove to myself that doing so is safe (the pattern you are removing is a common defensive coding pattern in optimization passes). When I look at the git history these lines were added in the initial pass. @rudkx do you remember why you did this here?

gottesmm · 2018-04-05T05:26:00Z

lib/SILOptimizer/Mandatory/MandatoryInlining.cpp


-  replaceDeadApply(InnerAI, NewInst);
+  replaceDeadApply(InnerAI, NewInstPair.first);

  auto newApplyAI = NewInstPair.second.getInstruction();
  assert(newApplyAI && "devirtualized but removed apply site?");
  I = newApplyAI->getIterator();
  auto NewAI = FullApplySite::isa(newApplyAI);


Why not just eliminate NewAI, i.e.:

return {FullApplySite::isa(newApplyAI), I};

Also next time, it would be helpful for reviewers who read the mailing list if you could put stuff like this in a separate focused PR. The reason why I am asking is that logically has nothing to do with the title/subject of the PR, then people who just look at Titles on the mailing list may not know that this is code that they should review.

And yes logically the SIL optimizer changes are distinct, and could be in a separate PR, but they were motivated by other changes in this PR. It can be tricky to find the right balance here. Sometimes, splitting up a change into too many PRs can also be difficult to review because its hard to get the big picture.

That is why in the PR you specifically tell the reviewer the big picture and uses references in between the PRs/etc to provide the connection for posterity. NOTE I am not saying that I have always done this well.

slavapestov · 2018-04-05T07:06:00Z

@swift-ci Please smoke test

slavapestov · 2018-04-05T07:11:02Z

@swift-ci Please test source compatibility

slavapestov · 2018-04-05T07:11:24Z

@swift-ci Please test

swift-ci · 2018-04-05T07:13:06Z

Build failed
Swift Test OS X Platform
Git Sha - 2fac858

swift-ci · 2018-04-05T07:13:07Z

Build failed
Swift Test Linux Platform
Git Sha - 2fac858

slavapestov · 2018-04-05T07:22:53Z

@gottesmm I forgot to push revised patches. I'll do after the source compat suite completes.

jrose-apple

I only reviewed the AST/Sema parts, but I'm pretty unhappy with how many extra hacks there are here. I'd still like to pursue the "formally change the language to not require access" option that would not require special casing protocol extension scopes nearly as much.

I suggest adding a cross-module execution test case that shows the tricky linker cases, including using the protocol in a generic context from the client module with optimizations enabled (to make sure we don't devirtualize).

jrose-apple · 2018-04-05T15:44:57Z

include/swift/AST/Decl.h

+  /// containing scope is not really visible. However, if the containing scope
+  /// is a protocol extension, they can show up here, so we have to filter
+  /// them out directly.
+  AccessLevel adjustAccessLevelForProtocolExtension(AccessLevel access) const;


Nitpick: this should be static.

It calls getDeclContext() on this

Uh…that seems fishy, then. Why isn't it only called when needed?

jrose-apple · 2018-04-05T15:45:30Z

include/swift/AST/Decl.h

-  getFormalAccessScope(const DeclContext *useDC = nullptr,
-                       bool respectVersionedAttr = false) const;
+  getFormalAccessScope(const DeclContext *useDC=nullptr,
+                       bool isUsageFromInline=false) const;


Nitpick: Why remove the spaces?

Also this name isn't really correct. The formal access scope doesn't change when used from an inline function; it changes when it's only being used as a symbol and not as part of API.

I renamed this in the other PR that got merged already.

jrose-apple · 2018-04-05T15:46:04Z

include/swift/AST/Decl.h

@@ -2319,7 +2326,8 @@ class ValueDecl : public Decl {
  /// A public declaration is accessible everywhere.
  ///
  /// If \p DC is null, returns true only if this declaration is public.
-  bool isAccessibleFrom(const DeclContext *DC) const;
+  bool isAccessibleFrom(const DeclContext *DC,
+                        bool forConformance=false) const;


Please include an explanation of the forConformance flag in the doc comment. This whole thing was really subtle and deserves to be written down.

A protocol extension of a private protocol can define internal or public members. We should not be able to find these members from another file or module if an internal or public type conforms to the protocol. Fixes <rdar://problem/21380336>.

slavapestov · 2018-04-06T06:27:04Z

@jrose-apple I added some more comments, cleaned it up a bit and added the SIL optimizer test case you requested.

jrose-apple

Thanks. I still don't think this is the right answer for the language, but it's a non-regressing answer. Can you file a Radar to track one of us looking into changing the witness rule?

slavapestov · 2018-04-06T19:04:24Z

Sure thing.

slavapestov · 2018-04-06T19:04:42Z

@swift-ci Please smoke test

slavapestov · 2018-04-06T19:05:19Z

@swift-ci Please smoke test

slavapestov requested a review from DougGregor April 4, 2018 09:33

slavapestov force-pushed the se-0025-edge-case branch from 2fac858 to ff18963 Compare April 5, 2018 01:28

gottesmm reviewed Apr 5, 2018

View reviewed changes

slavapestov force-pushed the se-0025-edge-case branch from ff18963 to aad1fc7 Compare April 5, 2018 07:01

jrose-apple reviewed Apr 5, 2018

View reviewed changes

slavapestov force-pushed the se-0025-edge-case branch from aad1fc7 to 2c8d502 Compare April 5, 2018 15:56

slavapestov mentioned this pull request Apr 6, 2018

Preliminary "opaque conformance" support in SIL #15784

Merged

AST: Plug a hole in access control checking

a5579d1

A protocol extension of a private protocol can define internal or public members. We should not be able to find these members from another file or module if an internal or public type conforms to the protocol. Fixes <rdar://problem/21380336>.

slavapestov force-pushed the se-0025-edge-case branch from 2c8d502 to a5579d1 Compare April 6, 2018 06:25

jrose-apple reviewed Apr 6, 2018

View reviewed changes

slavapestov merged commit da6bff5 into swiftlang:master Apr 6, 2018

alexprogrammr mentioned this pull request May 28, 2018

Linker error in release builds jedisct1/swift-sodium#152

Closed

Fix SE-0025 edge case involving public member of protocol extension of private protocol #15735

Fix SE-0025 edge case involving public member of protocol extension of private protocol #15735

Uh oh!

Conversation

slavapestov commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented Apr 5, 2018

Uh oh!

slavapestov commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov Apr 5, 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!

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 Apr 5, 2018

Uh oh!

slavapestov commented Apr 5, 2018

Uh oh!

slavapestov commented Apr 5, 2018

Uh oh!

swift-ci commented Apr 5, 2018

Uh oh!

swift-ci commented Apr 5, 2018

Uh oh!

slavapestov commented Apr 5, 2018

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 Apr 6, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Apr 6, 2018

slavapestov commented Apr 4, 2018 •

edited

Loading

slavapestov Apr 5, 2018 •

edited

Loading