Page MenuHomePhabricator

[ELF] Fix lld build on Windows/MinGW
ClosedPublic

Authored by aykevl on Nov 14 2019, 12:05 PM.

Details

Summary

The patch in https://reviews.llvm.org/D64077 causes a build failure because both the Defined and SharedSymbol classes are bigger than 80 bytes.

This patch fixes this build failure by changing the type of the bitfields. It is a similar change to the bitfield changes in https://reviews.llvm.org/D64238, but instead of changing to bool I decided to use uint8_t because one of the bitfields takes up two bits instead of one.

Note: I still need to run the tests on this patch, but it seems to compile in my setup.
I successfully ran ninja check-lld on a x64 Linux system with this patch applied.

Diff Detail

Event Timeline

aykevl created this revision.Nov 14 2019, 12:05 PM
Herald added a project: Restricted Project. · View Herald Transcript
mstorsjo added a subscriber: rnk.

Hmm, this is strange - I've successfully built lld for the MinGW ABI, but using clang, not gcc. If this fixes things for gcc, it would suggest that clang doesn't manage to match gcc's struct/bitfield layout in the previous case.

The change itself seems fine with me though. Adding @rnk, as he worked a lot on the struct layout/size thingies, and he might have some insight into the bitfield layout issue.

ruiu added a comment.Nov 15 2019, 12:55 AM

Yeah this is very interesting. This may be a sign that clang is not ABI compatible with GNU on Windows when bitfields are used this way.

FWIW, with GCC 7.3 targeting mingw, I'm getting the exact same size for both GCC and Clang, and the struct size assertions you're referring to pass just fine. With what version of GCC are you seeing different sizes?

I ran the following commands in CI:

gcc --version
c++ --version

And I got the following output:

gcc.exe (x86_64-posix-seh-rev0, Built by MinGW-W64 project) 8.1.0
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

c++.exe (x86_64-posix-seh-rev0, Built by MinGW-W64 project) 8.1.0
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Additionally, this is a part of the output of the CMake invocation:

-- The C compiler identification is GNU 8.1.0
-- The CXX compiler identification is GNU 8.1.0
-- The ASM compiler identification is GNU
-- Found assembler: C:/ProgramData/chocolatey/bin/gcc.exe
-- Check for working C compiler: C:/ProgramData/chocolatey/bin/gcc.exe
-- Check for working C compiler: C:/ProgramData/chocolatey/bin/gcc.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: C:/ProgramData/chocolatey/bin/c++.exe
-- Check for working CXX compiler: C:/ProgramData/chocolatey/bin/c++.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done

Another test. I compiled the following code: https://godbolt.org/z/hVCAUB

#include <stdint.h>

struct Bits {
    unsigned A : 1;
    uint8_t x;
};

int foo = sizeof(struct Bits);

As you can see in the Compiler Explorer, normal Linux/x86_64 outputs value 4 for foo:

foo:
        .long   4                       # 0x4

However, for the compiler I'm dealing with I'm getting the following output:

size.o:     file format pe-x86-64


Disassembly of section .data:

0000000000000000 <foo>:
   0:	08 00                	or     %al,(%rax)
	...

So apparently this particular build of GCC 8 uses 8 bytes for the struct with bitfields.

aykevl edited the summary of this revision. (Show Details)Nov 15 2019, 9:19 AM
aykevl edited the summary of this revision. (Show Details)

I ran the same code as above on MinGW 7.3.0, and with that version the struct size is 4 bytes. So it looks like GCC/MinGW changed how they lay out a struct in memory (breaking ABI compatibility?).

size.o:     file format pe-x86-64


Disassembly of section .data:

0000000000000000 <foo>:
   0:	04 00                	add    $0x0,%al
	...
gcc.exe (x86_64-posix-seh-rev0, Built by MinGW-W64 project) 7.3.0
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Therefore, this might actually be a bug in GCC rather than Clang. This patch would still be helpful to get LLVM to compile with MinGW 8.


I also ran ninja check-lld with this patch applied, which completed successfully on a linux/x86_64 system. It would be nice if someone could review this patch to get it included in a future LLVM release.

rnk accepted this revision.Nov 15 2019, 9:42 AM

I think this change looks good.

So far nobody has mentioned that the reduced struct (struct Foo { unsigned myBit : 1; char myByte; };) is 8 bytes with MSVC. It's highly possible that GCC is aligning their ABI with MSVC for better compatibility.

What I don't know is why the static_assert doesn't trigger with MSVC & clang-cl.

In any case, let's file a bug to:

  1. Track down the change in GCC so we know when and why this happened
  2. Decide if we should change clang's behavior to match GCC's bitfield behavior on Windows. This may be as simple as changing the -fms-bitfields default.

I'm not sure clang knows the GCC version it is aiming for compatibility with, but as long as we have a flag to get back the other behavior, we're doing pretty well.

This revision is now accepted and ready to land.Nov 15 2019, 9:42 AM

So far nobody has mentioned that the reduced struct (struct Foo { unsigned myBit : 1; char myByte; };) is 8 bytes with MSVC. It's highly possible that GCC is aligning their ABI with MSVC for better compatibility.

Right, then it makes sense that GCC/MinGW has changed its behavior.
https://godbolt.org/z/hzEGgj

This revision was automatically updated to reflect the committed changes.