Page MenuHomePhabricator

[MachO] Fix struct size assertion
ClosedPublic

Authored by smeenai on Nov 12 2021, 3:13 PM.

Details

Reviewers
gkm
int3
Group Reviewers
Restricted Project
Commits
rG637a3396b3f8: [MachO] Fix struct size assertion
Summary

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.)

Diff Detail

Event Timeline

smeenai created this revision.Nov 12 2021, 3:13 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
smeenai requested review of this revision.Nov 12 2021, 3:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2021, 3:13 PM

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).

int3 accepted this revision.Nov 12 2021, 4:31 PM
int3 added a subscriber: int3.

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!

This revision is now accepted and ready to land.Nov 12 2021, 4:31 PM
This revision was automatically updated to reflect the committed changes.

This assert is failing on VS2019 x64

(it is apparently 128).

This doesn't seem the greatest way to try and reduce memory usage in the first place.

This assert is failing on VS2019 x64

Do you have a buildbot link, or is this local?

It's local.

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!

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.

smeenai added a comment.EditedNov 22 2021, 12:26 PM

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.

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.

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
	+---

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
	+---

Ah, that makes sense. I'll put up a fix.

https://github.com/llvm/llvm-project/commit/2f5d6a0ea51b should fix the assertion. Sorry for the breakage here.

https://github.com/llvm/llvm-project/commit/2f5d6a0ea51b should fix the assertion. Sorry for the breakage here.

I confirm that the new commit fixed our builds. Thanks!