[SR-12723][Sema] Validate function type param representations thick-to-thin conversions #31814

LucianoPAlmeida · 2020-05-15T14:36:34Z

Check representations of function type parameters are valid conversions on SILGen and diagnose if they not instead of letting them hit assertions and crashing the compiler.

cc @xedin @slavapestov :)

Resolves SR-12723.

xedin · 2020-05-15T19:39:10Z

lib/Sema/CSSimplify.cpp

@@ -1358,10 +1359,10 @@ static bool matchFunctionRepresentations(FunctionTypeRepresentation rep1,
  case ConstraintKind::BindParam:
  case ConstraintKind::BindToPointerType:
  case ConstraintKind::Equal:
-    return rep1 != rep2;
+  case ConstraintKind::Subtype:


I think the reason why it's done like that is because there is a subtyping rule between some of the representations which is verified/maintained by SIL, is that right @slavapestov? I guess if we wanted to change status quo here we might add actual checks whether there is a subtyping rule between given two function representations and if not - add a tailored fix.

Humm, interesting ... I thought that maybe could be a rule of something like this, but I did a test on this example changing c convention to all others and thin, block, swift and except when the representation was the same all crashed, so my guess was that it only allowed this way... I don't understand much about SIL or how it verifies this but let me try to take a look
Thanks @xedin :)

To be honest I don't know that the subtype matrix here is e.g.

func foo(_: (@convention(c) () -> Void) -> Void) {} func bar(_: () -> Void) {} foo(bar)

It's accepted by type-checker but crashes with assert in SILGen. Looks like CSApply generates a conversion between () -> Void and @conversion(c) () -> Void. Maybe we should reject this code in Sema. @jckarter, thoughts?

Seems like the same happens with block convention too, thin doesn't.
For what I could see on SIL(although I don't understand much) is that SIL representations thin -> thick are conversions are allowed, but only in some situations ...

Right, I think I'm having progress, but question... if a function foo type has as parameter a @convetion(c) function type which makes its representation c, the representation of the function foo should still be swift?
@xedin @slavapestov @jckarter

@jckarter Just to clarify what you are saying is that we should forbid any conversions between different representations when function types are embedded into other type e.g. optional, argument/result of a function, generic arguments?

@xedin If it works today, I would still allow it for conversions from "thin" conventions (c, thin) to "thick" (swift, block) or conversions between thick conventions. It's only thick-to-thin conversions that can't be supported (except for the limited cases where we can tell that they statically reference a global function that doesn't require context).

Thank you for clarification! I think we can use locator in matchFunctionTypes to find where function type is inner and reject certain conversions in Sema.

Ok, I'll get back to this later today after work :)
Just to clarify, sorry I don't know if I fully understand the "except for the limited cases where we can tell that they statically reference a global function that doesn't require context" but from what I get here is we are forbidding different representations checking for only argument locator function types?

@xedin I've made some changes, let me know what you think :)

lib/Sema/CSSimplify.cpp

test/SILGen/function_conversion_objc.swift

slavapestov · 2020-05-18T18:17:43Z

@LucianoPAlmeida This should be detected in Sema in matchFunctionTypes(); SILGen is too late, because then it cannot influence overload resolution, etc. @xedin Can help you find the right place to add the checks.

LucianoPAlmeida · 2020-05-23T02:40:38Z

@LucianoPAlmeida This should be detected in Sema in matchFunctionTypes(); SILGen is too late, because then it cannot influence overload resolution, etc. @xedin Can help you find the right place to add the checks.

Right, @slavapestov that was actually the first approach when I send the PR, but the bigger question was what kind of conversion matrix actually crashes the compiler in the argument context but should be validated(because it is truly not convertible) or if although it crashes the conversion should be supported (so we can't really validate at this level)? :)

LucianoPAlmeida · 2020-05-23T20:03:08Z

I'm back working on this, but I have a couple of questions

If this high-level validation of parameter conversion is a possible way to go, should we move this to Sema and validate the convention matrix for subtypes as @slavapestov mentioned?
In the context of parameters (subtype conversion) function types with different representations always need a thunk? Does variance affect this in some way?
There is one specific block -> c convention, that hits an assertion while debugging or compiler explorer. But compiles with release Xcode version ending up in a runtime error while applying re-abstraction thunk

struct SR12723_C {
  let function: (@convention(block)() -> Void) -> Void
}

let block = SR12723_C { app in app() } // app  () -> ()  0x00000001000018f0 Test`partial apply forwarder for reabstraction thunk helper from @convention(thin) @convention(c) () -> () to @escaping @callee_unowned @convention(block) () -> () at <compiler-generated>

let f: (@convention(c)() -> Void) -> Void = block.function

f({  print("a") })

Should this case be supported?

cc @xedin @slavapestov @jckarter :)

test/Sema/diag_subtype_argument_representation.swift

xedin · 2020-06-04T21:37:23Z

validation-test/compiler_crashers_2_fixed/sr12723.swift

+    let _ :  (@convention(thin) () -> Void) -> Void = c.function 
+    // expected-error@-1 {{cannot convert value of type '(@convention(c) () -> Void) -> Void' to specified type '(@convention(thin) () -> Void) -> Void'}}
+
+    let _ :  (() -> Void) -> Void = c.function 


This and some other conversions here are thin-to-thick which should be allowed. I think this is actually meant to work in reverse - thin-to-thick is allowed but thick-to-thin isn't.

At least this is what Joe mentioned in his comment.

Hey @xedin :)
Hum, that's actually one of the questions I had, because on the param function type there is the fact that subtype relation is contravariant. So my question is if this affects the conversion in some way?
In this case, for example, converting (@convention(c)()->Void)->Void to (()->Void)->Void on A -> B to A'-> B' respectivelly, this means the subtype relation is A' is subtype of A so the ()->Void is a subtype of @convention(c)()->Void, does this means the conversion is thick-to-thin? ... at the time this makes sense to me, but that is actually a question I have, do you know if this makes sense or am I missing something? Let me know if I this makes sense :)

Also, the logic on matchFunctionTypes is only allowing exactly thin-to-thick and thick-to-thick any other conversions are being deferred to check if representations are the same.

Yes, you are correct, sorry for the confusion. I was just merely trying to point out that we might want to change diagnostic or add a custom note which explains precisely why these types couldn't be converted (related to contravariance) because it appears that it's passing thick-to-thin.

I got tripped by this myself because I didn't notice that diagnostic mentioned whole function type...

Right, thanks for the clarification :)
I've added in the last commit, thin to thin which seems to be allowed, so now we strictly only validating thick-to-thin.

we might want to change diagnostic or add a custom note which explains precisely why these types couldn't be converted (related to contravariance) because it appears that it's passing

About the note, I think it makes sense, but I'm not sure how to phrase it because I'm not sure if we should mention variance, or only mention that the representations are not convertible in a parameter subtype context ...

Yeah, it's a good question how note should be worded exactly...

After thinking about this some more - I think we should just open a follow up SR for the note and merge what we have now.

I think it makes sense :)

lib/Sema/CSSimplify.cpp

xedin · 2020-06-05T20:43:50Z

@swift-ci please test

xedin · 2020-06-05T20:45:22Z

Source compatibily bot is having a bad day but we should run it before merging.

swift-ci · 2020-06-05T20:46:42Z

Build failed
Swift Test Linux Platform
Git Sha - aae4984328c5ac68850a08f24cfcdd736dd9741d

LucianoPAlmeida · 2020-06-05T21:07:13Z

Source compatibily bot is having a bad day but we should run it before merging.

Right 👍

Seems like there are some issues with the CI in general lately, Linux bots were consistently failing in some PRs

swift-ci · 2020-06-05T21:17:07Z

Build failed
Swift Test OS X Platform
Git Sha - aae4984328c5ac68850a08f24cfcdd736dd9741d

lib/Sema/CSSimplify.cpp

…k to thin and thick to thick representations on subtype context

…rtain representation convertions do not crash

lib/Sema/CSSimplify.cpp

…epresentation conversions.

xedin

Looks great, thank you!

xedin · 2020-06-15T18:28:56Z

@swift-ci please smoke test

xedin · 2020-06-15T18:29:07Z

@swift-ci please test source compatibility release

LucianoPAlmeida · 2020-06-16T00:28:26Z

@xedin Source compatibility release test ok, this is ready to go? :)

xedin · 2020-06-16T00:29:08Z

@LucianoPAlmeida

LucianoPAlmeida · 2020-06-16T00:31:44Z

Thank you!

xedin reviewed May 15, 2020

View reviewed changes

CodaFi reviewed May 15, 2020

View reviewed changes

lib/Sema/CSSimplify.cpp Show resolved Hide resolved

LucianoPAlmeida force-pushed the SR-12723-conventions branch from c353d28 to 767a860 Compare May 16, 2020 00:21

LucianoPAlmeida changed the title ~~[SR-12723] Check representations when matching function types for Subtype constraint sub kind~~ [SR-12723] [WIP] Check if params representations are foreign to emit C function on SILGen May 16, 2020

LucianoPAlmeida force-pushed the SR-12723-conventions branch 2 times, most recently from 3ef6321 to aae4984 Compare May 18, 2020 00:57

LucianoPAlmeida changed the title ~~[SR-12723] [WIP] Check if params representations are foreign to emit C function on SILGen~~ [SR-12723] Check if function type param representations conversion are valid SILGen May 18, 2020

LucianoPAlmeida commented May 18, 2020

View reviewed changes

test/SILGen/function_conversion_objc.swift Outdated Show resolved Hide resolved

LucianoPAlmeida force-pushed the SR-12723-conventions branch from aae4984 to 0ff01e1 Compare May 23, 2020 01:57

LucianoPAlmeida force-pushed the SR-12723-conventions branch from ebac5b3 to 30a499b Compare May 28, 2020 02:13

LucianoPAlmeida commented May 28, 2020

View reviewed changes

test/Sema/diag_subtype_argument_representation.swift Outdated Show resolved Hide resolved

LucianoPAlmeida force-pushed the SR-12723-conventions branch from 30a499b to f55cb4f Compare May 28, 2020 02:22

LucianoPAlmeida requested a review from xedin May 28, 2020 23:52

LucianoPAlmeida force-pushed the SR-12723-conventions branch 3 times, most recently from 589940f to cfdb14e Compare May 29, 2020 01:25

LucianoPAlmeida changed the title ~~[SR-12723] Check if function type param representations conversion are valid SILGen~~ [SR-12723][Sema] Validate function type param representations thick-to-thin conversions conversions May 29, 2020

LucianoPAlmeida requested a review from jckarter May 29, 2020 09:29

LucianoPAlmeida changed the title ~~[SR-12723][Sema] Validate function type param representations thick-to-thin conversions conversions~~ [SR-12723][Sema] Validate function type param representations thick-to-thin conversions May 29, 2020

LucianoPAlmeida force-pushed the SR-12723-conventions branch from cfdb14e to 2dec7c5 Compare May 30, 2020 00:33

xedin reviewed Jun 4, 2020

View reviewed changes

xedin reviewed Jun 5, 2020

View reviewed changes

lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved

xedin reviewed Jun 5, 2020

View reviewed changes

lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved

xedin reviewed Jun 5, 2020

View reviewed changes

lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved

LucianoPAlmeida force-pushed the SR-12723-conventions branch 2 times, most recently from d727d89 to fab5a46 Compare June 5, 2020 09:12

slavapestov approved these changes Jun 8, 2020

View reviewed changes

lib/Sema/CSSimplify.cpp Show resolved Hide resolved

LucianoPAlmeida added 4 commits June 8, 2020 20:36

[CSSimplify] Modify logic on match representations to allow ONLY thic…

8f2f14b

…k to thin and thick to thick representations on subtype context

[tests] Adding silgen validation tests for SR-12723 to ensure that ce…

bacbc57

…rtain representation convertions do not crash

[CSSimplify] Simplify logic for valiting to thick conversion on subtype

baccbde

[CSSimplify] Thin to thin is also allowed

d6bf34e

LucianoPAlmeida force-pushed the SR-12723-conventions branch 2 times, most recently from 1841c77 to 5928b12 Compare June 9, 2020 09:23

[CSSimplify] Abstract function conversion validation into static method

adba283

LucianoPAlmeida force-pushed the SR-12723-conventions branch from 5928b12 to adba283 Compare June 10, 2020 00:37

LucianoPAlmeida requested a review from xedin June 12, 2020 01:54

xedin reviewed Jun 15, 2020

View reviewed changes

lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved

[CSSimplify] Adjusting naming and comments of method that validates r…

da53129

…epresentation conversions.

xedin approved these changes Jun 15, 2020

View reviewed changes

LucianoPAlmeida merged commit d22821e into swiftlang:master Jun 16, 2020

LucianoPAlmeida deleted the SR-12723-conventions branch March 25, 2021 23:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SR-12723][Sema] Validate function type param representations thick-to-thin conversions #31814

[SR-12723][Sema] Validate function type param representations thick-to-thin conversions #31814

LucianoPAlmeida commented May 15, 2020 •

edited

Loading

xedin May 15, 2020

LucianoPAlmeida May 15, 2020

xedin May 15, 2020

LucianoPAlmeida May 15, 2020

LucianoPAlmeida May 15, 2020

xedin May 27, 2020

jckarter May 27, 2020

xedin May 27, 2020

LucianoPAlmeida May 27, 2020

LucianoPAlmeida May 28, 2020

slavapestov commented May 18, 2020

LucianoPAlmeida commented May 23, 2020

LucianoPAlmeida commented May 23, 2020

xedin Jun 4, 2020

xedin Jun 4, 2020

LucianoPAlmeida Jun 5, 2020

xedin Jun 5, 2020

xedin Jun 5, 2020

LucianoPAlmeida Jun 5, 2020

xedin Jun 5, 2020

xedin Jun 5, 2020

LucianoPAlmeida Jun 5, 2020

xedin commented Jun 5, 2020

xedin commented Jun 5, 2020

swift-ci commented Jun 5, 2020

LucianoPAlmeida commented Jun 5, 2020

swift-ci commented Jun 5, 2020

xedin left a comment

xedin commented Jun 15, 2020

xedin commented Jun 15, 2020

LucianoPAlmeida commented Jun 16, 2020

xedin commented Jun 16, 2020

LucianoPAlmeida commented Jun 16, 2020

[SR-12723][Sema] Validate function type param representations thick-to-thin conversions #31814

[SR-12723][Sema] Validate function type param representations thick-to-thin conversions #31814

Conversation

LucianoPAlmeida commented May 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slavapestov commented May 18, 2020

LucianoPAlmeida commented May 23, 2020

LucianoPAlmeida commented May 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xedin commented Jun 5, 2020

xedin commented Jun 5, 2020

swift-ci commented Jun 5, 2020

LucianoPAlmeida commented Jun 5, 2020

swift-ci commented Jun 5, 2020

xedin left a comment

Choose a reason for hiding this comment

xedin commented Jun 15, 2020

xedin commented Jun 15, 2020

LucianoPAlmeida commented Jun 16, 2020

xedin commented Jun 16, 2020

LucianoPAlmeida commented Jun 16, 2020

LucianoPAlmeida commented May 15, 2020 •

edited

Loading