use GROUP for Elf code COMDATs #6564
Conversation
|
@klickverbot @ibuclaw please weigh in on this. |
|
This one is for code, #6560 is for data. They are separate pulls because of |
src/backend/elfobj.c
Outdated
| @@ -1687,8 +1687,8 @@ STATIC void setup_comdat(Symbol *s) | |||
| if (tyfunc(s->ty())) | |||
| { | |||
| //s->Sfl = FLcode; // was FLoncecode | |||
| //prefix = ".gnu.linkonce.t"; // doesn't work, despite documentation | |||
| prefix = ".text."; // undocumented, but works | |||
| prefix = ".gnu.linkonce.t"; // doesn't work, despite documentation | |||
There was a problem hiding this comment.
You might want to update the comment. ;)
|
@WalterBright: I never looked into .gnu.linkonce.* myself in any detail. I know that it crops up from inside glibc, but LLVM doesn't ever seem to emit it. All I can suggest is to try it out and then pay close attention to linker issues in the pre-release process. In particular, it would be wise to test on several versions of ld.bfd and gold (e.g. both on Ubuntu 14.04 and 16.10). When rolling out |
|
(I assume you are aware that COMDAT-like functionality is usually implemented using section groups on Linux/ELF?) |
|
Error is: Anyone know how to do COMDATs on Elf? Googling hasn't been helpful. |
|
Use a section group (SHT_GROUP) with the discard flag set (GRP_COMDAT). |
|
@klickverbot can you please send me a .o file with a GRP_COMDAT in it? I haven't been able to generate one. |
|
This archive contains a simple example of a C++ template function as compiled by recent Clang on Linux. |
|
(Perhaps I misunderstand what you are asking for? I haven't looked at DMD's codegen in a while.) |
|
Please send me the .o file itself, not a dump of it. Thanks! (Or the C++ source file. I was having trouble getting clang++ to emit these things.) |
|
The last post contains a link. |
|
Thank you. Will investigate. |
|
Ah, I found out where I was going wrong. My elf dumper wasn't printing the GROUP bit. |
|
So what's the verdict here? Does this help or not? |
|
FWIW I just tried this branch to build phobos unittests and got: |
|
More info on COMDAT/section groups Airs – Ian Lance Taylor » Linkers part 15 |
|
Typical example of where sections groups are needed, functions in text and EH tables in relro data. |
WalterBright
force-pushed
the
code-linkonce
branch
2 times, most recently
from
79562b7 to
10987c2
Compare
WalterBright
changed the title
|
Ok, I completely revised it to use GROUPs. It's only for code at the moment, I'll do it for data if this succeeds. |
WalterBright
force-pushed
the
code-linkonce
branch
4 times, most recently
from
3bf65cb to
27e2a9e
Compare
|
@WalterBright what is the differential size of |
src/backend/elfobj.c
Outdated
| */ | ||
| const char *p = cpp_mangle(s); | ||
| IDXSTR namidx = Obj::addstr(symtab_strings, p); | ||
| IDXSYM info = elf_addsym(namidx, 0, 0, STT_FUNC, STB_WEAK, comdatidx); |
There was a problem hiding this comment.
Just spent more than an hour on this, but I'm skeptical whether it could work out to squeeze the comdat group into setup_comdat.
It's already messy to prepone the symbol definition here (and still repeat it in Obj::comdat/func_start or wherever)
Even after making that already entangled code work somehow, it does not put other data that belongs to that function into the comdat group.
Eventually I ended up with a linker error complaining about a sth. in .rodata referencing a function in a discarded section.
`.text._D3std6format18__T10FormatSpecTaZ10FormatSpec6fillUpMFNaNfZv' referenced in section `.rodata' of generated/linux/debug/64/unittest/std/path.o: defined in discarded section `.text._D3std6format18__T10FormatSpecTaZ10FormatSpec6fillUpMFNaNfZv[_D3std6format18__T10FormatSpecTaZ10FormatSpec6fillUpMFNaNfZv]' of generated/linux/debug/64/unittest/std/path.o
Maybe that is a bug in my changes. But OTOH we'd also want to add EH tables into a function's COMDAT group.
How about we open that group (similar to func_start) and add the function as signature symbol after everything was emitted.
Not sure whether related symbols are all emitted with the function though.
There was a problem hiding this comment.
I'm curious why you're getting that error. I'm getting a different one, from both the autotester and my setup at home, where cppa in the test suite is crashing. I spent several hours on that last night and am still not able to determine the cause. It's maddening.
There was a problem hiding this comment.
Finally tracked it down. Agony! Added explanation in the comments.
WalterBright
force-pushed
the
code-linkonce
branch
from
27e2a9e to
60fd17f
Compare
As said, I eventually ended up w/ that error after making the comdat groups work w/ a few hacks. The auto-tester has been showing the same errors all the time already. Essentially the linker complains that it has data/code relocations in an object file that point to code sections in the same object file that are being discarded (due to duplicates in other object files). |
WalterBright
force-pushed
the
code-linkonce
branch
9 times, most recently
from
7239743 to
d0d3d02
Compare
|
@MartinNowak Things are working now, except for failing with a timeout on FreeBSD, 32 and 64 bit. Can you please check if it is hanging or not? I don't have a FreeBSD machine here - and FreeBSD should behave identically with Linux as far as the Elf object file format goes. |
|
ping @MartinNowak |
WalterBright
force-pushed
the
code-linkonce
branch
from
d0d3d02 to
0ce7343
Compare
|
@WalterBright FreeBSD_32 actually fails with a linker error: Some related bug reports: |
|
It was timing out before, now it is giving an error report. Hmm. Looks like I may have to build myself a FreeBSD box to figure this out. |
WalterBright
force-pushed
the
code-linkonce
branch
from
0ce7343 to
a48acfe
Compare
|
I've disabled this code for FreeBSD for the time being. It'll have to wait until I build a FreeBSD machine so I can figure out what is going wrong there, in the meantime, we can gain experience with this on Linux. |
|
@MartinNowak can we pull this into the beta? |
|
I built a new computer and installed FreeBSD 11.0 on it (the latest). It does not fail with the linker internal error. The installed whereas the one in the autotester is: Can we upgrade the linker in the autotester? Do we care about working with the old linker? |
Should've targetted stable and use maybe milestones to land in the beta. Don't think last minute additions before a release are a good idea either. |
I don't either. This change is a bit risky, and needs a longer testing period. |
|
This PR has caused a regression. Very annoying that the linker errors were ignored when merging this! |
|
|
||
| /* Get section index of the comdat | ||
| */ | ||
| IDXSEC comdatidx = section_cnt + 1; |
There was a problem hiding this comment.
Bug Issue 17338 – [Reg 2.075] link failure unsupported symbol section 0xff01 happens because you're guessing the section index here, but forgot to account for extended section indices (SHT_SYMTAB_SHNDX) once section_cnt reaches SHN_LORESERVE.
I'd opt for using the newly added addSectionToComdat for the comdat itself, after it was actually written out to it's section (and has a correct section number).
|
This PR causes another regression. |
I don't know what the "doesn't work" means anymore, so turn this on to flush out any problems, because the ".text." results in concatenation, not pick one.