Page MenuHomePhabricator

[CodeView] Align type records on 4-bytes when emitting PDBs
ClosedPublic

Authored by aganea on Feb 24 2020, 2:35 PM.

Details

Summary

When emitting PDBs, the TypeStreamMerger class is used to merge .debug$T records from the input .OBJ files into the output .PDB stream.
Records in .OBJs are not required to be aligned on 4-bytes, and "The Netwide Assembler 2.14" generates non-aligned records.

When compiling with -DLLVM_ENABLE_ASSERTIONS=ON, an assert was triggered in MergingTypeTableBuilder when non-ghash merging was used.
With ghash merging there was no assert.
As a result, LLD could potentially generate a non-aligned TPI stream.

We now align records on 4-bytes when record indices are remapped, in TypeStreamMerger::remapIndices().

Diff Detail

Event Timeline

aganea created this revision.Feb 24 2020, 2:35 PM
aganea marked an inline comment as done.Feb 24 2020, 2:41 PM
aganea added inline comments.
llvm/include/llvm/DebugInfo/CodeView/GlobalTypeTableBuilder.h
74

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.

aganea updated this revision to Diff 249496.Mar 10 2020, 2:25 PM
aganea edited the summary of this revision. (Show Details)
aganea added reviewers: amccarth, ruiu.
aganea added subscribers: amccarth, hans, ruiu.

Updated with better test.
I've moved the test in LLD because I can't find a codepath in LLVM that uses the TypeStreamMerger with OBJs as inputs and outputting into a PDB. The closest is llvm-pdbutil merge ... but that only takes TypeServer PDBs as inputs.

Adding more people:
+@amccarth
+@ruiu for the LLD test.
+@hans : This occurs in release/10.x but it's probably there for a long time.

rnk accepted this revision.Mar 10 2020, 4:30 PM

lgtm

Thanks! I guess we never encountered non-aligned type records.

lld/test/COFF/pdb-tpi-aligned-records.test
22

Are you sure we don't ignore SectionData? If you confirm the test fails if you remove the LF_PAD code before committing, that's good enough for me.

I guess to be doubly sure we could remove the Types: block below. I would expect yaml2obj to fall back to SectionData.

This revision is now accepted and ready to land.Mar 10 2020, 4:30 PM
aganea marked 2 inline comments as done.Mar 11 2020, 6:27 AM

Thanks for reviewing this Reid!

lld/test/COFF/pdb-tpi-aligned-records.test
22

If I comment out SectionData:, the test fails because the TPI record now starts with 0E000810 (the source Type stream is generated by LLVM) instead of 12000810 (the source Type stream is generated by NASM). This is because NASM generates wrongly-sized LF_PROCEDURE records, I've filled a bug here: https://bugzilla.nasm.us/show_bug.cgi?id=3392651
The SectionData: stream above was copy-pasted from a NASM-generated OBJ file.

If I remove the changes to the code, the test fails, and the records after become unaligned:

I only left Types: for information purposes, but I can comment it out, to make it obvious that the SectionData: is used.

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?

llvm/include/llvm/DebugInfo/CodeView/GlobalTypeTableBuilder.h
74

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?

75

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!"

aganea updated this revision to Diff 249720.EditedMar 11 2020, 12:37 PM
aganea marked 4 inline comments as done.

I find the assert messages and some of the comments mildly misleading.

Fixed, please check the updated diff. Let me know if that's better.

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?

The consumer only uses the llvm::codeview::RecordPrefix::RecordKind, which maps to the LF_* types (see TypeRecord.h). The record length llvm::codeview::RecordPrefix::RecordLen -- which we're modifying in this patch -- is only used for quickly skimming through a PDBs in the Visual Studio debugger: https://github.com/microsoft/microsoft-pdb/blob/082c5290e5aff028ae84e43affa8be717aa7af73/PDB/include/symtypeutils.h#L190. I suppose one reason for this 4 byte alignment is that unaligned reads were more expensive at the time the spec/code were written.

llvm/include/llvm/DebugInfo/CodeView/GlobalTypeTableBuilder.h
74
75

Fixed.

amccarth accepted this revision.Mar 11 2020, 1:13 PM

Thanks!

Thanks for the feedback Adrian!

This revision was automatically updated to reflect the committed changes.