Page MenuHomePhabricator

[mingw] Fix GCC ABI compatibility for comdat things
ClosedPublic

Authored by rnk on Jun 20 2018, 3:51 PM.

Details

Summary

GCC and the binutils COFF linker do comdats differently from MSVC.
If we want to be ABI compatible, we have to do what they do, which is to
emit unique section names like ".text$_Z3foov" instead of short section
names like ".text". Otherwise, the binutils linker gets confused and
reports multiple definition errors when two object files from GCC and
Clang containing the same inline function are linked together.

The best description of the issue is probably at
https://github.com/Alexpux/MINGW-packages/issues/1677, we don't seem to
have a good one in our tracker.

I fixed up the .pdata and .xdata sections needed everywhere other than
32-bit x86. GCC doesn't use associative comdats for those, it appears to
rely on the section name.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Jun 20 2018, 3:51 PM
rnk updated this revision to Diff 152196.Jun 20 2018, 4:08 PM
  • add missing test
rnk added a reviewer: pirama.Jun 20 2018, 4:09 PM
mstorsjo accepted this revision.Jun 21 2018, 1:46 AM

Seems sensible to me. I've tested this change with LLD as linker as well, and it worked just fine for building Qt, and for a small test setup similar to the example in the issue report.

This revision is now accepted and ready to land.Jun 21 2018, 1:46 AM
rnk added a comment.Jun 21 2018, 10:55 AM

Seems sensible to me. I've tested this change with LLD as linker as well, and it worked just fine for building Qt, and for a small test setup similar to the example in the issue report.

Hm, I would expect LLD to do the wrong thing for .pdata and .xdata under this scheme. Consider this example where I've made an inline function that has some .pdata:

$ cat t.h
void force_pdata();
inline int foo(int a, int b) {
  force_pdata();
  return a + b;
}

$ cat a.cpp
#include "t.h"
void force_pdata() {}
int bar(void);
int __declspec(dllexport) asdf() { return foo(1, 2) + bar(); }

$ cat b.cpp
#include "t.h"
int bar() { return foo(3, 4); }

$ gcc -c a.cpp && gcc -c b.cpp && lld-link a.o b.o -dll -out:t.dll  -noentry

$ dumpbin -disasm -pdata t.dll
... Disassembly shows four function prologues at:
0000000180001000:
0000000180001002:
0000000180001030:
0000000180001050:

Function Table (3)

           Begin    End      Info      Function Name

  00000000 00001000 00001002 000020C8
  0000000C 00001002 0000102F 000020CC
  00000018 00001030 0000104D 000020D8

It looks like LLD is tossing the otherwise unreferenced .pdata and .xdata for the inline function that starts at RVA 0x001050.

If we want LLD to support linking mingw object files from GCC, we'll need to do some extra work in LLD.

In D48402#1139600, @rnk wrote:

Seems sensible to me. I've tested this change with LLD as linker as well, and it worked just fine for building Qt, and for a small test setup similar to the example in the issue report.

Hm, I would expect LLD to do the wrong thing for .pdata and .xdata under this scheme.

Oh - I didn't actually test anything that uses that; I'm only using sjlj or dwarf for exception handling ATM, since libcxxabi doesn't support SEH exeptions yet.

It looks like LLD is tossing the otherwise unreferenced .pdata and .xdata for the inline function that starts at RVA 0x001050.

If we want LLD to support linking mingw object files from GCC, we'll need to do some extra work in LLD.

Even if they're supposed to be interoperable, more or less, it's not completely compatible in all ways.

Currently, there's three different MinGW scenarios wrt compilation+linking that actually work:

  1. Compiling with GCC, linking with ld.bfd (a completely traditional MinGW environment, normally together with libgcc and libstdc++)
  2. Compiling with clang, linking with ld.bfd (just plugging in clang as compiler in an otherwise traditional MinGW environment, still using libgcc+libstdc++)
  3. Compiling with clang, linking with LLD (using compiler-rt and libcxx)

The third setup is what I and Martell have been working on; the produced object files should otherwise be identical to the ones in case 2 (modulo any use of -fdwarf-exceptions). The fourth plausible combo, compiling with GCC but linking with LLD, most probably doesn't work right now - I haven't tried but would expect there to be lots of details missing in LLD.

If we make clang start producing this, how hard would it be to make LLD handle it gracefully? (I don't think I've touched the comdat handling in LLD significantly.)

rnk added a comment.Jun 21 2018, 1:21 PM

If we make clang start producing this, how hard would it be to make LLD handle it gracefully? (I don't think I've touched the comdat handling in LLD significantly.)

Probably not too hard. We'd make a special case for comdat .pdata and .xdata (and maybe .ctors and .dtors) with $ in the input section name. We'd take the symbol name after the $, look that symbol up, and pretend that this section was comdat associative with it.

This revision was automatically updated to reflect the committed changes.

Reid, thanks a lot for addressing this issue.

(For reference to @mstorsjo, I am trying option #2 that you mention).

I fixed up the .pdata and .xdata sections needed everywhere other than
32-bit x86. GCC doesn't use associative comdats for those, it appears to
rely on the section name.

With your change, the duplicate definition errors for x86_64 are addressed but still happen for 32-bit x86.

I also found that for x86_64, the section also has a comdat tag.

Sections:
....
  4 .text$_Z3fooi 00000014  0000000000000000  0000000000000000  000001bc  2**4
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE, LINK_ONCE_DISCARD (COMDAT _Z3fooi 10)

while for i686, it doesn't:

Sections:
....
  3 .text$___Z3fooi 00000013  00000000  00000000  0000012c  2**4
                 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE, LINK_ONCE_DISCARD

Could that explain the difference?

rnk added a comment.Jun 21 2018, 4:04 PM

Sections:
....

3 .text$___Z3fooi 00000013  00000000  00000000  0000012c  2**4
               CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE, LINK_ONCE_DISCARD
Could that explain the difference?

Woops, one too many calls to getNameWithPrefix.

In D48402#1140046, @rnk wrote:

Sections:
....

3 .text$___Z3fooi 00000013  00000000  00000000  0000012c  2**4
               CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE, LINK_ONCE_DISCARD
Could that explain the difference?

Woops, one too many calls to getNameWithPrefix.

I replaced getMangler().getNameWithPrefix(SecName, Symbol, DL); in appendComdatSymbolForMinGW with SecName.append(Symbol.begin(), Symbol.end());. This removes the extra underscore but still doesn't associate a comdat with the section. Is there anything else that I can try?

In D48402#1140046, @rnk wrote:

Sections:
....

3 .text$___Z3fooi 00000013  00000000  00000000  0000012c  2**4
               CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE, LINK_ONCE_DISCARD
Could that explain the difference?

Woops, one too many calls to getNameWithPrefix.

I replaced getMangler().getNameWithPrefix(SecName, Symbol, DL); in appendComdatSymbolForMinGW with SecName.append(Symbol.begin(), Symbol.end());. This removes the extra underscore but still doesn't associate a comdat with the section. Is there anything else that I can try?

Reid fixed the underscore issue in rL335304, so you may wanna give that a shot.

rnk added a comment.Oct 8 2018, 2:06 PM

Reid fixed the underscore issue in rL335304, so you may wanna give that a shot.

There's still a bug there: https://bugs.llvm.org/show_bug.cgi?id=39218 We don't match GCC's section name still. =/