Details
Diff Detail
- Repository
- rLLD LLVM Linker
- Build Status
Buildable 34196 Build 34195: arc lint + arc unit
Event Timeline
No objections to adding the assert, will be worth adding a bit more context as I can see someone modifying Symbol getting an assert fail and not knowing why the assert is there.
| ELF/Symbols.h | ||
|---|---|---|
| 469 | This assert will be the same for each symbol, slightly better for compile time to put in assertSymbols(). I think it will be worth mentioning where the 80 comes from and why the assert is there? I'm guessing it is a soft limit rather than a hard requirement. I think it will be worth a comment: It is important to keep the size of the SymbolUnion small for performance reasons. 80 bytes is a soft limit based on the size of Defined on a 64-bit system. An alternative, assuming Defined is the reference size: static_assert(sizeof(T) <= sizeof(Defined), "Classes derived from Symbol must be no bigger than Defined"); Obviously doesn't stop Symbol or Defined from getting larger though. | |
Ping :)
The wasm counterpart was committed as D64238. For this patch, we don't have to change unsigned to bool because the byte offset is already aligned.
@aykevl left a comment at https://reviews.llvm.org/D64238#1731181, which seems to suggest that the mingw32 compiler may have a different sizeof(SymbolUnion). Can you print its size?
I did some monkey-patching of Symbol.h:
--- a/lld/ELF/Symbols.h +++ b/lld/ELF/Symbols.h @@ -475,6 +475,12 @@ template <typename T> struct AssertSymbol { static_assert(sizeof(T) <= sizeof(SymbolUnion), "SymbolUnion too small"); static_assert(alignof(T) <= alignof(SymbolUnion), "SymbolUnion not aligned enough"); + static_assert(sizeof(T) <= 100, "Type bigger than 100 bytes"); + static_assert(sizeof(T) <= 96, "Type bigger than 96 bytes"); + static_assert(sizeof(T) <= 92, "Type bigger than 92 bytes"); + static_assert(sizeof(T) <= 88, "Type bigger than 88 bytes"); + static_assert(sizeof(T) <= 84, "Type bigger than 84 bytes"); + static_assert(sizeof(T) <= 80, "Type bigger than 80 bytes"); }; static inline void assertSymbols() {
The result is the following error:
C:\ProgramData\chocolatey\bin\c++.exe -DGTEST_HAS_RTTI=0 -D_FILE_OFFSET_BITS=64 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/lld/ELF -ID:/a/1/s/llvm-project/lld/ELF -ID:/a/1/s/llvm-project/lld/include -Itools/lld/include -Iinclude -ID:/a/1/s/llvm-project/llvm/include -Wa,-mbig-obj -Werror=date-time -std=gnu++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -O2 -DNDEBUG -fno-exceptions -fno-rtti -MD -MT tools/lld/ELF/CMakeFiles/lldELF.dir/MapFile.cpp.obj -MF tools\lld\ELF\CMakeFiles\lldELF.dir\MapFile.cpp.obj.d -o tools/lld/ELF/CMakeFiles/lldELF.dir/MapFile.cpp.obj -c D:/a/1/s/llvm-project/lld/ELF/MapFile.cpp
In file included from D:/a/1/s/llvm-project/lld/ELF/SymbolTable.h:13,
from D:/a/1/s/llvm-project/lld/ELF/MapFile.cpp:25:
D:/a/1/s/llvm-project/lld/ELF/Symbols.h:470:35: error: static assertion failed: SymbolUnion too large
static_assert(sizeof(SymbolUnion) <= 80, "SymbolUnion too large");
~~~~~~~~~~~~~~~~~~~~^~~~~
D:/a/1/s/llvm-project/lld/ELF/Symbols.h: In instantiation of 'struct lld::elf::AssertSymbol<lld::elf::Defined>':
D:/a/1/s/llvm-project/lld/ELF/Symbols.h:487:25: required from here
D:/a/1/s/llvm-project/lld/ELF/Symbols.h:482:27: error: static assertion failed: Type bigger than 84 bytes
static_assert(sizeof(T) <= 84, "Type bigger than 84 bytes");
~~~~~~~~~~^~~~~
D:/a/1/s/llvm-project/lld/ELF/Symbols.h:483:27: error: static assertion failed: Type bigger than 80 bytes
static_assert(sizeof(T) <= 80, "Type bigger than 80 bytes");
~~~~~~~~~~^~~~~
D:/a/1/s/llvm-project/lld/ELF/Symbols.h: In instantiation of 'struct lld::elf::AssertSymbol<lld::elf::SharedSymbol>':
D:/a/1/s/llvm-project/lld/ELF/Symbols.h:490:30: required from here
D:/a/1/s/llvm-project/lld/ELF/Symbols.h:482:27: error: static assertion failed: Type bigger than 84 bytes
static_assert(sizeof(T) <= 84, "Type bigger than 84 bytes");
~~~~~~~~~~^~~~~
D:/a/1/s/llvm-project/lld/ELF/Symbols.h:483:27: error: static assertion failed: Type bigger than 80 bytes
static_assert(sizeof(T) <= 80, "Type bigger than 80 bytes");
~~~~~~~~~~^~~~~From that it looks like both Defined and SharedSymbol are bigger than 80 bytes, most likely 88 bytes.
@mstorsjo Do you have more idea about this - on chocolatey\bin\c++.exe, sizeof(SymbolUnion) is different from both Itanium and MSVC sizes?
I wrote a patch, see: https://reviews.llvm.org/D70266
This runs on Azure Pipelines with the Bash task. I'm not sure which exact compiler is used, but it is most likely MinGW (GCC) using the MinGW ABI because that's the only ABI that can be used from Go (CGo) on Windows. I can check the exact compiler (and version) if needed.
This assert will be the same for each symbol, slightly better for compile time to put in assertSymbols().
I think it will be worth mentioning where the 80 comes from and why the assert is there? I'm guessing it is a soft limit rather than a hard requirement. I think it will be worth a comment:
An alternative, assuming Defined is the reference size:
Obviously doesn't stop Symbol or Defined from getting larger though.