Use function signatures for SILDeclRefs in witness_tables, vtables and witness_method instructions #7078

swiftix · 2017-01-27T02:40:10Z

Textual SIL was sometimes ambiguous when SILDeclRefs were used, because the textual representation of SILDeclRefs was the same for functions that have the same name, but different signatures.

This patch fixes this problem.

swiftix · 2017-01-27T02:40:28Z

@swift-ci Please test

swift-ci · 2017-01-27T03:10:39Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - a73b816ed4c352e8bf784652b118fd3e2a81030f
Test requested by - @swiftix

jrose-apple · 2017-01-27T03:16:14Z

lib/SIL/SILPrinter.cpp

    *this << AMI->getType();
  }
  void visitWitnessMethodInst(WitnessMethodInst *WMI) {
    if (WMI->isVolatile())
      *this << "[volatile] ";
-    *this << "$" << WMI->getLookupType() << ", " << WMI->getMember();
+    *this << "$" << WMI->getLookupType() << ", " << WMI->getMember() 
+          << " : " << WMI->getMember().getDecl()->getInterfaceType();


Does this need to use custom PrintOptions too?

Hmm. Good question. The tests will show.

They might not, if it happens not to hit this case in the stdlib either.

Yeah. I could try to construct a use-case by hand using non-qualified types.

swiftix · 2017-01-27T03:24:54Z

@swift-ci Please test OSX

swiftix · 2017-01-27T03:25:10Z

@swift-ci Please test OSX

swiftix · 2017-01-27T03:25:56Z

@swift-ci please test OSX

swiftix · 2017-01-27T03:29:01Z

@swift-ci please test OS X

swiftix · 2017-01-27T03:29:15Z

@swift-ci please test

swift-ci · 2017-01-27T03:29:15Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - a73b816ed4c352e8bf784652b118fd3e2a81030f
Test requested by - @swiftix

swift-ci · 2017-01-27T03:29:53Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - a73b816ed4c352e8bf784652b118fd3e2a81030f
Test requested by - @swiftix

swiftix · 2017-01-27T03:32:58Z

@swift-ci please test OS X

swift-ci · 2017-01-27T03:33:14Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - db2a5e51dd053d386f7141e69e10110a0c49eec9
Test requested by - @swiftix

swift-ci · 2017-01-27T04:07:03Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - a73b816ed4c352e8bf784652b118fd3e2a81030f
Test requested by - @swiftix

swiftix · 2017-01-27T04:33:24Z

@jrose-apple Hmm. By looking at the failing tests I got the impression that we need to use the fully qualified names only for the types coming from the stdlib.

See: https://ci.swift.org/job/swift-PR-osx/5245/consoleFull#1463937818ee1a197b-acac-4b17-83cf-a53b95139a76

If we use fully qualified names for user files, we get problems with sil-opt processing the -emit-sil output and the like, e.g. because it assumes by default "main" to be the name of the module. Of course, we could fix these tests by adding -module-name to the sil-opt command line options.

WDYT?

swiftix · 2017-01-27T06:12:05Z

@swift-ci please test OS X

swift-ci · 2017-01-27T06:12:27Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 064b72f556c80fbe45d2ecbe145d65b576654c44
Test requested by - @swiftix

swiftix · 2017-01-27T15:29:51Z

@swift-ci Please test Linux

swiftix · 2017-01-27T15:30:16Z

@swift-ci please test Linux

swift-ci · 2017-01-27T15:51:39Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 4eca265662f079ac2a7d8c01bb7232638af9de41
Test requested by - @swiftix

swiftix · 2017-01-27T16:18:16Z

@jrose-apple Jordan, the failures on Linux are all like this:

sil function not found '_AnyCollectionBox':

#_AnyCollectionBox!ivardestroyer.1: _AnyCollectionBox<Element>.Type : 
_TFCs17_AnyCollectionBoxE       // _AnyCollectionBox.__ivar_destroyer

The parser tries to treat the _AnyCollectionBox.Type as a function type I guess. In any case, probably these ivar_destroyers should be handled in a special way? What is actually the type of such a destroyer function? I guess it is not really _AnyCollectionBox<Element>.Type

jrose-apple · 2017-01-27T16:57:19Z

I don't know; I don't know how ivar destroyers work. @slavapestov, @jckarter?

jrose-apple · 2017-01-27T16:59:24Z

@jrose-apple Hmm. By looking at the failing tests I got the impression that we need to use the fully qualified names only for the types coming from the stdlib.

That doesn't make sense. What if the file you're emitting SIL for imports some other library that defines a type with the same name? Maybe the only time it's okay to not qualify a type is when it's from the current module. You could either add a print option for that, or just print qualified types all the time.

swiftix · 2017-01-27T17:10:04Z

That doesn't make sense. What if the file you're emitting SIL for imports some other library that defines a type with the same name? Maybe the only time it's okay to not qualify a type is when it's from the current module. You could either add a print option for that, or just print qualified types all the time.

Right. I realized it yesterday, after I asked you. I introduced some changes so that we don't use qualified types for the types from the current module. It seems to work nicely.

The last remaining issue seem to be those ivar destroyers. I need to understand what is so special about them and how to handle them properly.

swiftix · 2017-01-27T17:54:46Z

@swift-ci please test Linux

swiftix · 2017-01-27T17:55:24Z

@swift-ci please test Linux

swift-ci · 2017-01-27T18:31:13Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 60a2cba964f7fb90adc0563d2cf2f8367d23ef7c
Test requested by - @swiftix

swiftix · 2017-01-27T18:37:25Z

@swift-ci Please smoke test Linux

swiftix · 2017-01-27T18:37:42Z

@swift-ci please smoke test Linux

gottesmm · 2017-01-27T18:56:35Z

@swiftix Can you do a full test for this. So that the parse stdlib tests run?

swiftix · 2017-01-27T19:23:10Z

@swift-ci please smoke test OS X

swiftix · 2017-01-27T19:23:45Z

@swift-ci please smoke test

swiftix · 2017-01-27T20:00:43Z

@swift-ci please smoke test OS X

swiftix · 2017-01-27T20:01:21Z

@swift-ci please smoke test OS X

…d witness_method instructions. Textual SIL was sometimes ambiguous when SILDeclRefs were used, because the textual representation of SILDeclRefs was the same for functions that have the same name, but different signatures.

swiftix · 2017-01-27T20:17:09Z

@swift-ci please test

swift-ci · 2017-01-27T20:17:32Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 4eca265662f079ac2a7d8c01bb7232638af9de41
Test requested by - @swiftix

swift-ci · 2017-01-27T20:18:37Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 60a2cba964f7fb90adc0563d2cf2f8367d23ef7c
Test requested by - @swiftix

jrose-apple reviewed Jan 27, 2017

View reviewed changes

swiftix force-pushed the wip-textual-sil-sildeclrefs branch from a73b816 to db2a5e5 Compare January 27, 2017 03:23

swiftix force-pushed the wip-textual-sil-sildeclrefs branch from db2a5e5 to 064b72f Compare January 27, 2017 03:32

swiftix force-pushed the wip-textual-sil-sildeclrefs branch from 064b72f to 4eca265 Compare January 27, 2017 06:11

swiftix force-pushed the wip-textual-sil-sildeclrefs branch from 94fa2d7 to 8ad61d5 Compare January 27, 2017 20:16

swiftix changed the title ~~Use function signatures for SILDeclRefs in witness_tables, vtables an…~~ Use function signatures for SILDeclRefs in witness_tables, vtables and witness_method instructions Jan 27, 2017

swiftix merged commit 400f338 into swiftlang:master Jan 27, 2017

Use function signatures for SILDeclRefs in witness_tables, vtables and witness_method instructions #7078

Use function signatures for SILDeclRefs in witness_tables, vtables and witness_method instructions #7078

Uh oh!

Conversation

swiftix commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swift-ci commented Jan 27, 2017

Uh oh!

jrose-apple Jan 27, 2017

Choose a reason for hiding this comment

Uh oh!

swiftix Jan 27, 2017

Choose a reason for hiding this comment

Uh oh!

jrose-apple Jan 27, 2017

Choose a reason for hiding this comment

Uh oh!

swiftix Jan 27, 2017

Choose a reason for hiding this comment

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swift-ci commented Jan 27, 2017

Uh oh!

swift-ci commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swift-ci commented Jan 27, 2017

Uh oh!

swift-ci commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swift-ci commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swift-ci commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

jrose-apple commented Jan 27, 2017

Uh oh!

jrose-apple commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swift-ci commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

gottesmm commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!

swiftix commented Jan 27, 2017

Uh oh!