[SIL]/[SILOpt] Thunks and specializations are never public. #16959

huonw · 2018-06-02T01:21:13Z

The current behaviour of (some) open classes means private members are
forced to be public, but this only applies to the real members, not
thunks or specializations associated with them.

Fixes rdar://problem/40738913.

huonw · 2018-06-02T01:21:24Z

@swift-ci please test

swift-ci · 2018-06-02T01:52:54Z

Build failed
Swift Test Linux Platform
Git Sha - dd923dddcd99c0744ec75519543f90644fe7eaec

slavapestov · 2018-06-02T06:32:45Z

include/swift/SIL/SILFunction.h

@@ -430,6 +430,11 @@ class SILFunction
  /// method itself, the function can be referenced from vtables of derived
  /// classes in other compilation units.
  SILLinkage getEffectiveSymbolLinkage() const {
+    // These are implementations details that don't get special treatment: their
+    // stored linkage is the correct one.
+    if (isThunk() || isSpecialization())


I think it would make more sense to turn this into an assertion and fix those places that create thunks no?

Hm, I'm not sure: this is called in places that have just an arbitrary SILFunction:

https://github.com/apple/swift/blob/e8ee93b131d2ad360a8ff02034eb6c29718e411c/lib/IRGen/GenDecl.cpp#L1618-L1619

We could push this condition down to them, but I'm not sure it makes sense. To me, it seems reasonable to say that getEffectiveSymbolLinkage is the way to get the true IR/ELF/mach-O linkage, and thus having this split here is the right place.

Maybe I'm not understanding what you're visualizing?

I'm suggesting that getClassSubclassScope() should not be set on thunks. Whoever is creating the thunk's SILFunction is also setting getClassSubclassScope(), which seems clearly wrong.

Ah, I had misunderstood. That sounds good.

huonw · 2018-06-04T17:17:43Z

Linux failure (for future reference):

20:53:10 FAILED: ../buildbot_linux/foundation-linux-x86_64/Foundation/libFoundation.so 
...
../buildbot_linux/foundation-linux-x86_64/Foundation/Foundation/NSCFDictionary.swift.o:NSCFDictionary.swift.o:$S10Foundation15_NSCFDictionaryCMf: error: undefined reference to '$S10Foundation12NSDictionaryC14getDescription33_8C53A72A73712D943F7602D2573DFCFBLL2ofSSSgyp_tF'

jrose-apple · 2018-06-12T00:21:05Z

Mm. What about override thunks, for concrete subclasses of generic classes?

open class Base<T> {
  open func consume(_: T) {}
}

open class Sub: Base<String> {
  open override func consume(_: String) { print("override") }
}

slavapestov · 2018-06-12T04:31:32Z

@jrose-apple Override thunks get emitted in each translation unit that has a vtable for a subclass of the class. So you're still relying on the original method having public linkage.

huonw · 2018-06-23T12:06:27Z

@swift-ci please test

swift-ci · 2018-06-23T12:37:32Z

Build failed
Swift Test Linux Platform
Git Sha - 0c67e56f85370721980f31ead5349fc7183a64fd

huonw · 2018-06-23T23:15:39Z

@swift-ci please test source compatibility

huonw · 2018-06-25T12:55:57Z

@swift-ci please test

@slavapestov I've changed to not setting the subclass scope on thunks, except for signature-optimised thunks, since in that case the original entry point to the code gets marked as a thunk, and thus that needs to have the right subclass scope, so that it remains public if it is private.

huonw · 2018-06-25T23:22:29Z

@swift-ci please smoke test

slavapestov · 2018-06-25T23:43:50Z

include/swift/SIL/SILFunction.h

+  void setThunk(IsThunk_t isThunk, bool forSignatureOptimization = false) {
+    // The _original_ function for a method can turn into a thunk through
+    // signature optimization, meaning it needs to retain its subclassScope
+    assert(getClassSubclassScope() == SubclassScope::NotApplicable ||


This seems overkill. If you're intent on keeping this, you should add a new case to IsThunk_t for signature-optimized thunks. A future assertion could also ensure that any thunk other than a signature thunk is shared or private, etc.

huonw · 2018-06-26T03:23:34Z

@swift-ci please smoke test

Signature optimization is slightly different to (most) other thunks, in that it's taking an existing function and turning that into a thunk, rather than creating a thunk that calls an existing function. These symbols can be public, etc. and so need to be handled a bit different to other types of thunks.

…heir parent class. The "subclass scope" is meant to represent a connection to a vtable (and how public something needs to be), for things that end up in class vtables. Specializations and thunks are mostly internal implementation details and do not end up there, so subclass scope is not applicable to them. This stops the thunks and specializations being incorrectly public. (Note, there are some thunks that _are_ public facing: if a function has its signature optimized, the original entry point becomes a thunk, and this entry point is what ends up in vtables etc., so needs to remain around, which means keeping the same hacks for `private` members of an `open` class.) Fixes rdar://problem/40738913.

huonw · 2018-06-26T06:28:15Z

@swift-ci please smoke test

huonw · 2018-06-26T09:14:01Z

@swift-ci please test source compatibility

huonw requested a review from slavapestov June 2, 2018 01:21

huonw mentioned this pull request Jun 2, 2018

[Frontend] Turn symbols-missing-from-TBD validation on by default in debug builds on Apple platforms. #16838

Merged

slavapestov requested changes Jun 2, 2018

View reviewed changes

slavapestov self-assigned this Jun 7, 2018

huonw force-pushed the tbd-thunk-specialization branch from dd923dd to 0c67e56 Compare June 23, 2018 12:04

huonw force-pushed the tbd-thunk-specialization branch from 0c67e56 to 4b5c62c Compare June 25, 2018 12:53

slavapestov approved these changes Jun 25, 2018

View reviewed changes

huonw force-pushed the tbd-thunk-specialization branch from 4b5c62c to 9576d5d Compare June 26, 2018 03:23

huonw added 2 commits June 26, 2018 14:53

huonw force-pushed the tbd-thunk-specialization branch from 9576d5d to ed64fad Compare June 26, 2018 06:26

huonw merged commit 1c86abb into swiftlang:master Jun 26, 2018

huonw deleted the tbd-thunk-specialization branch June 26, 2018 13:08

huonw mentioned this pull request Jun 27, 2018

[SILOptimizer] Properly disconnect the signature optimized function from a parent class. #17539

Merged

[SIL]/[SILOpt] Thunks and specializations are never public. #16959

[SIL]/[SILOpt] Thunks and specializations are never public. #16959

Uh oh!

Conversation

huonw commented Jun 2, 2018

Uh oh!

huonw commented Jun 2, 2018

Uh oh!

swift-ci commented Jun 2, 2018

Uh oh!

slavapestov Jun 2, 2018

Choose a reason for hiding this comment

Uh oh!

huonw Jun 4, 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!

slavapestov Jun 7, 2018

Choose a reason for hiding this comment

Uh oh!

huonw Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

huonw commented Jun 4, 2018

Uh oh!

jrose-apple commented Jun 12, 2018

Uh oh!

slavapestov commented Jun 12, 2018

Uh oh!

huonw commented Jun 23, 2018

Uh oh!

swift-ci commented Jun 23, 2018

Uh oh!

huonw commented Jun 23, 2018

Uh oh!

huonw commented Jun 25, 2018

Uh oh!

huonw commented Jun 25, 2018

Uh oh!

slavapestov Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

huonw commented Jun 26, 2018

Uh oh!

huonw commented Jun 26, 2018

Uh oh!

huonw commented Jun 26, 2018

Uh oh!

Uh oh!

huonw Jun 4, 2018 •

edited

Loading