Update function type mangling to use Clang type when applicable. #34057

varungandhi-apple · 2020-09-24T07:59:07Z

Original PR: [DNM][AST] Keep track of an "isDerivableFromSwiftType" bit for Clang types. #29349 (now outdated).
First three commits are from [NFC] Minor cleanup related to Clang types in SIL #34056 and can be ignored (they don't affect the following two commits)
Follow up to Propagate Clang function types through SIL #33085 (Propagate Clang types in SIL)

Marking as a draft as there are a bunch of TODOs.

Check SDKs for potential changes
Remove a bunch of silly whitespace changes
Add more tests
Updating printing to not print the cType if it is canonical.

varungandhi-apple · 2020-09-24T08:05:26Z

I grepped the macOS and iOS SDKs for @convention(c) functions; there are only a handful of cases, and those shouldn't have different mangling under -use-clang-function-types. For @convention(block), there are more cases, but they are almost exclusively @convention(block) () -> () or @convention(block) (Bool) -> (), so they shouldn't be affected either.

rjmccall · 2020-09-24T19:09:22Z

Bool could be affected because I assume that our canonical back-mapping for Bool is the C _Bool type, not ObjC's BOOL. Of course, that only affects non-ARM64 targets.

rjmccall

It's interesting to me that you decided that it was important to set this correctly up-front rather than determine it retroactively. Can you talk about what went into that decision?

docs/ABI/Mangling.rst

include/swift/AST/ExtInfo.h

varungandhi-apple · 2020-09-24T20:15:38Z

It's interesting to me that you decided that it was important to set this correctly up-front rather than determine it retroactively.

Are you asking about the isCanonical bit? It needs to be set to 0/1 when we create SILFunctionType values. Since SILFunctionType values are always canonical, and "undetermined" state would not be canonical, we would need to determine the bit at the moment of creating the SILFunctionType or earlier. There's a similar "problem" when we compute canonical FunctionType values for checking type equality. Given those constraints, I thought it would be easier to do it immediately when the ClangTypeInfo was being created. As a side-effect, it also makes debugging potential issues somewhat easier.

varungandhi-apple · 2020-09-24T22:08:03Z

Bool could be affected because I assume that our canonical back-mapping for Bool is the C _Bool type, not ObjC's BOOL. Of course, that only affects non-ARM64 targets.

You're right, it's being mapped to CBool. There's only 1 place which is using this in the macOS SDK: RealityKit's swiftinterface. 🤦🏼

final public class __AssetLoadRequest {
  // other stuff
  public typealias CompletionHandler = @convention(block) (Swift.Bool) -> Swift.Void
  final public func setCompletionHandler(_ handler: @escaping RealityKit.__AssetLoadRequest.CompletionHandler)
}

I think this is not going to be a problem though, since the class is not an @objc class, it is a Swift class. So when the swiftinterface is regenerated with -use-clang-function-types, it will either have cType: "void (^)(_Bool)" or nothing (since it's canonical, we technically don't need to emit it in the swiftinterface).

rjmccall · 2020-09-25T06:30:56Z

It's interesting to me that you decided that it was important to set this correctly up-front rather than determine it retroactively.

Are you asking about the isCanonical bit? It needs to be set to 0/1 when we create SILFunctionType values.

I was talking about the "derivable from Swift type" bit, which I'm guessing this PR used to call "isCanonical". (Thank you for renaming it; that would have been a very confusing name.) I don't understand why we would need to know this immediately when we create SILFunctionType values. The vast majority of SILFunctionTypes are never mangled, which is the only thing the bit affects.

rjmccall · 2020-09-25T06:45:49Z

Bool could be affected because I assume that our canonical back-mapping for Bool is the C _Bool type, not ObjC's BOOL. Of course, that only affects non-ARM64 targets.

You're right, it's being mapped to CBool. There's only 1 place which is using this in the macOS SDK: RealityKit's swiftinterface. 🤦🏼
final public class __AssetLoadRequest {
  // other stuff
  public typealias CompletionHandler = @convention(block) (Swift.Bool) -> Swift.Void
  final public func setCompletionHandler(_ handler: @escaping RealityKit.__AssetLoadRequest.CompletionHandler)
}
I think this is not going to be a problem though, since the class is not an @objc class, it is a Swift class. So when the swiftinterface is regenerated with -use-clang-function-types, it will either have cType: "void (^)(_Bool)" or nothing (since it's canonical, we technically don't need to emit it in the swiftinterface).

Just to be sure, can you check what type we actually use as the SILFunctionType for this? SIL type lowering for @convention(block) function types bridges the function's component types, so it's possible that this actually bridges to use ObjCBool despite being spelled Bool. I don't remember all the details there.

The key question for SDK binary compatibility is: what native interfaces use @convention(c) or @convention(block) types? Note that we have to consider uses of imported typealiases in those native Swift interfaces, but we don't have to consider uses in non-native interfaces because the Swift mangling isn't part of the interaction.

varungandhi-apple · 2020-09-25T21:07:10Z

Maybe I didn't express myself clearly. Here's what we need for FunctionType.

We need to compare FunctionType values for equality in Sema.
```
let f1 : @convention(c, cType: "fancyCType") <...> = <...>
let f2 : @convention(c) <...> = f1
```
This should generate a type error. Generating a type error here requires canonical type computation.
If hypothetically, we had ClangTypeInfo equivalent to:
```
struct ClangTypeInfoInternal {
  enum State {
    case unresolved // lazily resolved
    case derivableFromSwiftType
    case notDerivableFromSwiftType
  }
  var state : State
  var clangType : ClangType
}
typealias ClangTypeInfo = ClangTypeInfoInternal?
```
We would potentially need to update the state from either unresolved -> derivableFromSwiftType or unresolved -> notDerivableFromSwiftType when computing the canonical FunctionType, because we cannot meaningfully compare two ClangTypeInfo values when either of them is in the unresolved state.

For FunctionType, the idea of lazily resolving the state (in case it is .unresolved) is still do-able because canonical type computation is an actual separate step. Today, we modify the clangType var and make sure it is canonical when computing a canonical FunctionType, so we could also have the extra dance around updating the state in the same place.

For SILFunctionType, this is not do-able because there is no separate canonical type computation step, all of them are canonical. Setting state to unresolved would break the invariant that all SILFunctionType values are canonical.

As a matter of simplicity, I chose to treat both of these uniformly by setting the state eagerly and getting rid of the unresolved case.

So this statement:

The vast majority of SILFunctionTypes are never mangled, which is the only thing the bit affects.

is mostly correct but not 100% correct. The bit also affects type equality (and as part of it, canonical type computation).

varungandhi-apple · 2020-09-25T21:36:28Z

Talked with John separately and he mentioned that it would be better to have the "is derivable from Swift type" be a query which takes a SILFunctionType and can be used by the mangler, with the source of truth being the parameters + result + stored Clang type. The additional storage in the pointer can be used as a cache if needed, but it is no longer the source of truth.

I'm going to try out this approach, it seems simpler than what I have right now.

varungandhi-apple · 2020-09-29T20:42:30Z

While writing tests, ran into a pre-existing bug with the demangler where @convention attributes are not printed in certain conditions. For example:

xcrun swift-demangle 's4main1gyySiXCvp'
$s4main1gyySiXCvp ---> main.g : (Swift.Int) -> ()

Is missing an @convention(c) which is implied by the XC. Investigating. The node structure is correct:

    kind=Type
      kind=CFunctionPointer
        kind=ArgumentTuple
          kind=Type
            kind=Structure
              kind=Module, text="Swift"
              kind=Identifier, text="Int"
        kind=ReturnType
          kind=Type
            kind=Tuple

varungandhi-apple · 2020-09-30T00:32:40Z

Opened #34122 for the existing demangling issue.

…type.

varungandhi-apple · 2020-10-07T23:52:10Z

@swift-ci please test Windows

varungandhi-apple · 2020-10-07T23:52:18Z

@swift-ci please test

zoecarver · 2020-10-08T01:01:12Z

@swift-ci please test windows platform.

zoecarver · 2020-10-08T01:01:26Z

@swift-ci please test Windows platform.

swift-ci · 2020-10-08T01:08:19Z

Build failed
Swift Test Linux Platform
Git Sha - 5e4f8fa1d889bfc58009898cd93a988127241e50

varungandhi-apple · 2020-10-08T02:14:06Z

@swift-ci please test Windows platform

varungandhi-apple · 2020-10-08T02:14:16Z

@swift-ci please test

varungandhi-apple · 2020-10-08T04:02:12Z

@swift-ci please clean test Windows platform

varungandhi-apple · 2020-10-08T15:13:53Z

@swift-ci please test Windows platform

varungandhi-apple · 2020-10-09T03:58:45Z

@swift-ci please test Windows platform

varungandhi-apple · 2020-10-09T04:46:14Z

@swift-ci please test Windows platform

varungandhi-apple · 2020-10-09T15:48:09Z

Now the issue is size_t expands to unsigned long long on Windows, not unsigned long, which still means that the mangling is different on Windows. Fixing that...

varungandhi-apple · 2020-10-09T16:42:22Z

@swift-ci please test Windows

varungandhi-apple · 2020-10-09T16:47:20Z

@swift-ci please test Windows

varungandhi-apple · 2020-10-09T17:40:02Z

@swift-ci please test Windows

varungandhi-apple · 2020-10-09T19:36:45Z

@swift-ci please test Windows

varungandhi-apple · 2020-10-09T20:57:39Z

@swift-ci please test

lib/Demangling/OldDemangler.cpp

lib/AST/ASTMangler.cpp

include/swift/AST/ASTMangler.h

lib/AST/ASTPrinter.cpp

lib/Demangling/Remangler.cpp

test/ClangImporter/clang-function-types.swift

varungandhi-apple · 2020-10-15T21:48:51Z

@swift-ci please test

varungandhi-apple · 2020-10-15T22:35:10Z

@swift-ci please test Windows

rjmccall

I think all of my substantive comments are satisfied; LGTM.

test/Serialization/clang-function-types.swift

rjmccall · 2020-10-20T05:54:19Z

test/ModuleInterface/full-convention.swift

+
+  // CHECK: p3c: @convention(c)
+  // CHECK:      @convention(c, cType: "void (*)(size_t)")
+  p3c: @convention(c, cType: "void (*)(void (*)(size_t))") (@convention(c, cType: "void (*)(size_t)") (Int) -> Void) -> Void


I wonder what github is trying to tell us about these colons.

…pes.

varungandhi-apple · 2020-10-21T22:58:42Z

@swift-ci smoke test and merge

`size_t` differs in 32 bit platforms like Android ARMv7 where it is `unsigned int` instead of `unsigned long`. The mangling changes one of the characters. The original tests are disabled in Android, and they are replicated in different files for Android, with both the 32 and the 64 bits versions of the mangling and the function signatures. The original tests are not modified in order to avoid complicated checks to avoid platforms like iphonesimulator-i386, which is a 32 bit platform, but still uses `unsigned long` as the underlying type for `size_t` Both tests started failing after swiftlang#34057 landed in the Android 32 bits CI machines.

varungandhi-apple mentioned this pull request Sep 24, 2020

[DNM][AST] Keep track of an "isDerivableFromSwiftType" bit for Clang types. #29349

Closed

rjmccall reviewed Sep 24, 2020

View reviewed changes

docs/ABI/Mangling.rst Outdated Show resolved Hide resolved

include/swift/AST/ExtInfo.h Outdated Show resolved Hide resolved

varungandhi-apple force-pushed the vg-track-isDerivable-update-mangling branch from ea58b6e to 59434a6 Compare September 26, 2020 00:12

varungandhi-apple force-pushed the vg-track-isDerivable-update-mangling branch from 59434a6 to f804ecb Compare October 6, 2020 20:39

[AST] Add method to check if a function type has a non-trivial Clang …

be109b7

…type.

varungandhi-apple force-pushed the vg-track-isDerivable-update-mangling branch from f804ecb to 5e4f8fa Compare October 7, 2020 23:51

varungandhi-apple marked this pull request as ready for review October 7, 2020 23:51

varungandhi-apple changed the title ~~Propagate 'isCanonical' bit for Clang types and use it for mangling.~~ Update function type mangling to use Clang type when applicable. Oct 8, 2020

varungandhi-apple force-pushed the vg-track-isDerivable-update-mangling branch from 5e4f8fa to 650fadc Compare October 8, 2020 02:13

varungandhi-apple force-pushed the vg-track-isDerivable-update-mangling branch from 650fadc to 2aa73a1 Compare October 9, 2020 03:58

varungandhi-apple force-pushed the vg-track-isDerivable-update-mangling branch from 2aa73a1 to 1c43570 Compare October 9, 2020 16:41

varungandhi-apple force-pushed the vg-track-isDerivable-update-mangling branch from 1c43570 to 44b017c Compare October 9, 2020 17:40

rjmccall reviewed Oct 12, 2020

View reviewed changes

lib/Demangling/OldDemangler.cpp Show resolved Hide resolved

zoecarver reviewed Oct 12, 2020

View reviewed changes

lib/AST/ASTMangler.cpp Show resolved Hide resolved

zoecarver reviewed Oct 12, 2020

View reviewed changes

include/swift/AST/ASTMangler.h Show resolved Hide resolved

lib/AST/ASTPrinter.cpp Outdated Show resolved Hide resolved

lib/Demangling/Remangler.cpp Outdated Show resolved Hide resolved

test/ClangImporter/clang-function-types.swift Show resolved Hide resolved

varungandhi-apple force-pushed the vg-track-isDerivable-update-mangling branch 2 times, most recently from ccd6d2f to e90e486 Compare October 15, 2020 21:48

rjmccall approved these changes Oct 20, 2020

View reviewed changes

varungandhi-apple added 2 commits October 21, 2020 15:57

[Mangler] Handle mangling for Clang types not derivable from Swift ty…

987d055

…pes.

[ASTPrinter] Print Clang type only if not derivable from Swift type.

6cb71c6

varungandhi-apple force-pushed the vg-track-isDerivable-update-mangling branch from e90e486 to 6cb71c6 Compare October 21, 2020 22:58

swift-ci merged commit cb900a6 into swiftlang:main Oct 22, 2020

drodriguez mentioned this pull request Nov 3, 2020

[android] Split demangling tests for Android 32/64 bits. #34554

Merged

Update function type mangling to use Clang type when applicable. #34057

Update function type mangling to use Clang type when applicable. #34057

Uh oh!

Conversation

varungandhi-apple commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varungandhi-apple commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjmccall commented Sep 24, 2020

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

varungandhi-apple commented Sep 24, 2020

Uh oh!

varungandhi-apple commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjmccall commented Sep 25, 2020

Uh oh!

rjmccall commented Sep 25, 2020

Uh oh!

varungandhi-apple commented Sep 25, 2020

Uh oh!

varungandhi-apple commented Sep 25, 2020

Uh oh!

varungandhi-apple commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varungandhi-apple commented Sep 30, 2020

Uh oh!

varungandhi-apple commented Oct 7, 2020

Uh oh!

varungandhi-apple commented Oct 7, 2020

Uh oh!

zoecarver commented Oct 8, 2020

Uh oh!

zoecarver commented Oct 8, 2020

Uh oh!

swift-ci commented Oct 8, 2020

Uh oh!

varungandhi-apple commented Oct 8, 2020

Uh oh!

varungandhi-apple commented Oct 8, 2020

Uh oh!

varungandhi-apple commented Oct 8, 2020

Uh oh!

varungandhi-apple commented Oct 8, 2020

Uh oh!

varungandhi-apple commented Oct 9, 2020

Uh oh!

varungandhi-apple commented Oct 9, 2020

Uh oh!

varungandhi-apple commented Oct 9, 2020

Uh oh!

varungandhi-apple commented Oct 9, 2020

Uh oh!

varungandhi-apple commented Oct 9, 2020

Uh oh!

varungandhi-apple commented Oct 9, 2020

Uh oh!

varungandhi-apple commented Oct 9, 2020

Uh oh!

varungandhi-apple commented Oct 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

varungandhi-apple commented Oct 15, 2020

Uh oh!

varungandhi-apple commented Oct 15, 2020

Uh oh!

rjmccall left a comment

varungandhi-apple commented Sep 24, 2020 •

edited

Loading

varungandhi-apple commented Sep 24, 2020 •

edited

Loading

varungandhi-apple commented Sep 24, 2020 •

edited

Loading

varungandhi-apple commented Sep 29, 2020 •

edited

Loading