smeenai added inline comments.
================
Comment at: lld/COFF/Chunks.cpp:47
+namespace {
+// This class exists just for the purpose of calculating the expected size of
----------------
pcc wrote:
> rnk wrote:
> > ruiu wrote:
> > > rnk wrote:
> > > > ruiu wrote:
> > > > > rnk wrote:
> > > > > > ruiu wrote:
> > > > > > > This might be useful but at the same time it looks a bit overly
> > > > > > > cautious? Perhaps the symbol size is more important but we don't
> > > > > > > have something like this for them, for example.
> > > > > > Well, we don't have checks for symbol size yet. :)
> > > > > >
> > > > > > Given that we don't have performance monitoring, I really want
> > > > > > people to think hard before they casually add another field to
> > > > > > SectionChunk. I wouldn't insist on it if we did, but these types of
> > > > > > static_asserts have proven useful in LLVM for preventing size creep.
> > > > > Then maybe reiterating everything again, how about checking directly
> > > > > with a number like `static_assert(sizeof(Chunk) == 48)`? This should
> > > > > suffice to prevent making the struct larger by accident.
> > > > It would be wrong for 32-bit. Rather than trying to express it as math
> > > > on sizeof(void*), I think the struct makes it clearer. And it helps
> > > > document hidden members like vptr.
> > > Yeah, but it still feels weird to me to repeat all the members again in a
> > > different file just for the purpose of assertion. For 32-bit, we can just
> > > set the upper bound like `static_assert(sizeof(Chunk) <= 48)` where 48 is
> > > the size of the struct in 64-bit.
> > Fair enough. Honestly, writing down all the fields in one place helped me
> > identify the profitable reorderings, but we don't need to commit it.
> I found D59044 (and, to a lesser extent, D59269) by staring at the output of
> clang with `-Xclang -fdump-record-layouts`. Maybe it would be worth adding
> this trick to the documentation somewhere?
The clang static analyzer has a padding checker
(https://clang.llvm.org/docs/analyzer/checkers.html#optin-performance-padding),
and clang-tools-extra has clang-reorder-fields to automatically reorder fields
according to the padding checker's findings. I haven't used those myself, but a
coworker had success with them in the past.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59797/new/
https://reviews.llvm.org/D59797
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
