rnk marked 4 inline comments as done. rnk added inline comments.
================ Comment at: lld/COFF/Chunks.cpp:47 +namespace { +// This class exists just for the purpose of calculating the expected size of ---------------- 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. ================ Comment at: lld/COFF/Chunks.h:232 + // Relocations for this section. ArrayRef<coff_relocation> Relocs; ---------------- I'm not sure we need to store this, actually, it's pretty easy to recompute on demand. At the very least, we could shorten the size to 32-bits. I think ~4 billion is a reasonable implementation limit on relocations. ================ Comment at: lld/COFF/Chunks.h:236 // the thunk instead of the actual original target Symbol. std::vector<Symbol *> RelocTargets; ---------------- riccibruno wrote: > I think that `llvm::SmallVector<Symbol *, 0>` is one pointer smaller, with > the drawback that it is limited to `2^32-1` elements. My plan is to see if I can eliminate it completely, but I wanted to treat it separately once I established that the size of this was important and put a cap on it. ================ Comment at: lld/COFF/Chunks.h:259 StringRef SectionName; std::vector<SectionChunk *> AssocChildren; ---------------- This, for example, is typically 2-3 elements long: each comdat .text section will have a an assoc pdata and xdata. A singly linked list could suffice instead. 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