This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Add calling convention support for DWARF and CodeView
ClosedPublic

Authored by rnk on Jun 7 2016, 6:33 PM.

Details

Summary

Now DISubroutineType has a 'cc' field which should be a DW_CC_ enum. If
it is present and non-zero, the backend will emit it as a
DW_AT_calling_convention attribute. On the CodeView side, we translate
it to the appropriate enum for the LF_PROCEDURE record.

I added a new LLVM vendor specific enum to the list of DWARF calling
conventions. DWARF does not appear to attempt to standardize these, so I
assume it's OK to do this until we coordinate with GCC on how to emit
vectorcall convention functions.

Diff Detail

Event Timeline

rnk updated this revision to Diff 59983.Jun 7 2016, 6:33 PM
rnk retitled this revision from to [DebugInfo] Add calling convention support for DWARF and CodeView.
rnk updated this object.
rnk added reviewers: dexonsmith, majnemer, aaboud, amccarth.
rnk added a subscriber: llvm-commits.
rnk updated this revision to Diff 59984.Jun 7 2016, 6:36 PM
  • Fix reading old bitcode
majnemer added inline comments.Jun 8 2016, 10:12 AM
include/llvm/IR/DebugInfoMetadata.h
959

DebugInfoMetadata.h includes Dwarf.h, can we make this return an enum CallingConvention?

include/llvm/Support/Dwarf.h
390

Can we give this an underlying type of uint8_t?

rnk added inline comments.Jun 8 2016, 10:29 AM
include/llvm/IR/DebugInfoMetadata.h
959

I considered it, but that would not be consistent with the other methods for getting the DW_LANG_, etc. I thought maybe there was a reason for that. Also, we can narrow this down to a byte all around whether we go with the enum or not.

aaboud edited edge metadata.Jun 8 2016, 11:20 AM

In general, direction looks good to me.
I add some minor comments below.

lib/Bitcode/Reader/BitcodeReader.cpp
2412

Consider emitting the CC as last field in the bitcode, i.e. Record[3].
Notice that the order in bitcode and text does not have to be the same.

lib/Bitcode/Writer/BitcodeWriter.cpp
1469

Consider emitting CC as last field (see previous comment).

lib/Support/Dwarf.cpp
392

Notice that we had the DW_CC_lo_user and SW_CC_hi_user before, and now we do not.
Is that intentional?

test/DebugInfo/X86/DW_AT_calling-convention.ll
2

Do you have the original source of this test?
Can you add it as a comment into the LIT test?

unittests/IR/MetadataTest.cpp
75

How about adding a test case that checks setting CallingConvention correctly?

rnk marked 4 inline comments as done.Jun 8 2016, 11:43 AM
rnk added inline comments.
lib/Bitcode/Reader/BitcodeReader.cpp
2412

Yeah, that's better.

lib/Support/Dwarf.cpp
392

I don't think it makes sense to stringify calling convention 0x40 as DW_CC_lo_user, given that GCC uses it for a real value:
https://github.com/gcc-mirror/gcc/blob/8b066242809973ce7ae3d24712f430a9a2bd786a/include/dwarf2.h#L181

test/DebugInfo/X86/DW_AT_calling-convention.ll
2

Yeah, it's the same as the codeview case.

rnk updated this revision to Diff 60074.Jun 8 2016, 11:45 AM
rnk marked 2 inline comments as done.
rnk edited edge metadata.
  • Address review comments
aaboud accepted this revision.Jun 8 2016, 11:57 AM
aaboud edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 8 2016, 11:57 AM
amccarth added inline comments.Jun 8 2016, 1:17 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
923

Can you comment on why only some of these are mapped?

rnk updated this revision to Diff 60096.Jun 8 2016, 1:26 PM
rnk edited edge metadata.
  • Add comments and move to uint8_t
This revision was automatically updated to reflect the committed changes.