SILGen: Don't consider a function pointer call to lie about nonoptional returns. #2368

jckarter · 2016-05-02T16:49:49Z

We would incorrectly consider a call like struct.functionPointer(), where functionPointer is a property of block or function pointer type, to be able to lie about its return type nullability, even if the function pointer's own return type was a value type that isn't nullable. This would lead us to generate nonsense bitcast instructions from () to ()? that would get lowered to traps in IRGen. Fixes rdar://problem/23346344.

jckarter · 2016-05-02T16:50:01Z

@swift-ci Please test and merge

jrose-apple · 2016-05-02T18:06:54Z

lib/SILGen/SILGenExpr.cpp

A non-object pointer type should also get this behavior.

We probably need a new canBeNullableInC predicate then.

Actually, mayLieAboutNonOptionalReturn already does this check; it just does it incorrectly because it doesn't notice that the decl might be a member. (And it also gets it wrong in the same way.)

@jrose-apple It isn't clear to me how mayLieAboutNonOptionalReturn currently handles UnsafePointers. AFAICT it only checks hasReferenceSemantics, which is only true for class, metatype, and function types.

Yeah, that's what I meant: it looks like we already get this wrong. I'm not happy about us now checking in two places; it seems redundant.

We need this extra check because ApplyExprs can only lie about their return nullability if the result of the application is nullable in C. The existing code recurs into the ApplyExpr's subexpression but loses that context.

jckarter · 2016-05-02T21:03:22Z

@slavapestov @bitjammer Do you recognize this test failure?

******************** TEST 'Swift :: Reflection/typeref_decoding_objc.swift' FAILED ********************
Script:
--
rm -rf /Users/buildnode/jenkins/workspace/swift-PR-osx/buildbot_incremental/swift-macosx-x86_64/test-iphonesimulator-i386/Reflection/Output/typeref_decoding_objc.swift.tmp && mkdir -p /Users/buildnode/jenkins/workspace/swift-PR-osx/buildbot_incremental/swift-macosx-x86_64/test-iphonesimulator-i386/Reflection/Output/typeref_decoding_objc.swift.tmp
xcrun --toolchain default --sdk /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator9.3.sdk /Users/buildnode/jenkins/workspace/swift-PR-osx/buildbot_incremental/swift-macosx-x86_64/bin/swiftc -target i386-apple-ios7.0  -module-cache-path '/var/folders/_8/79jmzf2142z2xydc_01btlx00000gn/T/swift-testsuite-clang-module-cachehQbFic' -F /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator9.3.sdk/../../../Developer/Library/Frameworks    /Users/buildnode/jenkins/workspace/swift-PR-osx/swift/test/Reflection/Inputs/ObjectiveCTypes.swift -parse-as-library -emit-module -emit-library -module-name TypesToReflect -Xfrontend -enable-reflection-metadata -Xfrontend -enable-reflection-names -o /Users/buildnode/jenkins/workspace/swift-PR-osx/buildbot_incremental/swift-macosx-x86_64/test-iphonesimulator-i386/Reflection/Output/typeref_decoding_objc.swift.tmp/libTypesToReflect.dylib
/Users/buildnode/jenkins/workspace/swift-PR-osx/buildbot_incremental/swift-macosx-x86_64/bin/swift-reflection-dump -arch i386 -binary-filename /Users/buildnode/jenkins/workspace/swift-PR-osx/buildbot_incremental/swift-macosx-x86_64/test-iphonesimulator-i386/Reflection/Output/typeref_decoding_objc.swift.tmp/libTypesToReflect.dylib > /Users/buildnode/jenkins/workspace/swift-PR-osx/buildbot_incremental/swift-macosx-x86_64/test-iphonesimulator-i386/Reflection/Output/typeref_decoding_objc.swift.tmp/typeref_decoding.txt
diff -u /Users/buildnode/jenkins/workspace/swift-PR-osx/swift/test/Reflection/typeref_decoding_objc.result.txt /Users/buildnode/jenkins/workspace/swift-PR-osx/buildbot_incremental/swift-macosx-x86_64/test-iphonesimulator-i386/Reflection/Output/typeref_decoding_objc.swift.tmp/typeref_decoding.txt
--
Exit Code: 1

Command Output (stdout):
--
--- /Users/buildnode/jenkins/workspace/swift-PR-osx/swift/test/Reflection/typeref_decoding_objc.result.txt  2016-05-02 11:16:19.000000000 -0700
+++ /Users/buildnode/jenkins/workspace/swift-PR-osx/buildbot_incremental/swift-macosx-x86_64/test-iphonesimulator-i386/Reflection/Output/typeref_decoding_objc.swift.tmp/typeref_decoding.txt   2016-05-02 12:55:47.000000000 -0700
@@ -55,9 +55,8 @@
 (struct Swift.UInt)
 - Metadata sources:
 - Capture types:
-- Metadata sources:
 !!! Invalid typeref
-!!! Invalid matadata source
+- Metadata sources:
 - Capture types:
 (struct Swift.StaticString)
 (bound_generic_struct Swift.UnsafeBufferPointer
@@ -66,9 +65,8 @@
 (struct Swift.UInt)
 - Metadata sources:
 - Capture types:
-- Metadata sources:
 !!! Invalid typeref
-!!! Invalid matadata source
+- Metadata sources:
 - Capture types:
 (bound_generic_struct Swift.UnsafeBufferPointer
   (struct Swift.UInt8))
@@ -78,7 +76,6 @@
 (struct Swift.UInt)
 - Metadata sources:
 - Capture types:
-- Metadata sources:
 !!! Invalid typeref
-!!! Invalid matadata source
+- Metadata sources:


--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
Testing Time: 1099.71s
********************
Failing Tests (1):
    Swift :: Reflection/typeref_decoding_objc.swift

jckarter · 2016-05-02T21:03:54Z

There's a related-looking failure in the Linux build too.

jckarter · 2016-05-02T21:17:23Z

@swift-ci Please test and merge

slavapestov · 2016-05-02T21:28:59Z

@jckarter Jordan already disabled the test. I'm working on it now.

jckarter · 2016-05-02T23:14:39Z

@slavapestov @jrose-apple The typeref_decoding_objc.swift test still appears to be failing; does that need to be disabled too?

jckarter · 2016-05-03T16:44:13Z

@swift-ci Please test

jckarter · 2016-05-03T16:49:20Z

lib/SILGen/SILGenExpr.cpp

@jrose-apple I updated the mayLieAboutOptionalReturn code to check for pointer types and factored out the isNullableTypeInC check into a separate helper function. This look good?

Sorry, there are several more pointer types: OpaquePointer, Selector, and…well, I guess we don't have to care about NSZone.

I'm not happy about this list being hardcoded in more and more places, though. I suppose it's not enough to check the Clang type because that would include things like va_list, which might coincidentally be pointers.

Using hasReferenceSemantics to cover (thin) function pointers also feels a little sketchy, but I suppose we're not ever going to change that.

Is there a list somewhere in the Clang importer we could adapt?

Unfortunately it's not defined in terms of Swift types. https://github.com/apple/swift/blob/master/lib/ClangImporter/ClangImporter.cpp#L1533

…ugh, it looks like that code is wrong too.

jckarter · 2016-05-04T04:37:13Z

@swift-ci Please test

jckarter · 2016-05-04T04:37:38Z

lib/SILGen/SILGenExpr.cpp

@jrose-apple Updated again to handle OpaquePointer and Selector. Any other concerns?

jckarter · 2016-05-04T16:03:54Z

@swift-ci Please test

- Now that *Pointer types are imported with nullability, there's the potential for non-object pointer APIs to lie about their nullability, so extend the hack to cover them. - We would incorrectly consider a call like "struct.functionPointer()", where functionPointer is a property of block or function pointer type, to be able to lie about its return type nullability, even if the function pointer's own return type was a value type that isn't nullable. This would lead us to generate nonsense bitcast instructions from () to ()? that would get lowered to traps in IRGen. Fix this by checking for nullable types only on the types of expressions, where we have semantic information enough to understand what the types really mean, and only checking the Clang-ness of declarations. Fixes rdar://problem/23346344.

jckarter · 2016-05-05T01:31:21Z

@swift-ci Please test

jckarter · 2016-05-05T01:32:59Z

@jrose-apple As we discussed via IM, I've refactored mayLieAboutNonOptionalReturn so that all the checks for nonoptional types occur only on expressions, where we have semantic context enough to understand where the types are coming from. When we look at decls, we now only assert that the relevant decl type is nullable then check whether the decl hasClangNode, without any redundant non-assertion type checks. Sound good?

jrose-apple · 2016-05-05T16:01:04Z

Yeah, makes sense. Thanks for working through this!

jckarter · 2016-05-05T16:17:28Z

No problem, thanks for the help @jrose-apple!

jrose-apple reviewed May 2, 2016
View reviewed changes

jckarter force-pushed the r23346344 branch from ebd112b to e8bfc3d Compare May 2, 2016 21:16

jckarter force-pushed the r23346344 branch from e8bfc3d to b496bb0 Compare May 3, 2016 16:44

jckarter reviewed May 3, 2016
View reviewed changes

jckarter force-pushed the r23346344 branch from b496bb0 to 04ec6e6 Compare May 4, 2016 04:36

jckarter reviewed May 4, 2016
View reviewed changes

lib/SILGen/SILGenExpr.cpp Outdated

Copy link

Contributor Author

jckarter May 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrose-apple Updated again to handle OpaquePointer and Selector. Any other concerns?

jckarter force-pushed the r23346344 branch from 04ec6e6 to 8e000c9 Compare May 4, 2016 16:03

jckarter force-pushed the r23346344 branch from 8e000c9 to e1ed305 Compare May 5, 2016 01:31

jckarter merged commit 5302e38 into swiftlang:master May 5, 2016

jckarter deleted the r23346344 branch May 5, 2016 16:17

SILGen: Don't consider a function pointer call to lie about nonoptional returns. #2368

SILGen: Don't consider a function pointer call to lie about nonoptional returns. #2368

Uh oh!

Conversation

jckarter commented May 2, 2016

Uh oh!

jckarter commented May 2, 2016

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!

jckarter commented May 2, 2016

Uh oh!

jckarter commented May 2, 2016

Uh oh!

jckarter commented May 2, 2016

Uh oh!

slavapestov commented May 2, 2016

Uh oh!

jckarter commented May 2, 2016

Uh oh!

jckarter commented May 3, 2016

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!

jckarter commented May 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter commented May 4, 2016

Uh oh!

jckarter commented May 5, 2016

Uh oh!

jckarter commented May 5, 2016

Uh oh!

jrose-apple commented May 5, 2016

Uh oh!

jckarter commented May 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants