Propagate Clang function types through SIL #33085

varungandhi-apple · 2020-07-23T22:47:13Z

Re-landing the patch from #29691.

There are still some issues that need to be ironed out:

~~During type lowering, the Clang type is not being computed when we make an @convention(c) function. As far as I know, this is the only place missing, but I need to investigate more.~~ [Updated the primary function in type lowering which does the adjustment, but there still seem to be spots which haven't been fixed..] We see an assertion failure in IRGen where the remangling check fails (the original type does not store a Clang type but the demangled type does).
I'm not sure if the changes from isDerivable bit PR [DNM][AST] Keep track of an "isDerivableFromSwiftType" bit for Clang types. #29349 should be also included in this PR or land that separately. There are a couple of possible strategies: (1) land this PR, land the isDerivable bit PR, land the groundwork for the C++ template importing or (2) land this PR, land the groundwork for the C++ template importing, land the isDerivable bit PR. (The isDerivable bit is necessary so that the name mangling for @convention(c) functions which have a derivable C type doesn't change).

More generally, I feel like the API around ExtInfo is a bit problematic; there is no good place to enforce invariants such as if rep == CFunctionPointer || rep == Block, then you must have an associated Clang type. The presence of the with methods means that you are sometimes forced to create illegal intermediate states when creating new ExtInfo values. I'm wondering if we should refactor it to use some kind of "builder" API where you have an ExtInfoBuilder which represents a WIP state, and then you call a finish : (ExtInfoBuilder) -> ExtInfo method which checks if the builder is well-formed, before emitting an ExtInfo.

lib/AST/Decl.cpp

varungandhi-apple · 2020-07-24T16:41:35Z

The presence of the with methods means that you are sometimes forced to create illegal intermediate states when creating new ExtInfo values.

This also manifests as us having code like the following, because we are not enforcing the invariant rep == CFunctionPointer ==> NoEscape = true.

static bool isEscaping(Type type) {
  if (auto *funcType = type->getAs<AnyFunctionType>()) {
    if (funcType->getExtInfo().getRepresentation() ==
          FunctionTypeRepresentation::CFunctionPointer)
      return false;

    return !funcType->getExtInfo().isNoEscape();
  }

  return false;
}

This is... not great IMO. If I see the isNoEscape() method, it seems reasonable to expect it to return the right value, regardless of what the representation is.

varungandhi-apple · 2020-07-25T18:15:29Z

We should land the refactoring in #33118 first, assuming no one has major objections.

varungandhi-apple · 2020-08-19T00:14:39Z

To give an update here:

I've broken out the first two commits into a separate PR Setup code for Clang types in SIL. #33541, along with some cleanup.
Asserting that a function type with @convention(c) or @convention(block) always carries a clang::Type has been trickier than expected; I will update this PR once that is done. At the moment, I'm changing parts of lowering to use SILExtInfoBuilder instead of an ASTExtInfo with a SIL representation (because the isPseudogeneric bit is computed right before creating the Clang type, but the rest of the ExtInfo including a SIL representation needs to be passed in to getSILFunctionType). However there are still issues to be ironed out due to the convention not being set correctly in certain places during lowering.

varungandhi-apple · 2020-08-21T22:36:23Z

Well, we still have merge conflicts. 💩

~~Going to land PR #33541 first, then rebase this on top of master.~~ FIXED

varungandhi-apple · 2020-08-24T01:42:17Z

I ended up rebasing the base PR #33541, force-pushing and then started rebasing this PR. The conflicts that came up are coming up primarily due to changes to SILFunctionType (it is storing an extra async bit) in #33554 and related PRs. It would be better to fix that bit first and move it to SILExtInfoBuilder (I've pointed this out in a comment in #33554) and then rebase this PR. FIXED

varungandhi-apple · 2020-08-28T05:54:20Z

I wonder why this fails only on Linux. Not seeing a similar issue building SwiftPM on macOS, or with small snippets using higher-order functions on macOS.

Incorrect reconstructed type for $sSvSgA3ASbIetCyyd_SgSbIetCyyyd_SgD
Original type:
(bound_generic_enum_type decl=Swift.(file).Optional
  (sil_function_type type=@convention(c) (Optional, Optional, Optional<@convention(c) (Optional, Optional) -> Bool>) -> Bool
    (input=bound_generic_enum_type decl=Swift.(file).Optional
      (struct_type decl=Swift.(file).UnsafeMutableRawPointer))
    (input=bound_generic_enum_type decl=Swift.(file).Optional
      (struct_type decl=Swift.(file).UnsafeMutableRawPointer))
    (input=bound_generic_enum_type decl=Swift.(file).Optional
      (sil_function_type type=@convention(c) (Optional, Optional) -> Bool
        (input=bound_generic_enum_type decl=Swift.(file).Optional
          (struct_type decl=Swift.(file).UnsafeMutableRawPointer))
        (input=bound_generic_enum_type decl=Swift.(file).Optional
          (struct_type decl=Swift.(file).UnsafeMutableRawPointer))
        (result=struct_type decl=Swift.(file).Bool)
        (substitution_map generic_signature=)
        (substitution_map generic_signature=) clang_type=PointerType 0xb52e540 '_Bool (*)(void *, void *)'
`-FunctionProtoType 0xb52e490 '_Bool (void *, void *)' cdecl
  |-BuiltinType 0x924bb70 '_Bool'
  |-PointerType 0x924c310 'void *' imported
  | `-BuiltinType 0x924bb50 'void'
  `-PointerType 0x924c310 'void *' imported
    `-BuiltinType 0x924bb50 'void'
))
    (result=struct_type decl=Swift.(file).Bool)
    (substitution_map generic_signature=)
    (substitution_map generic_signature=)))
Reconstructed type:
(bound_generic_enum_type decl=Swift.(file).Optional
  (sil_function_type type=@convention(c) (Optional, Optional, Optional<@convention(c) (Optional, Optional) -> Bool>) -> Bool
    (input=bound_generic_enum_type decl=Swift.(file).Optional
      (struct_type decl=Swift.(file).UnsafeMutableRawPointer))
    (input=bound_generic_enum_type decl=Swift.(file).Optional
      (struct_type decl=Swift.(file).UnsafeMutableRawPointer))
    (input=bound_generic_enum_type decl=Swift.(file).Optional
      (sil_function_type type=@convention(c) (Optional, Optional) -> Bool
        (input=bound_generic_enum_type decl=Swift.(file).Optional
          (struct_type decl=Swift.(file).UnsafeMutableRawPointer))
        (input=bound_generic_enum_type decl=Swift.(file).Optional
          (struct_type decl=Swift.(file).UnsafeMutableRawPointer))
        (result=struct_type decl=Swift.(file).Bool)
        (substitution_map generic_signature=)
        (substitution_map generic_signature=) clang_type=PointerType 0xb52e540 '_Bool (*)(void *, void *)'
`-FunctionProtoType 0xb52e490 '_Bool (void *, void *)' cdecl
  |-BuiltinType 0x924bb70 '_Bool'
  |-PointerType 0x924c310 'void *' imported
  | `-BuiltinType 0x924bb50 'void'
  `-PointerType 0x924c310 'void *' imported
    `-BuiltinType 0x924bb50 'void'
))
    (result=struct_type decl=Swift.(file).Bool)
    (substitution_map generic_signature=)
    (substitution_map generic_signature=) clang_type=PointerType 0xb4e82c0 '_Bool (*)(void *, void *, _Bool (*)(void *, void *))'
`-FunctionProtoType 0xb4e8210 '_Bool (void *, void *, _Bool (*)(void *, void *))' cdecl
  |-BuiltinType 0x924bb70 '_Bool'
  |-PointerType 0x924c310 'void *' imported
  | `-BuiltinType 0x924bb50 'void'
  |-PointerType 0x924c310 'void *' imported
  | `-BuiltinType 0x924bb50 'void'
  `-PointerType 0xb52e540 '_Bool (*)(void *, void *)'
    `-FunctionProtoType 0xb52e490 '_Bool (void *, void *)' cdecl
      |-BuiltinType 0x924bb70 '_Bool'
      |-PointerType 0x924c310 'void *' imported
      | `-BuiltinType 0x924bb50 'void'
      `-PointerType 0x924c310 'void *' imported
        `-BuiltinType 0x924bb50 'void'
))

varungandhi-apple · 2020-08-29T05:18:01Z

Did manage to reproduce the issue on Linux. The problem is coming up due to https://github.com/apple/swift-package-manager/blob/96b0ff907fdd4d91fcd207f4c49aae4fce1d9ec5/swift-tools-support-core/Sources/TSCUtility/IndexStore.swift#L215-L215 . We end up specializing a dlsym wrapper (!) with the generic parameter having type (@convention(c) (UnsafeMutableRawPointer?, UnsafeMutableRawPointer?, (@convention(c) (UnsafeMutableRawPointer?, UnsafeMutableRawPointer?) -> Bool)?) -> Bool)? thanks to
https://github.com/apple/indexstore-db/blob/8c593c430c714aac7e777b501f26e8e36849315e/include/indexstore/indexstore_functions.h#L539-L542

Not entirely sure what is dropping the Clang type though, haven't been able to create a minimal example with the same type on macOS. 🤔

EDIT: 🤦🏼 GitHub

This type of permanent link will render as a code snippet only in the repository it originated in. In other repositories, the permalink code snippet will render as a URL.

varungandhi-apple · 2020-08-29T07:33:22Z

OK, there are at least two issues here.

~~We are not calling checkInvariants() in all the right constructors.~~ FIXED
UseClangFunctionTypes really needs to be turned on at this point. Either that, or the mangling code needs to be relaxed or we add a temporary re-computation step that needs to be removed later. Otherwise, if the setting is false, we end up dropping Clang function types when computing canonical AST function types, which means that when we create a SIL ExtInfo for a pointer to a higher-order function, we may no longer have the Clang type for a parameter. FIXED

Fixing those two things leads to crashes elsewhere from the little that I tested, so there's probably a third problem somewhere.

Still not entirely clear why we hit this on Linux but not on macOS. 🤷🏼

varungandhi-apple · 2020-09-02T05:49:40Z

stdlib/Random.swift is failing with a deserialization error on 32-bit iOS simulator.

******************** TEST 'Swift(iphonesimulator-i386) :: stdlib/Random.swift' FAILED ******************** Script: -- : 'RUN: at line 1'; rm -rf "/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/validation-test-iphonesimulator-i386/stdlib/Output/Random.swift.tmp" && mkdir -p "/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/validation-test-iphonesimulator-i386/stdlib/Output/Random.swift.tmp" && xcrun --toolchain default --sdk '/Applications/Xcode-beta.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.0.sdk' /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/bin/swiftc -target i386-apple-ios7.0-simulator -module-cache-path '/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/swift-test-results/i386-apple-ios7.0-simulator/clang-module-cache' -F '/Applications/Xcode-beta.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/Library/Frameworks' -toolchain-stdlib-rpath -Xlinker -rpath -Xlinker /usr/lib/swift -swift-version 4 -Xfrontend -ignore-module-source-info -module-cache-path '/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/swift-test-results/i386-apple-ios7.0-simulator/clang-module-cache' /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/validation-test/stdlib/Random.swift -o /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/validation-test-iphonesimulator-i386/stdlib/Output/Random.swift.tmp/a.out -module-name main -Xfrontend -disable-access-control && codesign -f -s - /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/validation-test-iphonesimulator-i386/stdlib/Output/Random.swift.tmp/a.out && /usr/bin/env DYLD_LIBRARY_PATH='/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/lib/swift/iphonesimulator' LD_LIBRARY_PATH='/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/lib/swift/iphonesimulator:' SIMCTL_CHILD_DYLD_LIBRARY_PATH='/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/lib/swift/iphonesimulator' xcrun --toolchain default --sdk '/Applications/Xcode-beta.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.0.sdk' simctl spawn --standalone 'iPhone 5' /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/validation-test-iphonesimulator-i386/stdlib/Output/Random.swift.tmp/a.out -- Exit Code: 254

Command Output (stderr):

/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/validation-test/stdlib/Random.swift:28:9: warning: variable 'g' was never mutated; consider changing to 'let' constant
var g = SystemRandomNumberGenerator()
~~~ ^
let
:0: error: fatal error encountered while reading from module 'Swift'; please file a bug report with your project and the crash log
:0: note: module 'Swift' full misc version is '5.3(5.3)/Apple Swift version 5.3-dev (LLVM 19447e27024a8d4, Swift 19032d59c06c27b)'
:0: note: compiling as Swift 4.1.50, with 'Swift' built as Swift 5.3 (this is supported but may expose additional compiler issues)

*** DESERIALIZATION FAILURE (please include this section in any bug report) ***
(see "While..." info below)
Stack dump:
0. Program arguments: /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/bin/swift-frontend -frontend -c -primary-file /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/validation-test/stdlib/Random.swift -target i386-apple-ios7.0-simulator -enable-objc-interop -sdk /Applications/Xcode-beta.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.0.sdk -F /Applications/Xcode-beta.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/Library/Frameworks -module-cache-path /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/swift-test-results/i386-apple-ios7.0-simulator/clang-module-cache -swift-version 4 -ignore-module-source-info -disable-access-control -target-sdk-version 14.0 -module-name main -o /var/folders/_8/79jmzf2142z2xydc_01btlx00000gn/T/Random-081e9b.o

Apple Swift version 5.3-dev (LLVM 19447e27024a8d4, Swift 19032d59c06c27b)
While evaluating request ExecuteSILPipelineRequest(Run pipelines { Mandatory Diagnostic Passes + Enabling Optimization Passes } on SIL for main.main)
While running pass #425 SILModuleTransform "MandatorySILLinker".
While deserializing SIL function "swift_stdlib_random"
0 swift-frontend 0x000000010bce7b75 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 37
1 swift-frontend 0x000000010bce6b48 llvm::sys::RunSignalHandlers() + 248
2 swift-frontend 0x000000010bce8156 SignalHandler(int) + 262
3 libsystem_platform.dylib 0x00007fff6f1da5fd _sigtramp + 29
4 libsystem_platform.dylib 0x00007ffee844a9b0 _sigtramp + 18446744071447184336
5 libsystem_c.dylib 0x00007fff6f0b0808 abort + 120
6 swift-frontend 0x000000010817bdfe swift::ModuleFileSharedCore::fatal(llvm::Error) + 110
7 swift-frontend 0x0000000108121eef swift::ModuleFile::fatal(llvm::Error) + 63
8 swift-frontend 0x0000000108174eac swift::ModuleFile::fatal() + 60
9 swift-frontend 0x000000010815332b swift::SILDeserializer::readSILFunctionChecked(llvm::PointerEmbeddedInt<unsigned int, 31>, swift::SILFunction*, llvm::StringRef, bool, bool) + 8283
10 swift-frontend 0x0000000108166500 swift::SILDeserializer::lookupSILFunction(swift::SILFunction*, bool) + 368
11 swift-frontend 0x00000001081e85b5 swift::SerializedSILLoader::lookupSILFunction(swift::SILFunction*, bool) + 117
12 swift-frontend 0x0000000108213e08 swift::SILLinkerVisitor::process() + 472
13 swift-frontend 0x0000000108213c02 swift::SILLinkerVisitor::processFunction(swift::SILFunction*) + 82
14 swift-frontend 0x000000010827a702 swift::SILModule::linkFunction(swift::SILFunction*, swift::SILModule::LinkingMode) + 114
15 swift-frontend 0x0000000107fe2dae (anonymous namespace)::SILLinker::run() + 78
16 swift-frontend 0x0000000107ea649e swift::SILPassManager::runModulePass(unsigned int) + 558
17 swift-frontend 0x0000000107eaaeba swift::SILPassManager::execute() + 666
18 swift-frontend 0x0000000107ea2af8 swift::SILPassManager::executePassPipelinePlan(swift::SILPassPipelinePlan const&) + 72
19 swift-frontend 0x0000000107ea2a93 swift::ExecuteSILPipelineRequest::evaluate(swift::Evaluator&, swift::SILPipelineExecutionDescriptor) const + 51
20 swift-frontend 0x0000000107ec5dcd swift::SimpleRequest<swift::ExecuteSILPipelineRequest, std::__1::tuple<> (swift::SILPipelineExecutionDescriptor), (swift::RequestFlags)1>::evaluateRequest(swift::ExecuteSILPipelineRequest const&, swift::Evaluator&) + 29
21 swift-frontend 0x0000000107ead57b llvm::Expectedswift::ExecuteSILPipelineRequest::OutputType swift::Evaluator::getResultUncachedswift::ExecuteSILPipelineRequest(swift::ExecuteSILPipelineRequest const&) + 363
22 swift-frontend 0x0000000107ea2d24 swift::executePassPipelinePlan(swift::SILModule*, swift::SILPassPipelinePlan const&, bool, swift::irgen::IRGenModule*) + 68
23 swift-frontend 0x0000000107eb04b7 swift::runSILDiagnosticPasses(swift::SILModule&) + 87
24 swift-frontend 0x000000010790f488 swift::CompilerInstance::performSILProcessing(swift::SILModule*) + 56
25 swift-frontend 0x0000000107822b1e performCompileStepsPostSILGen(swift::CompilerInstance&, std::__1::unique_ptr<swift::SILModule, std::__1::default_deleteswift::SILModule >, llvm::PointerUnion<swift::ModuleDecl*, swift::SourceFile*>, swift::PrimarySpecificPaths const&, int&, swift::FrontendObserver*) + 894
26 swift-frontend 0x000000010782245e performCompileStepsPostSema(swift::CompilerInstance&, int&, swift::FrontendObserver*) + 350
27 swift-frontend 0x000000010781a8ed swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 4589
28 swift-frontend 0x00000001077b510d main + 861
29 libdyld.dylib 0x00007fff6efe1cc9 start + 1
:0: error: unable to execute command: Abort trap: 6
:0: error: compile command failed due to signal 6 (use -v to see invocation)

varungandhi-apple · 2020-09-02T21:21:09Z

For 32-bit iOS/watchOS, both int and long are 32-bits. The failure happens in

https://github.com/apple/swift/blob/bfc9b5447f4499bfb363b948b91524ad6e5759a7/lib/Serialization/DeserializeSIL.cpp#L589-L594

The deserialized type ty ends up having a long, whereas the existing function fn ends up having an int. The Clang types end up being different, so the Swift types end up being different. 😬

varungandhi-apple · 2020-09-03T04:19:02Z

Reduced test case:

// RUN: %target-swift-frontend %s -emit-ir -o /dev/null

import StdlibUnittest
import SwiftShims
let _ = swift_stdlib_random

Here, swift_stdlib_random has the second argument as size_t, which is a typedef for long unsigned int. I'm not really sure why ty ends up getting a long above.

varungandhi-apple · 2020-09-03T06:55:52Z

Looking at a slightly less complicated example.

// RUN: %target-swift-frontend %s -emit-ir -o /dev/null

import StdlibUnittest
import SwiftShims

// void swift_stdlib_random(void *buf, __swift_size_t nbytes);
func f(_ p: UnsafeMutableRawPointer, _ c: Int) { swift_stdlib_random(p, c) }

In applyNormalCall:

RValue CallEmission::applyNormalCall(SGFContext C) {
  // We use the context emit-into initialization only for the
  // outermost call.
  SGFContext uncurriedContext = C;

  auto formalType = callee.getSubstFormalType();
  // ^ @convention(thin) (UnsafeMutableRawPointer, Int) -> ()
  auto origFormalType = callee.getOrigFormalType();
  // ^ @convention(c, cType: "void (*)(void * _Nonnull, __swift_size_t)") (UnsafeMutableRawPointer, Int) -> ()

  bool isCurried = (uncurriedSites.size() < callee.getParameterListCount());
  assert(!isCurried);

  // Get the callee type information.
  auto calleeTypeInfo = callee.getTypeInfo(SGF);
  // ^ calleeTypeInfo.substFnType = @convention(c, cType: "void (*)(void *, int)") (UnsafeMutableRawPointer, Int) -> ()
  // BAD! We shouldn't have dropped the info from the original formal type and only
  // to recompute the information from the Swift type later on.

Looking at getTypeInfo, it seems like something is off with the SILDeclRef Constant being stored in the Callee class instance.

We end up creating a SILFunction fn for swift_stdlib_random with type @convention(c, cType: "void (*)(void *, int)") (UnsafeMutableRawPointer, Int) -> () thanks to the above code. This gets passed into SIL deserialization to update the function's linkage. However, during deserialization, we end up getting a void (*)(void *, long) Clang type for this function. This is strange because:

__swift_size_t is specially imported into Swift as Int according to MappedTypes.def.
__swift_size_t is defined as a typedef to (essentially) long unsigned int. Notice the unsigned. But somehow the unsigned is gone? 🤔

Due to this mismatch, deserialization falls over.

rjmccall · 2020-09-03T17:55:33Z

Why is deserialization falling over? The import mapping is definitely not 1-1; certain unsigned types being imported as Int is the least of the problems there. But I don't know why the type would be inconsistent in different sources.

varungandhi-apple · 2020-09-03T18:38:43Z

Deserialization falls over because, at the moment, the types are expected to match up exactly. It's not clear to me what ought to be done in case of a mismatch here.

https://github.com/apple/swift/blob/bfc9b5447f4499bfb363b948b91524ad6e5759a7/lib/Serialization/DeserializeSIL.cpp#L589-L594

The import mapping is definitely not 1-1; certain unsigned types being imported as Int is the least of the problems there.

Yeah, I'm not saying that's a problem. But we need to maintain a consistent view internally. I think something is off with this part I highlighted earlier:

  auto formalType = callee.getSubstFormalType();
  // ^ @convention(thin) (UnsafeMutableRawPointer, Int) -> ()
  auto origFormalType = callee.getOrigFormalType();
  // ^ @convention(c, cType: "void (*)(void * _Nonnull, __swift_size_t)") (UnsafeMutableRawPointer, Int) -> ()

  auto calleeTypeInfo = callee.getTypeInfo(SGF);
  // ^ calleeTypeInfo.substFnType = @convention(c, cType: "void (*)(void *, int)") (UnsafeMutableRawPointer, Int) -> ()

It's not clear to me why the substituted formal type is @convention(thin) instead of @convention(c). If that's necessary, then we need a way to make sure that calleeTypeInfo.substFnType ends up using the Clang type from the original formal type.

rjmccall · 2020-09-03T20:53:21Z

SIL type lowering definitely needs to be using the Clang type from the original formal type if there is one.

varungandhi-apple · 2020-09-04T23:34:57Z

So I misspoke slightly here. The original formal type somehow ends up being @convention(thin, cType: "void (*)(void * _Nonnull, __swift_size_t)"), which should be impossible. The code for creating an ASTExtInfo value is very straight-forward... and I don't see anything off there. Going to try to use ASan+UBSan to see if there's any memory corruption...

varungandhi-apple · 2020-09-05T00:44:08Z

OK. I realized my mistake. I was looking at an AbstractionPattern, not an @convention(thin) function carrying a Clang type. It is possible for an abstraction pattern to have an @convention(thin) function and also simultaneously carry a Clang type.

The problem (or at least one of them) was that when we create the SILConstantInfo, the formal and lowered types end up being @convention(thin), but the SILFnType ends up being @convention(c). However, we were passing in the ExtInfo through the lowered type for creating the SILFnType, instead of using the one in bridgedTypes.Pattern.

varungandhi-apple · 2020-09-09T03:34:14Z

@swift-ci please test

varungandhi-apple · 2020-09-09T03:53:09Z

I think this is ready for review @rjmccall.

I've left the EqualUpToClangTypes check in as a stop-gap measure to prevent the compiler from crashing while still having round-trip testing when building with UseClangFunctionTypes turned on. I intend on removing it in a follow-up PR which adds the mangling.
I have some basic tests in place, should I add more test cases or do you think this is sufficient?
There are a bunch of formatting changes due to clang-format, which make this change larger than it ought to be. 🤷🏼

varungandhi-apple · 2020-09-10T05:29:25Z

@swift-ci please test source compatibility

rjmccall

Sorry for the slow review. Could you make a separate PR that just does the NFC set-up work, like converting various places to use ExtInfoBuilder or ExtInfo::getThin()?

include/swift/AST/ExtInfo.h

include/swift/AST/PrintOptions.h

lib/AST/ExtInfo.cpp

lib/SIL/IR/SILFunctionType.cpp

This patch includes a large number of changes to make sure that: 1. When ExtInfo values are created, we store a ClangTypeInfo if applicable. 2. We reduce dependence on storing SIL representations in ASTExtInfo values. 3. Reduce places where we sloppily create ASTExtInfo values which should store a Clang type but don't. In certain places, this is unavoidable; see [NOTE: ExtInfo-Clang-type-invariant]. Ideally, we would check that the appropriate SILExtInfo does always store a ClangTypeInfo. However, the presence of the HasClangFunctionTypes option means that we would need to condition that assertion based on a dynamic check. Plumbing the setting down to SILExtInfoBuilder's checkInvariants would be too much work. So we weaken the check for now; we should strengthen it once we "turn on" HasClangFunctionTypes and remove the dynamic feature switch.

varungandhi-apple · 2020-09-16T19:05:45Z

Fixed the bug in UnexpectedClangTypeError::dump and removed a couple of cleanup commits which can be added separately (will land those in a follow-up PR).

@swift-ci test

varungandhi-apple · 2020-09-17T02:30:10Z

@swift-ci test

swift-ci · 2020-09-17T02:33:06Z

Build failed
Swift Test Linux Platform
Git Sha - 3db2d21f55e7526e343a606dcdda5f313a6eb572

swift-ci · 2020-09-17T02:34:33Z

Build failed
Swift Test OS X Platform
Git Sha - 3db2d21f55e7526e343a606dcdda5f313a6eb572

rjmccall

Generally looks good to me.

varungandhi-apple · 2020-09-22T05:54:06Z

@swift-ci please test Windows

varungandhi-apple · 2020-09-22T05:55:24Z

@swift-ci test macOS platform

varungandhi-apple · 2020-09-22T05:55:41Z

@swift-ci test Linux platform

varungandhi-apple · 2020-09-22T07:09:53Z

@swift-ci please clean test Windows

varungandhi-apple requested a review from rjmccall July 23, 2020 22:47

varungandhi-apple mentioned this pull request Jul 23, 2020

[cxx-interop] Support function templates. #33053

Merged

varungandhi-apple commented Jul 23, 2020

View reviewed changes

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

varungandhi-apple force-pushed the vg-clang-types-in-sil-retry branch from 7e449d1 to f64538e Compare July 24, 2020 04:57

varungandhi-apple mentioned this pull request Aug 19, 2020

Setup code for Clang types in SIL. #33541

Merged

varungandhi-apple force-pushed the vg-clang-types-in-sil-retry branch from f64538e to 76309a9 Compare August 21, 2020 22:33

varungandhi-apple marked this pull request as ready for review August 21, 2020 22:34

varungandhi-apple force-pushed the vg-clang-types-in-sil-retry branch from 76309a9 to e16ea36 Compare August 27, 2020 22:58

varungandhi-apple force-pushed the vg-clang-types-in-sil-retry branch 3 times, most recently from 41acabb to bf0d615 Compare September 1, 2020 18:44

varungandhi-apple force-pushed the vg-clang-types-in-sil-retry branch 2 times, most recently from 928f0c7 to 1cbdee0 Compare September 6, 2020 08:15

swiftlang deleted a comment from swift-ci Sep 7, 2020

varungandhi-apple force-pushed the vg-clang-types-in-sil-retry branch from d488957 to 3db2d21 Compare September 8, 2020 23:34

rjmccall reviewed Sep 16, 2020

View reviewed changes

varungandhi-apple added 2 commits September 16, 2020 10:34

[Printer] Conditionally print Clang types in emitted SIL.

983399c

varungandhi-apple force-pushed the vg-clang-types-in-sil-retry branch from 3db2d21 to 983399c Compare September 16, 2020 19:01

rjmccall approved these changes Sep 22, 2020

View reviewed changes

varungandhi-apple merged commit 7b967a8 into swiftlang:master Sep 22, 2020

varungandhi-apple deleted the vg-clang-types-in-sil-retry branch September 22, 2020 15:48

This was referenced Sep 24, 2020

[NFC] Minor cleanup related to Clang types in SIL #34056

Merged

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

Merged

Propagate Clang function types through SIL #33085

Propagate Clang function types through SIL #33085

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

varungandhi-apple commented Jul 24, 2020

Uh oh!

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

Uh oh!

Uh oh!

varungandhi-apple commented Aug 19, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

varungandhi-apple commented Aug 28, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Command Output (stderr):

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

varungandhi-apple commented Sep 3, 2020

Uh oh!

rjmccall commented Sep 3, 2020

Uh oh!

varungandhi-apple commented Sep 3, 2020

Uh oh!

rjmccall commented Sep 3, 2020

Uh oh!

varungandhi-apple commented Sep 4, 2020

Uh oh!

varungandhi-apple commented Sep 5, 2020

Uh oh!

varungandhi-apple commented Sep 9, 2020

Uh oh!

varungandhi-apple commented Sep 9, 2020

Uh oh!

varungandhi-apple commented Sep 10, 2020

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

varungandhi-apple commented Sep 16, 2020

Uh oh!

varungandhi-apple commented Sep 17, 2020

Uh oh!

swift-ci commented Sep 17, 2020

Uh oh!

swift-ci commented Sep 17, 2020

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

varungandhi-apple commented Sep 22, 2020

Uh oh!

varungandhi-apple commented Sep 22, 2020

Uh oh!

varungandhi-apple commented Jul 23, 2020 •

edited

Loading

varungandhi-apple commented Jul 25, 2020 •

edited

Loading

varungandhi-apple commented Aug 21, 2020 •

edited

Loading

varungandhi-apple commented Aug 24, 2020 •

edited

Loading

varungandhi-apple commented Aug 29, 2020 •

edited

Loading

varungandhi-apple commented Aug 29, 2020 •

edited

Loading

varungandhi-apple commented Sep 2, 2020 •

edited

Loading

varungandhi-apple commented Sep 2, 2020 •

edited

Loading

varungandhi-apple commented Sep 3, 2020 •

edited

Loading