Page MenuHomePhabricator

[ELF] Assert sizeof(SymbolUnion) <= 80
ClosedPublic

Authored by MaskRay on Jul 2 2019, 6:40 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jul 2 2019, 6:40 AM
Herald added a project: Restricted Project. · View Herald Transcript

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 ↗(On Diff #207549)

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.

MaskRay updated this revision to Diff 207558.Jul 2 2019, 7:21 AM

Add the comment suggested by peter.smith

grimar added a comment.Jul 2 2019, 7:36 AM

Looks fine.

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.

ruiu accepted this revision.Jul 8 2019, 11:48 PM

LGTM

This revision is now accepted and ready to land.Jul 8 2019, 11:48 PM
This revision was automatically updated to reflect the committed changes.