This is an archive of the discontinued LLVM Phabricator instance.

[CodeView] Implement function-type indices
ClosedPublic

Authored by majnemer on Jun 2 2016, 1:14 AM.

Diff Detail

Event Timeline

majnemer updated this revision to Diff 59347.Jun 2 2016, 1:14 AM
majnemer retitled this revision from to [CodeView] Implement function-type indices.
majnemer added reviewers: rnk, aaboud.
majnemer added a subscriber: llvm-commits.
majnemer updated this revision to Diff 59386.Jun 2 2016, 8:34 AM
  • Small cleanup
rnk edited edge metadata.Jun 2 2016, 8:49 AM

Test? You can probably update types-basic.ll, since it has functions in it already.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
929

FIXME to deal with instance methods

944

FIXME about pushing DW_AT_calling_convention through the metadata

majnemer updated this revision to Diff 59394.Jun 2 2016, 9:02 AM
majnemer edited edge metadata.
  • Address review comments
aaboud edited edge metadata.Jun 2 2016, 9:05 AM

Few comments below.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
484–487
  1. You may use "auto *SP" (like in line 443) instead of "const DISubprogram *".
  2. Can you explain why we might emit debug info for functions that have no DISubprogram attached to it?
485

I thought we wanted to have the index of FuncIdRecord, not the index of ProcedureRecord, right?
Check line 128.

majnemer updated this revision to Diff 59408.Jun 2 2016, 9:46 AM
majnemer marked an inline comment as done.
majnemer edited edge metadata.
  • Address review comments
rnk accepted this revision.Jun 2 2016, 10:14 AM
rnk edited edge metadata.

woot lgtm

This revision is now accepted and ready to land.Jun 2 2016, 10:14 AM
This revision was automatically updated to reflect the committed changes.

LGTM.
Only one comment (you might want to fix it in a follow up commit).

llvm/trunk/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
129 ↗(On Diff #59416)

Should not we return TypeIndex(0) in this case?
Or even better TypeIndex::None(), where:
static TypeIndex None() { return TypeIndex(SimpleTypeKind::None); }