This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo]: preparation to implement DW_AT_alignment
ClosedPublic

Authored by vleschuk on Sep 30 2016, 12:00 AM.

Details

Summary

As discussed in https://reviews.llvm.org/D24425 splitting the implementation of DW_AT_alignment into several patches. This one is a preparation that includes:

  • Added alignment attribute to DIVariable family: modified DIGlobalVariable and DILocalVariable classes and their users (like DIBuilder)
  • Added support for alignment in DI*Variable in bitcode. Had to perform a little workaround to support older bitcode versions (see BitcodeReader.cpp and BitcodeWriter.cpp)
  • As discussed in https://reviews.llvm.org/D24425 renamed "AlignInBits" into "AlignInBytes" to match the fact that user specifies alignment in bytes in code and DWARF attribute also displays this value in bytes.

Diff Detail

Repository
rL LLVM

Event Timeline

vleschuk updated this revision to Diff 72987.Sep 30 2016, 12:00 AM
vleschuk retitled this revision from to [DebugInfo]: preparation to implement DW_AT_alignment.
vleschuk updated this object.
vleschuk added a subscriber: llvm-commits.
vleschuk updated this revision to Diff 73535.Oct 4 2016, 12:47 PM
  • Revert API changes: DIBuilder create* methods now take alignment in bits instead of bytes, like before (conversion will be done when emitting DW_AT_alignment)
  • Remove alignment from createBasicType and createBitFieldMember - not applicable
aprantl edited edge metadata.Oct 4 2016, 1:01 PM

There also needs to be a bitcode upgrade test if you are changing the serialization format.

lib/Bitcode/Reader/BitcodeReader.cpp
2779 ↗(On Diff #73535)

Why change this?

2781 ↗(On Diff #73535)

"." at the end of the sentence; also it is probably not the 11th field.

lib/Bitcode/Writer/BitcodeWriter.cpp
1725 ↗(On Diff #73535)

Can you explain?

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1409 ↗(On Diff #73535)

Why is this correct?

vleschuk marked an inline comment as done.Oct 6 2016, 2:57 AM
vleschuk added inline comments.
lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1409 ↗(On Diff #73535)

Actually in old code the following is always true

DT->getAlignInBits() == FieldSize;

Because

FieldSize == DD->getBaseTypeSize(DT);

and for bitfields Alignment field was set in the following way:

AlignInBits = CGM.getContext().getTypeAlign(BitFieldDecl->getType());

The variable names are not chosen very well here, I think this part of code needs a little refactoring, however I think it deserves separate commit.

As now we have got rid of getAlignInBits() for bitfields, we just use default type alignment here directly.

vleschuk updated this revision to Diff 73767.Oct 6 2016, 5:00 AM
vleschuk edited edge metadata.
vleschuk marked an inline comment as done.
  • Added bitcode test for DILocalVariable
  • Added comment for BitcodeWriter explaining serialization format change for DILocalVariable

There also needs to be a bitcode upgrade test if you are changing the serialization format.

I have added a bitcode test: create a *.bc file with llvm-3.9 and added a check that it is disassembled correctly by current version. Global variable case is already covered by diglobalvariable-3.8.ll test and I don't think it needs additional tests as the only change is additional field in the end, and there are already assembler tests which check correct assembly/disassembly procedures.

An alternative for DILocalVariable would be to stick a HasAlignment bit into Record[0] & 2, similar to how DISubprogram works. This may be simpler / more readable / more space efficient.

Do any of the bitfield tests need upgrading to remove the alignment and verify they still work?

include/llvm/IR/DIBuilder.h
209 ↗(On Diff #73767)

Have you verified that this doesn't break bitfields?

vleschuk updated this revision to Diff 74113.Oct 10 2016, 2:47 AM
  • Store information regarding bitcode format in Record[0] field for DILocalVariable (HasAlignment flag)
  • Remove "align: " field from bitfield tests
vleschuk marked an inline comment as done.Oct 10 2016, 2:51 AM

An alternative for DILocalVariable would be to stick a HasAlignment bit into Record[0] & 2, similar to how DISubprogram works. This may be simpler / more readable / more space efficient.

Done, thanks for the tip!

Do any of the bitfield tests need upgrading to remove the alignment and verify they still work?

Yep, thanks for reminding. Removed alignment debug info from bitfield-related tests and checked that they pass.

include/llvm/IR/DIBuilder.h
209 ↗(On Diff #73767)

Yes, I ran some manual tests, and also double checked bitfield-related tests (both for DWARF and COFF).

aprantl added inline comments.Oct 10 2016, 12:50 PM
include/llvm/IR/DIBuilder.h
460 ↗(On Diff #74113)

missing space

461 ↗(On Diff #74113)

extra space :-)

include/llvm/IR/DebugInfoMetadata.h
1829 ↗(On Diff #74113)

Sorry for noticing this so late:
Does this really need to be a 64bit integer, or would a 32-bit unsigned be sufficient?
A 32-bit unsigned could immediately follow the previous unsigned field und thus save 8 bytes.

lib/Bitcode/Writer/BitcodeWriter.cpp
1735 ↗(On Diff #74113)

It is more useful to future readers to explain what we did, rather than explain what alternatives we rejected ands why.
I would just enumerate the 3(?) different formats of this record and how to differentiate them.

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1410 ↗(On Diff #74113)

Thanks!

vleschuk updated this revision to Diff 74168.Oct 10 2016, 1:31 PM
vleschuk marked an inline comment as done.
  • Fixed the comment for DILocalVariable in BitcodeWriter to be more self-descriptive.
vleschuk marked an inline comment as done.Oct 10 2016, 1:31 PM
vleschuk added inline comments.
include/llvm/IR/DebugInfoMetadata.h
1829 ↗(On Diff #74113)

SizeInBits and OffsetInBits in DebugInfoMetadata scope are 64bit. If we change this to 32bit for efficiency I'd suggest to make it as separate commit and change all of them.

lib/Bitcode/Writer/BitcodeWriter.cpp
1735 ↗(On Diff #74113)

Done. Updated comment.

vleschuk updated this revision to Diff 74169.Oct 10 2016, 1:39 PM
vleschuk marked an inline comment as done.
  • Fix indentation in comment.
aprantl added inline comments.Oct 10 2016, 1:57 PM
include/llvm/IR/DebugInfoMetadata.h
1829 ↗(On Diff #74113)

I don't see how that follows: SizeInBits may legitimately have values larger than 2^32, but the alignment should rarely be anything larger than fits in one byte?

probinson added inline comments.
include/llvm/IR/DebugInfoMetadata.h
1829 ↗(On Diff #74113)

It's not hard to imagine page-aligned items and pages are generally bigger than what a byte can express. But it is hard to imagine needing more than a 32-bit value to express alignment.

aprantl added inline comments.Oct 10 2016, 2:24 PM
lib/Bitcode/Writer/BitcodeWriter.cpp
1730 ↗(On Diff #74169)

-> inlinedAt

1737 ↗(On Diff #74169)

I don't think we want this to be static.

vleschuk updated this revision to Diff 74687.Oct 14 2016, 7:42 AM
  • Use DIAlignment type instead of uin64_t
  • Fix mistype in comment
  • HasAlignmentFlag in BitcodeReader is now not static
vleschuk marked 3 inline comments as done.Oct 14 2016, 7:42 AM
aprantl accepted this revision.Oct 14 2016, 9:36 AM
aprantl edited edge metadata.

Thanks, this looks good now!

This revision is now accepted and ready to land.Oct 14 2016, 9:36 AM
This revision was automatically updated to reflect the committed changes.