Skip to content
New issue

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

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

Already on GitHub? # to your account

ELF: Emit (most) instantiated symbols in COMDATs #4748

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

kinke
Copy link
Member

@kinke kinke commented Sep 7, 2024

No description provided.

@kinke kinke changed the title Fix issue #3589 ELF: Emit (most) instantiated symbols in COMDATs Sep 7, 2024
@kinke kinke marked this pull request as ready for review September 7, 2024 12:12
Comment on lines +263 to +268
const bool inCOMDAT = needsCOMDAT() ||
// ELF needs some help for ODR linkages:
// https://github.com/ldc-developers/ldc/issues/3589
(linkage == templateLinkage &&
global.params.targetTriple->isOSBinFormatELF());
return {linkage, inCOMDAT};

Choose a reason for hiding this comment

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

I don't think this works, I'll double confirm on weka source code, but we have been getting too much linker issues by applying COMDAT to most of the symbols. e.g.:

ld.lld: error: relocation refers to a discarded section: .text._D5mecca3lib10reflection__T2asVAyaa13_6e6f7468726f7720406e6f6763TDFNaNbNiNfZvZQBzFNcMQuZ9__lambda2MFNaNbNiNeZQBs
>>> defined in core/3rdparty/build/mecca/libmecca-notrace.a(.._.._mecca_src_mecca_lib_exception.d.o)
>>> section group signature: _D5mecca3lib10reflection__T2asVAyaa13_6e6f7468726f7720406e6f6763TDFNaNbNiNfZvZQBzFNcMQuZ9__lambda2MFNaNbNiNeZQBs
>>> prevailing definition is in core/3rdparty/build/mecca/libmecca-notrace.a(.._.._mecca_src_mecca_reactor_io_inotify.d.o)
>>> referenced by reflection.d:556 (../core/3rdparty/mecca/src/mecca/lib/reflection.d:556)
>>>               .._.._mecca_src_mecca_lib_exception.d.o:(_D5mecca3lib9exception6ExcBuf__T15constructHelperHTC4coreQBt11AssertErrorTAyaZQBuMFNbNiNeQpmbKQuZQBu) in archive core/3rdparty/build/mecca/libmecca-notrace.a

This verifies on symbols with InternalLinkage and only after upgrading the compiler (symbols with internal linkage are the ones that are relocations to discarded symbols). I haven't tested with the same LLVM version tho, could be an upstream regression.


Bottom line: weka@e1cca7a with --enable-wekamods=true yields this linker error, and, without the mods, data culling doesn't work (we start to get duplicate sections, as described in the issue). The --enable-wekamods=true works on older versions. So we are on this middle ground where none works unless we only mark specific linkage with comdat.

I'll try to create a test to demonstrate this.

Choose a reason for hiding this comment

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

Tested, and it works! I see, this works if we guarantee that templateLinkage is not ever InternalLinkage, which probably is just a derivative of _odr attributes? What about --fvisibility=hidden, this doesn't influence templateLinkage, right? If that's all true, I'm good with this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

templateLinkage is always an ODR linkage:

ldc/driver/main.cpp

Lines 470 to 472 in 580ff68

templateLinkage = global.params.linkonceTemplates == LinkonceTemplates::no
? LLGlobalValue::WeakODRLinkage
: LLGlobalValue::LinkOnceODRLinkage;

@kinke kinke merged commit 58280f5 into ldc-developers:master Sep 9, 2024
20 checks passed
@kinke kinke deleted the fix3589 branch September 9, 2024 09:08
JohanEngelen pushed a commit to weka/ldc that referenced this pull request Sep 10, 2024
ELF: Emit (most) instantiated symbols in COMDATs
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants