[DebugInfo] Ignore noescape bit for all @convention(c) pointers

[asl]

Currently @escaping and @NoEscape @convention(c) functions are mangled in the same way: the escaping flag is silently ignored. This cases all kinds of problems and type mismatches when type is reconstructed by a demangler as flag is lost.

Ignore noescape bit in the ctors for AnyFunctionType / SILFunctionType

Resolves SR-15871

[compnerd]

I think that the change looks good; would you mind adding some test cases to the unittests for the demangler/remangler please?

[asl]

@compnerd Sure, added!

[jckarter]

A @convention(c) function pointer has no context. It shouldn't matter whether it's "escaping" or not. Is there somewhere we can canonicalize that distinction away? Adding type manglings comes with a whole bunch of backward compatibility headache for dealing with older OS runtimes that don't support the new mangling.

[asl]

@jckarter This was exactly my thought (see SR). However:

• Generic parameters could be only @escaping, so we cannot canonicalize early and we cannot drop @escaping (see e.g. test/decl/protocol/special/Sendable.swift)
• Optimizations are promoting @escaping to @noescape, so looks like we'd need lots of special cases

Note that the mangling is applied only for the debug info generation, not for the real symbols (similar thing is done for escaping / non-escaping @convention(block) pointers).

[jckarter]

All @convention(c) values are escaping; you should never have to write @escaping @convention(c), and if you do, the @escaping should be ignored. So as a generic parameter, that's fine. Maybe we can make it so that the constructors for AnyFunctionType and SILFunctionType's ExtInfo force the escaping bit on when the C or Thin representation is used? That would avoid the need to special case those representations in optimizations and such, since attempts to construct a nonescaping function type would naturally create the same type as the escaping type.

[asl]

@jckarter I feel like trying to open a small can of worms :) Looks like the representation field is a bit abused (and this is why AST function representation and SIL function representation enums overlay – see FIXME there) and therefore non-supported representations could be passed from SIL level down to AnyFunctionType. As a result I had to remove assertion, otherwise we cannot extract a representation in the ctor.

Please review

[jckarter]

This looks good, thanks!

[asl]

@jckarter We cannot do this in ctor apparently :)

The reason is quite severe: types are unique. If we'd silently drop noescape bit in the ctor we will create new function type which might be identical to the existing one.

So, we'd need to do this in ::get, not in ctor.

[jckarter]

Ah, yeah, that makes sense!

[jckarter]

@swift-ci Please test

[jckarter]

@swift-ci Please test source compatibility

[asl]

@jckarter Given that all @convention(c) are escaping now, this conversion from compiler_crashers_2_fixed/sr12723.swift seems to be prohibited now:

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

Are we ok with this? This certainly a source code compatibility break...

[jckarter]

@asl That's strange, passing an escaping function as nonescaping should be allowed. Maybe it's confused by the combination of representation and escapingness change?

[asl]

@jckarter I misread the logs. We're having extra diagnostics now, e.g.:

swift/validation-test/compiler_crashers_2_fixed/sr12723.swift:65:42: error: converting non-escaping value to '@convention(thin) () -> Void' may allow it to escape
    let _ :  (() -> Void) -> Void = thin.function
                                         ^
swift/validation-test/compiler_crashers_2_fixed/sr12723.swift:65:42: error: cannot convert value of type '(@escaping @convention(thin) () -> Void) -> Void' to specified type '(() -> Void) -> Void'
    let _ :  (() -> Void) -> Void = thin.function

[asl]

@jckarter @BradLarson will you please trigger CI for me?

[jckarter]

@swift-ci Please test

[jckarter]

@swift-ci Please test macOS

[jckarter]

That failure on macOS looks unrelated, let me try it again.

[jckarter]

Hm, I don't see why test/IRGen/objc_retainAutoreleasedReturnValue.swift's output would be affected by these changes, but the failure did reproduce in two builds, and that test doesn't appear to be failing on other PRs. @asl Are you able to reproduce the test failure locally on your own machine?

[asl]

@jckarter I cannot reproduce on arm64-darwin. Just did another rebase of the branch on top of main – and still nothing...

[jckarter]

@asl The test only applies for x86_64 macOS, can you test in that configuration?

[asl]

@jckarter Looks like not easily as utils/build_script ignores --swift-darwin-supported-archs and always builds for arm64... Need to figure out what is broken there.

[jckarter]

If it built the x86_64 slice of the runtime and standard library, it'll hopefully work to go into the build dir and do /path/to/llvm/build/bin/llvm-lit -sv test-macosx-x86_64/IRGen/objc_retainAutoreleasedReturnValue.swift to run the test targeting x86_64. Otherwise, brute force way to do it would be to build the whole thing as if it was on Intel with arch -x86_64 utils/build-script or something like that.

[asl]

Looks like utils/build-script silently filters out everything non-arm64. Anyway, I managed to build necessary stuff by hand and after some plumbing, trying to reproduce & compare with main

[asl]

Interesting. In main we're having:

tail call swiftcc void @"$s34objc_retainAutoreleasedReturnValue10useClosureyySo12NSDictionaryC_yADXEtF"(%TSo12NSDictionaryC* %0, i8* bitcast (void (%TSo12NSDictionaryC*)* @"$s34objc_retainAutoreleasedReturnValue4testyySo12NSDictionaryCFyADXEfU_" to i8*), %swift.opaque* null)

while with this PR:

tail call swiftcc void @"$s34objc_retainAutoreleasedReturnValue10useClosureyySo12NSDictionaryC_yADXEtF09$s34objc_bcd16Value4testyySo12H10CFyADXEfU_Tf1nc_n"(%TSo12NSDictionaryC* %0)

Note that functions in question are @convention(thin). SIL in main looks like:

  %2 = function_ref @$s34objc_retainAutoreleasedReturnValue4testyySo12NSDictionaryCFyADXEfU_ : $@convention(thin) (@guaranteed NSDictionary) -> () // user: %3
  %3 = convert_function %2 : $@convention(thin) (@guaranteed NSDictionary) -> () to $@convention(thin) @noescape (@guaranteed NSDictionary) -> () // user: %4
  %4 = thin_to_thick_function %3 : $@convention(thin) @noescape (@guaranteed NSDictionary) -> () to $@noescape @callee_guaranteed (@guaranteed NSDictionary) -> () // user: %6
  // function_ref useClosure(_:_:)
  %5 = function_ref @$s34objc_retainAutoreleasedReturnValue10useClosureyySo12NSDictionaryC_yADXEtF : $@convention(thin) (@guaranteed NSDictionary, @noescape @callee_guaranteed (@guaranteed NSDictionary) -> ()) -> (

And in branch we do not have

  %3 = convert_function %2 : $@convention(thin) (@guaranteed NSDictionary) -> () to $@convention(thin) @noescape (@guaranteed NSDictionary) -> () // user: %4

I'm attaching LLVM IR just in case. It looks like escape => noescape conversion inhibited some specializations

main.ll.txt
pr.ll.txt

[jckarter]

Ah. I think the mangled name change should be harmless, then, so you can just change the test expectation. Thanks!

[asl]

Yes... Though I'm a bit worried in general that this exposes some additional transformations :)

[asl]

@jckarter will you please ping the CI?

[jckarter]

@swift-ci Please test

[jckarter]

@asl I agree that exposing the optimizer to new patterns is a concern, but it doesn't seem harmful here. And I've been generally trying to eliminate unnecessary function representation changes from generated SIL, so it's nice that this change helps there. Thanks for fixing this!

[compnerd]

I think that this regressed the Windows build (seems that the CI didnt trigger?) CC: @shahmishal

[compnerd]

https://ci-external.swift.org/job/oss-swift-windows-toolchain-x86_64-vs2019/528/