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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
208546b
Add test case for issue #3589
kinke Sep 7, 2024
43f59b1
ELF: Emit (most) instantiated symbols in COMDATs
kinke Sep 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#### Bug fixes
- Fix potentially corrupt IR layouts for bit fields. (#4646, #4708)
- Fix potentially corrupt IR layouts for explicitly under-aligned aggregates, a regression introduced in LDC v1.31. (#4734, #4736)
- ELF: Emit (most) instantiated symbols in COMDATs for proper link-time culling. (#3589, #4748)

# LDC 1.39.0 (2024-07-04)

Expand Down
8 changes: 7 additions & 1 deletion gen/tollvm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,13 @@ LLGlobalValue::LinkageTypes DtoLinkageOnly(Dsymbol *sym) {
}

LinkageWithCOMDAT DtoLinkage(Dsymbol *sym) {
return {DtoLinkageOnly(sym), needsCOMDAT()};
const auto linkage = DtoLinkageOnly(sym);
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};
Comment on lines +263 to +268
Copy link

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.

Copy link

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;

}

bool needsCOMDAT() {
Expand Down
22 changes: 22 additions & 0 deletions tests/codegen/gh3589.d
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// UNSUPPORTED: Windows || Darwin

// emit duplicate instantiated globals into two object files:
// RUN: %ldc -c %S/inputs/gh3589_a.d -I%S/inputs -of=%t_a.o
// RUN: %ldc -c %S/inputs/gh3589_b.d -I%S/inputs -of=%t_b.o

// link & run:
// RUN: %ldc -I%S/inputs %t_a.o %t_b.o -run %s

extern extern(C) __gshared {
// magic linker symbols to refer to the start and end of test_section
byte __start_test_section;
byte __stop_test_section;
}

void main() {
import gh3589_a, gh3589_b;
assert(a_info == b_info);

const sectionSize = cast(size_t) (&__stop_test_section - &__start_test_section);
assert(sectionSize == 4);
}
2 changes: 2 additions & 0 deletions tests/codegen/inputs/gh3589_a.d
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import gh3589_templ;
shared a_info = &getInfo!0;
2 changes: 2 additions & 0 deletions tests/codegen/inputs/gh3589_b.d
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import gh3589_templ;
shared b_info = &getInfo!0;
4 changes: 4 additions & 0 deletions tests/codegen/inputs/gh3589_templ.di
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import ldc.attributes;
template getInfo(int I) {
@(section("test_section")) @assumeUsed shared int getInfo = I;
}