It was checking for 64-bit builds incorrectly. Unfortunately,
ConcatInputSection has grown a bit in the meantime, and I don't see any
obvious way to shrink it. Perhaps icfEqClass could use 32-bit hashes
instead of 64-bit ones, but xxHash64 is supposed to be much faster than
xxHash32 (https://github.com/Cyan4973/xxHash#benchmarks), so that sounds
like a loss. (Unrelatedly, we should really look at using XXH3 instead
of xxHash64 now.)
Details
Details
- Reviewers
gkm int3 - Group Reviewers
Restricted Project - Commits
- rG637a3396b3f8: [MachO] Fix struct size assertion
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
How do people feel about moving asserts like this into the corresponding source files (InputSection.cpp) instead of the headers? It's nice to keep the assert close to the definition, but it also causes errors to be emitted from every single source file including the header (I got 21 separate errors when I fixed the size check without updating the size, for example).
Comment Actions
Oof, what an oversight... thanks for catching it!
How do people feel about moving asserts like this into the corresponding source files (InputSection.cpp) instead of the headers?
Sounds good to me!
Comment Actions
(it is apparently 128).
This doesn't seem the greatest way to try and reduce memory usage in the first place.
Comment Actions
Could you re-run the compilation of InputSection.cpp with the /d1reportAllClassLayout argument added, so I can see how the structs are getting laid out? (https://reviews.llvm.org/paste/edit/form/default/ is a handy place to upload that.) Thanks!
Comment Actions
I tried, but this will take me longer to set up, which I can't do right now. I may be able to return to this later if I am still blocked by it.
I would highly recommend simply not using hardcoded constants like that in portable code however, or to clearly #ifdef whatever platform you are assuming this should be valid for.
Comment Actions
There shouldn't be anything platform-specific (besides the pointer size, which is already accounted for) affecting the size of this struct (and the assertion doesn't fail for me on the MSVC environments I have access to, and I haven't gotten any buildbot failures either), which is why I wanted those details to understand what was different about your environment and why this size assertion wasn't portable.
Comment Actions
We are also seeing this in our internal Windows builds. As can be seen below, the size difference in debug is due to the std::vector<lld::macho::Reloc> member, which is actually due to the _Vector_val internal vector member that uses a different base class in debug mode (std::_Container_base12 instead of std::_Container_base0). Could we simply turn off this static assert for debug builds (e.g. with #ifdef NDEBUG)?
Class layout of ConcatInputSection x64 debug:
class lld::macho::ConcatInputSection size(128):
+---
0 | +--- (base class lld::macho::InputSection)
0 | | {vfptr}
8 | | parent
16 | | align
20. | | callSiteCount (bitstart=0,nbits=31)
20. | | isFinal (bitstart=31,nbits=1)
24 | | ?$ArrayRef@E data
40 | | ?$vector@UReloc@macho@lld@@V?$allocator@UReloc@macho@lld@@@std@@ relocs
72 | | ?$TinyPtrVector@PEAVDefined@macho@lld@@ symbols
80 | | shared
| +---
88 | replacement
96 | icfEqClass
112 | wasCoalesced
113 | live
| <alignment member> (size=6)
120 | outSecOff
+---
class std::vector<struct lld::macho::Reloc,class std::allocator<struct lld::macho::Reloc> > size(32):
+---
0 | ?$_Compressed_pair@V?$allocator@UReloc@macho@lld@@@std@@V?$_Vector_val@U?$_Simple_types@UReloc@macho@lld@@@std@@@2@$00 _Mypair
+---
class std::_Compressed_pair<class std::allocator<struct lld::macho::Reloc>,class std::_Vector_val<struct std::_Simple_types<struct lld::macho::Reloc> >,1> size(32):
+---
0 | +--- (base class std::allocator<struct lld::macho::Reloc>)
| +---
0 | ?$_Vector_val@U?$_Simple_types@UReloc@macho@lld@@@std@@ _Myval2
+---
class std::_Vector_val<struct std::_Simple_types<struct lld::macho::Reloc> > size(32):
+---
0 | +--- (base class std::_Container_base12)
0 | | _Myproxy
| +---
8 | _Myfirst
16 | _Mylast
24 | _Myend
+---The same layout for x64 release:
class lld::macho::ConcatInputSection size(120):
+---
0 | +--- (base class lld::macho::InputSection)
0 | | {vfptr}
8 | | parent
16 | | align
20. | | callSiteCount (bitstart=0,nbits=31)
20. | | isFinal (bitstart=31,nbits=1)
24 | | ?$ArrayRef@E data
40 | | ?$vector@UReloc@macho@lld@@V?$allocator@UReloc@macho@lld@@@std@@ relocs
64 | | ?$TinyPtrVector@PEAVDefined@macho@lld@@ symbols
72 | | shared
| +---
80 | replacement
88 | icfEqClass
104 | wasCoalesced
105 | live
| <alignment member> (size=6)
112 | outSecOff
+---
class std::vector<struct lld::macho::Reloc,class std::allocator<struct lld::macho::Reloc> > size(24):
+---
0 | ?$_Compressed_pair@V?$allocator@UReloc@macho@lld@@@std@@V?$_Vector_val@U?$_Simple_types@UReloc@macho@lld@@@std@@@2@$00 _Mypair
+---
class std::_Compressed_pair<class std::allocator<struct lld::macho::Reloc>,class std::_Vector_val<struct std::_Simple_types<struct lld::macho::Reloc> >,1> size(24):
+---
0 | +--- (base class std::allocator<struct lld::macho::Reloc>)
| +---
0 | ?$_Vector_val@U?$_Simple_types@UReloc@macho@lld@@@std@@ _Myval2
+---
class std::_Vector_val<struct std::_Simple_types<struct lld::macho::Reloc> > size(24):
+---
0 | +--- (base class std::_Container_base0)
| +---
0 | _Myfirst
8 | _Mylast
16 | _Myend
+---Comment Actions
https://github.com/llvm/llvm-project/commit/2f5d6a0ea51b should fix the assertion. Sorry for the breakage here.