This is an archive of the discontinued LLVM Phabricator instance.

[codeview] Add DISubprogram::ThisAdjustment
ClosedPublic

Authored by rnk on Jun 22 2016, 11:50 AM.

Details

Summary

This represents the adjustment applied to the implicit 'this' parameter
in the prologue of a virtual method in the MS C++ ABI. The adjustment is
always zero unless multiple inheritance is involved.

This increases the size of DISubprogram by 8 bytes, unfortunately. The
adjustment really is a signed 32-bit integer. If this size increase is
too much, we could probably win it back by splitting out a subclass with
info specific to virtual methods (virtuality, vindex, thisadjustment,
containingType).

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 61581.Jun 22 2016, 11:50 AM
rnk retitled this revision from to [codeview] Add DISubprogram::ThisAdjustment.
rnk updated this object.
rnk added reviewers: aprantl, dexonsmith.
rnk added subscribers: llvm-commits, amccarth, aaboud.
amccarth added inline comments.Jun 22 2016, 1:45 PM
lib/Bitcode/Reader/BitcodeReader.cpp
2466 ↗(On Diff #61581)

It looks like you inverted the sense of the condition while trying to allow for the extra optional field.

I think you want:

if (Record.size() < 18 || Record.size() > 20)
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1174 ↗(On Diff #61581)

Was this for debugging? Did you mean to leave it in?

aprantl edited edge metadata.Jun 22 2016, 3:27 PM

Is this something that maybe is only needed on function definitions versus declarations?
We're planning to split these up anyway, maybe we can make this field optional after/during the split?

I think this also needs a binary bitcode test that tests autoupgrading from the old format.

rnk added a comment.Jun 22 2016, 4:28 PM

Is this something that maybe is only needed on function definitions versus declarations?
We're planning to split these up anyway, maybe we can make this field optional after/during the split?

Unfortunately, it would need to live on the declaration, because we need it when emitting the class member list.

I think this also needs a binary bitcode test that tests autoupgrading from the old format.

Sure.

rnk updated this revision to Diff 61702.Jun 23 2016, 11:06 AM
rnk marked 2 inline comments as done.
rnk edited edge metadata.
  • Add bitcode binary compat test
  • Fix bugs in bitcode reader
rnk added a comment.Jun 23 2016, 11:29 AM
In D21614#465188, @rnk wrote:

Is this something that maybe is only needed on function definitions versus declarations?
We're planning to split these up anyway, maybe we can make this field optional after/during the split?

Unfortunately, it would need to live on the declaration, because we need it when emitting the class member list.

This is actually like the virtualindex, which only lives on the declaration and not the definition. So, shall we go ahead?

majnemer accepted this revision.Jun 30 2016, 6:51 PM
majnemer added a reviewer: majnemer.
majnemer added a subscriber: majnemer.

LGTM

This revision is now accepted and ready to land.Jun 30 2016, 6:51 PM
This revision was automatically updated to reflect the committed changes.
dexonsmith edited edge metadata.Jun 30 2016, 8:07 PM
dexonsmith added a subscriber: dexonsmith.

You never answered my question from a few weeks ago:

Every 8 bytes counts for a lot since we have so many of these in LTO. If we can find some way to make it optional (via subclassing?) that would be great.

http://reviews.llvm.org/D21614

Files:
include/llvm/IR/DIBuilder.h
include/llvm/IR/DebugInfoMetadata.h
lib/AsmParser/LLParser.cpp
lib/Bitcode/Reader/BitcodeReader.cpp
lib/Bitcode/Writer/BitcodeWriter.cpp
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
lib/IR/AsmWriter.cpp
lib/IR/DIBuilder.cpp
lib/IR/DebugInfoMetadata.cpp
lib/IR/LLVMContextImpl.h
test/DebugInfo/COFF/virtual-methods.ll
unittests/IR/MetadataTest.cpp

<D21614.61581.patch>