[PATCH] D61698: [COFF] Store alignment in log2 form, NFC

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 3 inline comments as done.
rnk added a comment.

In the long term, I think we want to hoist section characteristics up into chunk, and that's going to create the 8192 max alignment constraint. This is just a first step to refactor the code to use the accessors.



================
Comment at: lld/COFF/Chunks.h:73
+    Align = Align ? Align : 1;
+    assert(llvm::isPowerOf2_32(Align) && "alignment is not a power of 2");
+    P2Align = llvm::Log2_32(Align);
----------------
ruiu wrote:
> If this can be triggered by feeding a bad file, this should be a call of error() instead of assert().
I don't think it's possible. I think it might be possible with -aligncomm or -functionpadmin, though, since those raise alignment on the command line and take arbitrary input.


================
Comment at: lld/COFF/Chunks.h:136
+  // The alignment of this chunk. The writer uses the value. Maximum object file
+  // alignment is 2**13, or 8192, so valid values are 0 to 13.
+  uint8_t P2Align = 0;
----------------
ruiu wrote:
> Instead of emphasizing that the maximum alignment is 2^13, can we support an alignment up to a reasonable upper limit, say, 2^32? That's apparently large enough, and by doing that, we do not have to explain a trivia that what value is the maximum alignment value we can express by COFF. (And the runtime cost of doing that is virtually nothing.)
Originally I was planning to make this a bitfield, in which case, the comment is relevant, but if we store the characteristics in the on-disk format here, then this comment isn't really needed. It makes more sense to move it to the setter, since that has to deal with the edge case of invalid alignments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61698/new/

https://reviews.llvm.org/D61698