[stdlib] Make _stdlib_initializeReturnAutoreleased non-@inlinable #17909
Conversation
This is merely a helper for StdlibUnittest; there is no reason to leave it inlinable.
|
@swift-ci please test |
| // Use 'dynamic' to force Objective-C dispatch, which uses the | ||
| // return-autoreleased call sequence. | ||
| @objc | ||
| internal dynamic func returnsAutoreleased(_ x: AnyObject) -> AnyObject { | ||
| return x | ||
| } | ||
|
|
||
| // Use 'dynamic' to prevent this call to be duplicated into other modules. |
There was a problem hiding this comment.
This comment worries me a little. Are these pieces of code really equivalent?
There was a problem hiding this comment.
We lose a call through the Objective-C runtime, but the comment indicates this particular one was only there to defeat implicit inlinability in previous versions of the stdlib.
The important call is the one to returnsAutoreleased; that needs to go through objc_msgSend or else this turns into a messy noop. I verified that the important call still gets compiled to use message passing after this change.
There was a problem hiding this comment.
Update: This may still turn out to be a messy no-op; disabling the call to _stdlib_initializeReturnAutoreleased() in StdlibUnittest seemingly has no effect on any of the tests.
There was a problem hiding this comment.
This affects only the i386 slice, where because of the way the autorelease return value optimization was implemented, the first attempt would always fail due to the lazy PLT stub. The intent here is to make sure at least one return-autorelease occurs before the test suite so that this behavior doesn't cause behavior variation in tests. Make sure you test a 32-bit simulator.
There was a problem hiding this comment.
Wait, the #if arch(x86_64) makes this a no-op on the 32-bit simulators. Is that condition wrong?
(In #17907, I found that ARC on x86-64 does require a warmup like this -- except I had to make it call into Foundation.)
There was a problem hiding this comment.
Ah, maybe I misremembered and it's x86_64 that's affected (@gparker42 would know). It could be that these days something else triggers an autorelease-return early enough to happen before this one, maybe.
There was a problem hiding this comment.
My comment above was mistaken: re-running the tests with the warmup disabled actually led to some newly reported leaks. (Well, one leak, in Array.BridgedToObjC/Custom/BridgeBack/Cast, when the test was compiled without optimizations.) So the warmup is actually necessary, and the code does work as intended.
Testing with lldb confirms that _stdlib_initializeReturnAutoreleased does indeed perform the first call to objc_autoreleaseReturnValue, at least in the handful of tests I tried.
TL;DR: All is well. 🎉
This is merely a helper for StdlibUnittest; there is no reason to leave it inlinable.