🍒[5.9][IRGen] Distributed: Destroy loaded arguments after the call #65941
Conversation
ktoso
changed the title
ktoso
added
distributed
|
@swift-ci please test |
|
Weird failure, I'll investigate
and I'll destroy them in reverse order as we create |
lib/IRGen/GenDistributed.cpp
Outdated
| @@ -575,6 +581,13 @@ void DistributedAccessor::emitLoadOfWitnessTables(llvm::Value *witnessTables, | |||
| } | |||
|
|
|||
| void DistributedAccessor::emitReturn(llvm::Value *errorValue) { | |||
| // Destroy loaded arguments. | |||
| // This MUST be done before deallocating, as otherwise we'd try to swift_release freed memory, | |||
There was a problem hiding this comment.
the latest change is this; we MUST destroy BEFORE we dealloc (ofc, duh, ufff)
|
@swift-ci please test |
|
Fixed formatting |
|
@swift-ci please test |
ktoso
force-pushed
the
pick-rdar-109207043
branch
from
1593dee to
585cde3
Compare
|
@swift-ci please test |
Arguments that have been loaded due to underlying method conversion have to be destroyed after the call otherwise they are going to leak. Resolves: swiftlang#65853 Resolves: rdar://109207043
ktoso
force-pushed
the
pick-rdar-109207043
branch
from
585cde3 to
2f34f20
Compare
|
@swift-ci please test |
|
What; I'm checking this exact same branch locally and the test passes... Looking into it hmm |
|
Could have been the missing empty directory directive hmmm |
|
@swift-ci please test |
|
Interesting, that was it so I'll bring that |
Description: Decoding of remote call arguments is synthesized in IR, and the lifetime of the arguments that have been loaded due to underlying method conversion have to be destroyed after the call otherwise they are going to leak. This PR fixes this lifetime issue resolving the memory leak of arguments passed to remote call targets.
EDIT: Added the follow up from #65952 immediately here; which fixes the order in which we destroy and dealloc: we MUST deallocate AFTER we destroy since otherwise the destroy does not make the refcount drop to zero, and we were still missing deinitializers running. The patch now contains complete fix.
Risk: Low, isolated to distributed remote calls; confirmed to work on all platforms
Review by: @jckarter
Testing: CI testing
Original PR: #65874
Radar: rdar://109207043
Resolves: #65853