[PATCH] D75081: [CodeView] Align type records on 4-bytes when emitting PDBs

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

I find the assert messages and some of the comments mildly misleading.  In my mind, there's a difference between a record's size and a record's alignment.  One way to achieve alignment is start at an aligned address and to make sure each record has a size that's a multiple of the alignment.  That seems to be the approach here, and that's fine.  But I'm concerned the wording choice could mislead someone who ends up trying to diagnose any problems in this vicinity.

Not knowing the details of how the records are actually read back and used, I harbor some concern that padding out the size of the record might confuse the consumer.  If a record actually works out to 43 bytes, this code will copy the 43 bytes, add one pad byte, and change the record size to 44.  Is it important that the consumer know the original size was 43 so that they don't mistake the pad byte for actual record data?



================
Comment at: llvm/include/llvm/DebugInfo/CodeView/GlobalTypeTableBuilder.h:74
                            CreateFunc Create) {
+    assert(RecordSize < UINT32_MAX && "Record too big");
+    assert(RecordSize % 4 == 0 && "Record is not aligned to 4 bytes!");
----------------
aganea wrote:
> I just duplicated these two lines from MergingTypeTableBuilder, but I think the test is wrong, it should say `RecordSize < MaxRecordLength` (which is 0xFF00). Changing it breaks the `long-name.ll` test, I could send a patch later.
Or perhaps `RecordSize <= MaxRecordLength`.  If it's one byte short of MaxRecordLength, then it should have been rounded up to the next multiple of 4 bytes, so MaxRecordLength itself is a legal size, right?


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/GlobalTypeTableBuilder.h:75
+    assert(RecordSize < UINT32_MAX && "Record too big");
+    assert(RecordSize % 4 == 0 && "Record is not aligned to 4 bytes!");
+
----------------
I know you just copied these lines, but the assert message is slightly misleading.  Record size and alignment are related but different things, so it might be better to say something like "RecordSize is not a multiple of 4 bytes which will cause misalignment!"


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

https://reviews.llvm.org/D75081