[ELF] Fix lld build on Windows/MinGW

[aykevl]

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.

[mstorsjo]

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]

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.

[mstorsjo]

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?

[aykevl]

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

[aykevl]

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]

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]

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.

[aykevl]

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

lld/ELF/Symbols.h

public:
   // The following fields have the same meaning as the ELF symbol attributes.
   uint8_t type;    // symbol type
   uint8_t stOther; // st_other field value
 
   uint8_t symbolKind;
 
   // Symbol visibility. This is the computed minimum visibility of all
   // observed non-DSO symbols.
-  unsigned visibility : 2;
+  uint8_t visibility : 2;
 
   // True if the symbol was used for linking and thus need to be added to the
   // output file's symbol table. This is true for all symbols except for
   // unreferenced DSO symbols, lazy (archive) symbols, and bitcode symbols that
   // are unreferenced except by other bitcode objects.
-  unsigned isUsedInRegularObj : 1;
+  uint8_t isUsedInRegularObj : 1;
 
   // Used by a Defined symbol with protected or default visibility, to record
   // whether it is required to be exported into .dynsym. This is set when any of
   // the following conditions hold:
   //
   // - If there is an interposable symbol from a DSO.
   // - If -shared or --export-dynamic is specified, any symbol in an object
   //   file/bitcode sets this property, unless suppressed by LTO
   //   canBeOmittedFromSymbolTable().
-  unsigned exportDynamic : 1;
+  uint8_t exportDynamic : 1;
 
   // True if the symbol is in the --dynamic-list file. A Defined symbol with
   // protected or default visibility with this property is required to be
   // exported into .dynsym.
-  unsigned inDynamicList : 1;
+  uint8_t inDynamicList : 1;
 
   // False if LTO shouldn't inline whatever this symbol points to. If a symbol
   // is overwritten after LTO, LTO shouldn't inline the symbol because it
   // doesn't know the final contents of the symbol.
-  unsigned canInline : 1;
+  uint8_t canInline : 1;
 
   // Used by Undefined and SharedSymbol to track if there has been at least one
   // undefined reference to the symbol. The binding may change to STB_WEAK if
   // the first undefined reference from a non-shared object is weak.
-  unsigned referenced : 1;
+  uint8_t referenced : 1;
 
   // True if this symbol is specified by --trace-symbol option.
-  unsigned traced : 1;
+  uint8_t traced : 1;
 
   inline void replace(const Symbol &newSym);
 
   bool includeInDynsym() const;
   uint8_t computeBinding() const;
   bool isWeak() const { return binding == llvm::ELF::STB_WEAK; }
 
   bool isUndefined() const { return symbolKind == UndefinedKind; }
Symbol(Kind k, InputFile *file, StringRefZ name, uint8_t binding,
         canInline(false), referenced(false), traced(false), needsPltAddr(false),
         isInIplt(false), gotInIgot(false), isPreemptible(false),
         used(!config->gcSections), needsTocRestore(false),
         scriptDefined(false) {}
 
 public:
   // True the symbol should point to its PLT entry.
   // For SharedSymbol only.
-  unsigned needsPltAddr : 1;
+  uint8_t needsPltAddr : 1;
 
   // True if this symbol is in the Iplt sub-section of the Plt and the Igot
   // sub-section of the .got.plt or .got.
-  unsigned isInIplt : 1;
+  uint8_t isInIplt : 1;
 
   // True if this symbol needs a GOT entry and its GOT entry is actually in
   // Igot. This will be true only for certain non-preemptible ifuncs.
-  unsigned gotInIgot : 1;
+  uint8_t gotInIgot : 1;
 
   // True if this symbol is preemptible at load time.
-  unsigned isPreemptible : 1;
+  uint8_t isPreemptible : 1;
 
   // True if an undefined or shared symbol is used from a live section.
-  unsigned used : 1;
+  uint8_t used : 1;
 
   // True if a call to this symbol needs to be followed by a restore of the
   // PPC64 toc pointer.
-  unsigned needsTocRestore : 1;
+  uint8_t needsTocRestore : 1;
 
   // True if this symbol is defined by a linker script.
-  unsigned scriptDefined : 1;
+  uint8_t scriptDefined : 1;
 
   // The partition whose dynamic symbol table contains this symbol's definition.
   uint8_t partition = 1;
 
   bool isSection() const { return type == llvm::ELF::STT_SECTION; }
   bool isTls() const { return type == llvm::ELF::STT_TLS; }
   bool isFunc() const { return type == llvm::ELF::STT_FUNC; }
   bool isGnuIFunc() const { return type == llvm::ELF::STT_GNU_IFUNC; }