[IRGen] Fix ObjC method signatures for .cxx_construct and .cxx_destruct #17884
Conversation
The usual strategy is to get things into master and cherry-pick them over. I believe GitHub will let you change your target branch without opening another PR.
We can nominate this for you, just please cherry-pick. |
test/IRGen/objc_methods.swift
Outdated
| @objc func takesString(_ x: String) -> String { return x } | ||
| @objc func takesArray(_ x: [AnyObject]) -> [AnyObject] { return x } | ||
| @objc func takesDict(_ x: [NSObject: AnyObject]) -> [NSObject: AnyObject] { return x } | ||
| @objc func takesSet(_ x: Set<NSObject>) -> Set<NSObject> { return x } | ||
|
|
||
There was a problem hiding this comment.
Nitpick: can you avoid changing whitespace elsewhere in the file? (We also generally try to clear out trailing whitespace even in indented blocks, but even without that it's better if the diff is as small as possible.)
There was a problem hiding this comment.
Yes, I can change that. I originally had my editor set to clear trailing whitespace, but then found that lib/IRGen/GenObjC.cpp has trailing whitespace and reversed course.
test/IRGen/objc_methods.swift
Outdated
| } | ||
| } | ||
|
|
||
| // CHECK: [[IMPLICIT_PARAMS_SIGNATURE:@.*]] = private unnamed_addr constant [8 x i8] c"v16@0:8\00" |
There was a problem hiding this comment.
Nitpick: this should probably be named something like DESTRUCT_SIGNATURE.
There was a problem hiding this comment.
I gave it a non-descriptive name because that signature is shared by two tested methods (-[ObjcDestructible .cxx_destruct] and -[Foo baz]) that don't really have anything in common. Do you have a name in mind that suits both?
There was a problem hiding this comment.
Ahh, I get it. Maybe NO_ARGS_SIGNATURE, then?
|
Also, thanks for the note that the branch policy isn't mentioned on the "Contributing Code" page. We should tweak that. |
"@?" is not a method signature. An acceptable format is "vM@0:N", where N is the pointer size and M is twice the pointer size. The numeric values don't matter anymore on Apple platforms because the runtime does not rely on them (they used to be stack offsets from the base pointer), but given that the first (self) and second (_cmd) parameters are pointers, the expected value is easily obtained and it is probably good style to include it. Fixes https://bugs.swift.org/browse/SR-8203
fay59
force-pushed
the
cxx-destruct-signature
branch
from
4b870ab to
917ad05
Compare
fay59
changed the title
|
The PR base has been updated. I might need some more hand-holding: should I be the one to kick off automated tests? |
Because you are a first-time contributor, you will not be able to invoke CI. See Commit Access. @swift-ci please smoke test |
|
(And to the outside observer, neither can I!) |
|
Hmph, looks like swift-ci might not yet support branch-switching - these failures look like it's still trying to build 4.2. ping: @shahmishal In the mean time please leave this open and open a separate pull request against master. |
I'm starting the PR against 4.2, but I actually have no idea what I'm doing. I suspect that you have to get someone's blessing to do this, but it didn't seem to be explicitly mentioned in the contribution guidelines. Let me know if this should be rebased on another branch.
Explanation
For aeons past, the Swift compiler has emitted incorrect method signatures for
.cxx_constructand.cxx_destructmethods. The signature was hard-coded to"@?", which is the Objective-C type signature for a block. Type signatures and method signatures are different, and this is not a valid method signature.Scope
Only Swift classes that inherit from
NSObjectget.cxx_{construct,destruct}methods. It wouldn't be possible to call a Swift.cxx_constructor.cxx_destructwithNSInvocation, although I'm not entirely sure at which point that would fail. This bug is essentially annoying only to people who look at binaries.Issue
https://bugs.swift.org/browse/SR-8203
Risk
Negligible. We know from experience that you can put garbage in the method signature of
.cxx_destructand it'll go unnoticed for years. In the unlikely, worst-case scenario in which I got it wrong, the garbage will have changed and someone else will step up in 2023 to fix it without anyone else ever noticing that there was a problem.Testing
Added a test in test/IRGen/objc_methods.swift.
Reviewed By
You?