This is an archive of the discontinued LLVM Phabricator instance.

[LLD] cleans up context and symbol table to allow multiple invocations to lld::elf::linker()
AbandonedPublic

Authored by MaskRay on Jan 25 2023, 7:56 AM.

Details

Summary

Fixes: https://github.com/llvm/llvm-project/issues/59874

Initially after the first invocation of the linker() the symbol table is not cleared and this causes issues with the second invocation when linking a new shared library due to a conflict in the presence of a previously existing _DYNAMIC symbol tripping a runtime assertion.
The linker already had a cleanup lambda registered in an error handler. This patch just declares that lambda up front, and calls it before registering in the error handler. This way the linker can be called multiple times. This is necessary when JIT-ing e.g. AMDGCN kernels.

Diff Detail

Event Timeline

bjoo created this revision.Jan 25 2023, 7:56 AM
Herald added a project: Restricted Project. · View Herald Transcript
bjoo requested review of this revision.Jan 25 2023, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2023, 7:56 AM
jhuber6 accepted this revision.Jan 25 2023, 9:27 AM

This looks reasonable to me. I'd like to hear if @MaskRay has any objections. Thanks for submitting a bug fix!

This revision is now accepted and ready to land.Jan 25 2023, 9:27 AM
bjoo added a comment.Jan 25 2023, 9:34 AM

Thanks @jhuber6 . My only remaining concern is that this is a bit of a blunt instrument. I am not sure for example whether the new symtap creation causes a leak ultimately. If more knowledgeable folks than I can suggest refinements I'd be happy to implement them.

MaskRay added 1 blocking reviewer(s): MaskRay.Jan 30 2023, 4:09 PM
This revision now requires review to proceed.Jan 30 2023, 4:09 PM
MaskRay requested changes to this revision.EditedJan 30 2023, 4:45 PM

(Back from a trip)
This is not a proper fix. After D108850 the user of lld::elf::link now needs to manually invoke CommonLinkerContext::destroy(); (see lld/tools/lld/lld.cpp).
I agree that requiring another function call is not that ergonomic but I think we need a better solution instead of landing this.

This revision now requires changes to proceed.Jan 30 2023, 4:45 PM

I am working on a better fix.

aganea added a subscriber: aganea.Jan 31 2023, 1:32 PM

As mentioned in https://github.com/llvm/llvm-project/issues/59874, this is on purpose. More info here: https://reviews.llvm.org/D142949#4093346. One should call safeLldMain() instead! Let me finish/land D119049 and things should look better afterwards.

MaskRay commandeered this revision.Aug 15 2023, 10:57 PM
MaskRay edited reviewers, added: bjoo; removed: MaskRay.

Obsoleted by D119049

This revision is now accepted and ready to land.Aug 15 2023, 10:57 PM
MaskRay abandoned this revision.Aug 15 2023, 10:58 PM