Skip to content

[IRGen] Distributed: Destroy loaded arguments after the call #65874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
May 16, 2023
Merged

[IRGen] Distributed: Destroy loaded arguments after the call #65874

merged 2 commits into from
May 16, 2023

Conversation

xedin
Copy link
Contributor

@xedin xedin commented May 11, 2023

Arguments that have been loaded due to underlying method conversion
have to be destroyed after the call otherwise they are going to leak.

Resolves: #65853
Resolves: rdar://109207043

@xedin xedin requested a review from ktoso as a code owner May 11, 2023 21:51
@xedin
Copy link
Contributor Author

xedin commented May 11, 2023

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented May 11, 2023

@ktoso I added IRGen test because I couldn't figure out why deinit is not printed in lit environment.

@xedin
Copy link
Contributor Author

xedin commented May 12, 2023 •
edited
Loading

Looks like Linux doesn't like parameters with archetypes, although it works just fine on macOS and simulators, here is the assert it's crashing with:

swift-frontend: /home/build-user/swift/lib/IRGen/GenArchetype.cpp:77: swift::irgen::MetadataResponse swift::irgen::emitArchetypeTypeMetadataRef(swift::irgen::IRGenFunction &, swift::CanArchetypeType, swift::irgen::DynamicMetadataRequest): Assertion `archetype->getParent() && "Not a nested archetype"' failed.
 #8 0x0000000000b4de73 swift::irgen::emitArchetypeTypeMetadataRef(swift::irgen::IRGenFunction&, swift::CanTypeWrapper<swift::ArchetypeType>, swift::irgen::DynamicMetadataRequest) (/home/build-user/build/buildbot_linux/swift-linux-x86_64/bin/swift-frontend+0xb4de73)
1.	Swift version 5.9-dev (LLVM e2c8b75d8731442, Swift b6735999ba0d722)
2.	Compiling with effective version 4.1.50
3.	While evaluating request IRGenRequest(IR Generation for file "/home/build-user/swift/test/Distributed/Runtime/distributed_actor_func_calls_remoteCall_genericFunc.swift")
4.	While emitting IR SIL function "@$s4main7GreeterC8generic26strict__SSSd_xSayq_GtYaKSeRzSERzSeR_SER_r0_lFTE".
 for 'generic2(strict:_:_:)' (in module 'main')
 #9 0x0000000000b0d11f swift::CanTypeVisitor<(anonymous namespace)::EmitTypeMetadataRef, swift::irgen::MetadataResponse, swift::irgen::DynamicMetadataRequest>::visit(swift::CanType, swift::irgen::DynamicMetadataRequest) MetadataRequest.cpp:0:0
#10 0x0000000000b0954a swift::irgen::IRGenFunction::emitTypeMetadataRef(swift::CanType, swift::irgen::DynamicMetadataRequest) (/home/build-user/build/buildbot_linux/swift-linux-x86_64/bin/swift-frontend+0xb0954a)
#11 0x0000000000c8e7ae swift::irgen::emitGenericRequirementFromSubstitutions(swift::irgen::IRGenFunction&, swift::GenericRequirement, swift::MetadataState, swift::SubstitutionMap, bool) (/home/build-user/build/buildbot_linux/swift-linux-x86_64/bin/swift-frontend+0xc8e7ae)
#12 0x0000000000b1222d swift::irgen::GenericArguments::collect(swift::irgen::IRGenFunction&, swift::SubstitutionMap) crtstuff.c:0:0
#13 0x0000000000b119f6 emitNominalMetadataRef(swift::irgen::IRGenFunction&, swift::NominalTypeDecl*, swift::CanType, swift::irgen::DynamicMetadataRequest) MetadataRequest.cpp:0:0
#14 0x0000000000b0d289 swift::CanTypeVisitor<(anonymous namespace)::EmitTypeMetadataRef, swift::irgen::MetadataResponse, swift::irgen::DynamicMetadataRequest>::visit(swift::CanType, swift::irgen::DynamicMetadataRequest) MetadataRequest.cpp:0:0
#15 0x0000000000b0954a swift::irgen::IRGenFunction::emitTypeMetadataRef(swift::CanType, swift::irgen::DynamicMetadataRequest) (/home/build-user/build/buildbot_linux/swift-linux-x86_64/bin/swift-frontend+0xb0954a)
#16 0x0000000000b0a074 swift::irgen::IRGenFunction::emitTypeMetadataRefForLayout(swift::SILType, swift::irgen::DynamicMetadataRequest) (/home/build-user/build/buildbot_linux/swift-linux-x86_64/bin/swift-frontend+0xb0a074)
#17 0x0000000000c51446 swift::irgen::IRGenFunction::emitValueWitnessTableRef(swift::SILType, swift::irgen::DynamicMetadataRequest, llvm::Value**) (/home/build-user/build/buildbot_linux/swift-linux-x86_64/bin/swift-frontend+0xc51446)
#18 0x0000000000c51370 swift::irgen::IRGenFunction::emitValueWitnessTableRef(swift::SILType, llvm::Value**) (/home/build-user/build/buildbot_linux/swift-linux-x86_64/bin/swift-frontend+0xc51370)
#19 0x0000000000c70ffa swift::irgen::IRGenFunction::emitValueWitnessFunctionRef(swift::SILType, llvm::Value*&, swift::irgen::ValueWitness) (/home/build-user/build/buildbot_linux/swift-linux-x86_64/bin/swift-frontend+0xc70ffa)
#20 0x0000000000c7446a swift::irgen::emitDestroyCall(swift::irgen::IRGenFunction&, swift::SILType, swift::irgen::Address) (/home/build-user/build/buildbot_linux/swift-linux-x86_64/bin/swift-frontend+0xc7446a)
#21 0x0000000000d3f99a swift::irgen::TypeInfo::callOutlinedDestroy(swift::irgen::IRGenFunction&, swift::irgen::Address, swift::SILType) const (/home/build-user/build/buildbot_linux/swift-linux-x86_64/bin/swift-frontend+0xd3f99a)
#22 0x0000000000cb7f0d swift::irgen::RecordTypeInfoImpl<(anonymous namespace)::LoadableStructTypeInfo, swift::irgen::LoadableTypeInfo, (anonymous namespace)::StructFieldInfo, true>::destroy(swift::irgen::IRGenFunction&, swift::irgen::Address, swift::SILType, bool) const GenStruct.cpp:0:0
#23 0x0000000000d5c5f2 (anonymous namespace)::DistributedAccessor::emitReturn(llvm::Value*) GenDistributed.cpp:0:0
#24 0x0000000000d5aedf (anonymous namespace)::DistributedAccessor::emit() GenDistributed.cpp:0:0

@ktoso ktoso added the distributed Feature → concurrency: distributed actor label May 12, 2023
ktoso
ktoso reviewed May 12, 2023
View reviewed changes
Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks -- I'll see if I can figure out what linux doesn't like there

@xedin
Copy link
Contributor Author

xedin commented May 12, 2023

@ktoso I can XFAIL these tests on Linux and land this if you'd like?

@ktoso
Copy link
Contributor

ktoso commented May 15, 2023 •
edited
Loading

Maybe that's a good idea to ensure we're on time in apple platform releases and continue fixing on Linux.

I'll do that On second thought, let's try to figure it out. I'll see if someone has some insights to share

@xedin
[IRGen] Distributed: Destroy loaded arguments after the call
8569c9d 
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
@xedin xedin force-pushed the rdar-109207043 branch from 415efac to 24bb013 Compare May 15, 2023 21:46
@xedin
Copy link
Contributor Author

xedin commented May 15, 2023

@swift-ci please test Linux platform

@xedin xedin force-pushed the rdar-109207043 branch from 24bb013 to 89ba257 Compare May 16, 2023 00:31
@xedin
Copy link
Contributor Author

xedin commented May 16, 2023

@swift-ci please test

@xedin xedin merged commit cb51359 into swiftlang:main May 16, 2023
@ktoso
Copy link
Contributor

ktoso commented May 16, 2023 •
edited
Loading

Thanks a lot LGTM!

@ktoso ktoso mentioned this pull request May 16, 2023
Merged
@ktoso
Copy link
Contributor

ktoso commented May 16, 2023

I prepared a cherry pick over here #65941

@ktoso ktoso mentioned this pull request May 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktoso ktoso ktoso left review comments

Assignees
No one assigned
Labels
distributed Feature → concurrency: distributed actor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remote target (for distributed actor) arguments are not properly deinitialized
2 participants
@xedin @ktoso