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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
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!");
----------------
amccarth wrote:
> 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?
Microsoft does an exclusive check, perhaps we should do the same: https://github.com/microsoft/microsoft-pdb/blob/082c5290e5aff028ae84e43affa8be717aa7af73/PDB/dbi/tpi.cpp#L1130


================
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!");
+
----------------
amccarth wrote:
> 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!"
Fixed.


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

https://reviews.llvm.org/D75081