Page MenuHomePhabricator

CodeView type info support preview (LLVM portion)
Needs ReviewPublic

Authored by DaveBartolomeo on Oct 30 2015, 11:09 AM.

Details

Reviewers
majnemer
Summary

This diff is a preview of the implementation of the LLVM portion of the CodeView RFC being discussed on the llvm-dev and cfe-dev lists. The code isn't ready for detailed code review yet. There will likely be some changes to the design based on feedback, plus there's still a bit of code clean-up to do. Once the design is settled and I've got the code finalized, I'll break it up into more easily-digestible patches for official code review. For now, though, this diff should be useful to anyone who has read the RFC and wants to see some concrete details.

The new LLVMCodeView library under lib\CodeView and the corresponding include directory implements support for emitting debug info in the CodeView format. This phase of the design only includes support for CodeView type records. Clients that need to emit type records will use a class derived from TypeTableBuilder. TypeTableBuilder provides member functions for writing each kind of type record; each of these functions eventually calls the writeRecord virtual function to emit the actual bits of the record. Derived classes override writeRecord to implement the folding of duplicate records and the actual emission to the appropriate destination. LLVMCodeView provides MemoryTypeTableBuilder, which creates the table in memory. When Clang emits type records, it uses MDTypeTableBuilder, which emits to LLVM metadata.
The rest of the types in LLVMCodeView define the actual CodeView type records and all of the supporting enums and other types used in the type records. The TypeIndex class is of particular interest, because it is used by clients as a handle to a type in the type table.

The remaining changes are mostly in lib\IR, where the debug info metadata, DIBuilder, and the readers and writers thereof needed to be updated to support CodeView. I added the new DICodeViewTypes and DICodeViewUDT nodes, added a DICodeViewTypes field to DICompileUnit, and added TypeIndex fields to DISubprogram and DIVariable.

Diff Detail

Event Timeline

DaveBartolomeo retitled this revision from to CodeView type info support preview (LLVM portion).
DaveBartolomeo updated this object.
DaveBartolomeo added a reviewer: majnemer.
DaveBartolomeo added a subscriber: llvm-commits.
aaboud added a subscriber: aaboud.Nov 1 2015, 3:43 AM

I'm sure there will be many suggestions from other people as well, but from my perspective, it would be a good idea to separate out LLVMCodeView into a different commit. It doesn't depend on anything else here. Rather, the rest of the stuff here depends on it. So it could go in separately with its own set of tests (speaking of which, it doesn't appear to have any tests).

aaboud added a comment.Nov 9 2015, 8:17 AM

Hi Dave,
I understand that this patch is an initial one.
However, I have some concerns that might be related to the design more than to the code itself.

Please see below.

Thanks,
Amjad

include/llvm/CodeView/TypeIndex.h
156

What would be the usage of these operators?
Is there a need to sort the TypeIndex in list?
Does it make sense to have an order between basic types, e.g. int vs. float?

include/llvm/CodeView/TypeRecord.h
13

It might be a good idea that all records: type, symbol, id, etc. be derived from a single base object.
For example: CodeViewRecord.

49

I do not see the connection between TypeIndex and TypeRecord, and I am not sure that it is a good idea that the TypeRecord derived classes requires TypeIndex class as a reference to other TypeRecord.

In my opinion, it would be better to:

  1. have a TypeIndex member in TypeRecord.
  2. change TypeRecord derived classes to require TypeRecord pointer instead of TypeIndex.
lib/IR/DebugInfoMetadata.cpp
313

It have been already said, but I want to second it.
I do not think that it is a good idea to have CodeView debug info entries in the metadata.
I believe that the current DINodes are sufficient to create CodeView debug info.
And if there are few things that need to be extended, let's do it.

For consistence reason, I suggest to place the CodeView files in include/llvm/DebugInfo/CodeView & lib/DebugInfo/CodeView

Hi All,

I appreciate the code drop with the general direction you've been going. It'll help iron out a lot of the direction here.

A way to start here will probably to get a lot of the constants and definitions in by writing a codeview dumping system similar to llvm-dwarfdump. That way as the rest of code view support is added we'll be able to get testcases added in a much better way.

A few questions in general:

Why are codeview types split out as a separate top level entity? It seems this is to support the front end writing out types completely, but a few questions (admittedly without reading the spec, I apologize in advance):

Do types in code view not need to be able to reference things outside of themselves? i.e. a method function referencing a concrete instance.
Do types in code view not need to have parts of them referenced from outside? i.e. method functions

We've had some ideas (and gone down a bit) of the path of having dwarf largely emitted by the front end but the need for some of these references makes a huge difference. Is there something about the format that causes this to be unnecessary here?

Some nits (I know you said it wasn't ready for review, but I'll get some of them early):

a) Formatting. Please use clang-format on all new code.
b) I'm not a fan of having to have codeview::TypeIndex() everywhere. Could we refactor this in a different way? I'm trying to keep the internal representation as format neutral as possible and think we're a lot of the way there. If it helps as part of this we can take the formats and define custom "tags" for the metadata that work for each format.

Anyhow, some thoughts while you're working through things.

-eric