This is an archive of the discontinued LLVM Phabricator instance.

LLVM CodeView library
ClosedPublic

Authored by DaveBartolomeo on Nov 24 2015, 10:52 AM.

Details

Summary

This diff is the initial implementation of the LLVM CodeView library. There is much more work to be done, namely a CodeView dumper and tests. This patch should help others make progress on the LLVM->CodeView debug info emission while I continue with the implementation of the dumper and tests.

This library implements support for emitting debug info in the CodeView format. This phase of the implementation 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. In the future, other classes derived from TypeTableBuilder will write to other destinations, such as the type stream in a PDB.

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 library provides a relatively low-level interface based on the actual on-disk format of CodeView. For example, type records refer to other type records by TypeIndex, rather than by an actual pointer to the referent record. This allows clients to emit type records one at a time, rather than having to keep the entire transitive closure of type records in memory until everything has been emitted. At some point, having a higher-level interface layered on top of this one may be useful for debuggers and other tools that want a more holistic view of the debug info. The lower-level interface should be sufficient for compilers and linkers to do the debug info manipulation that they need to do efficiently.

Diff Detail

Event Timeline

DaveBartolomeo retitled this revision from to LLVM CodeView library.
DaveBartolomeo updated this object.
DaveBartolomeo added a reviewer: majnemer.
DaveBartolomeo added a subscriber: llvm-commits.
rnk added a subscriber: rnk.Dec 2 2015, 1:50 PM
rnk added inline comments.
include/llvm/CodeView/CodeView.h
1 ↗(On Diff #41063)

Each file should have the standard LLVM header that references the license: http://llvm.org/docs/CodingStandards.html#file-headers

9 ↗(On Diff #41063)

I notice our existing PDB library, which wraps DIA, duplicates these constants. I'll take responsibility for going back and making them use this.

include/llvm/CodeView/TypeRecordBuilder.h
6–7 ↗(On Diff #41063)

You probably don't want to use angle brackets to bring in non-system headers.

lib/CodeView/TypeRecordBuilder.cpp
15 ↗(On Diff #41063)

This will not work on big endian machines. LLVM has an endian writer in include/llvm/Support/EndianWriter.h that will wrap a raw_ostream, and provide an API like this:

endian::Writer<endian::little> ES(Stream);
ES.write<uint32_t>(Value);
rnk added a comment.Dec 2 2015, 2:41 PM

I also think we should sink this into DebugInfo. Moving it into that directory won't add any extra implicit dependencies or anything.

I've moved the CodeView library under DebugInfo, and updated makefiles, #includes, and include guards to reflect the new location.
I've fixed the TypeRecordBuilder class to be host-endianness-agnostic by using endian::Writer<endianness::little>.
I've added standard LLVM header comments to all .h and .cpp files.

silvas added a subscriber: silvas.Dec 14 2015, 5:43 PM

A couple style nits.

include/llvm/DebugInfo/CodeView/CodeView.h
123

In LLVM we usually don't use throw(). Unless there is a specific reason you're using it (in which case please put a prominent comment somewhere explaining the reason), please remove it.

lib/DebugInfo/CodeView/CodeViewOStream.cpp
11 ↗(On Diff #42805)

This file also seems unneeded.

lib/DebugInfo/CodeView/FunctionId.cpp
13 ↗(On Diff #42805)

What is the point of this file? Can you just delete it for now?

lib/DebugInfo/CodeView/ListRecordBuilder.cpp
29

Is this TODO related to the assertion?

lib/DebugInfo/CodeView/MemoryTypeTableBuilder.cpp
14

In LLVM, we typically put a using namespace llvm (and any other namespaces) at the top of the .cpp file instead of wrapping the whole file in namespaces.

lib/DebugInfo/CodeView/TypeIndex.cpp
11 ↗(On Diff #42805)

This file also seem to be unneeded.

lib/DebugInfo/CodeView/TypeTableBuilder.cpp
196

Pull Record.getSlots() out into a local variable. That will shorten/clarify a lot of this code.

211

Fixed mostly stylistic feedback from Sean Silva.
Removed a few .cpp files that are empty because all of the relevant member functions were defined inline in the header.
Made comments into complete sentences.
Removed accidental use of throw().

DaveBartolomeo marked 6 inline comments as done.Dec 17 2015, 4:47 PM
DaveBartolomeo added inline comments.
lib/DebugInfo/CodeView/ListRecordBuilder.cpp
29

It is. I've updated the comment to be more descriptive.

rnk accepted this revision.Dec 17 2015, 5:17 PM
rnk added a reviewer: rnk.

I think we should go ahead and commit this and iterate on the outstanding issues.

Things I'd like to do in the near future:

  • Doxygen comments
  • Tests
  • An in tree user that actually generates type info :)
  • Re-layer the lib/DebugInfo/PDB library on CodeView. Currently they have significant duplication.
This revision is now accepted and ready to land.Dec 17 2015, 5:17 PM
Closed by commit rL256385: LLVM CodeView library (authored by dbartol). · Explain WhyDec 24 2015, 10:16 AM
This revision was automatically updated to reflect the committed changes.

Hi Dave,

This looks pretty good, I appreciate all the work. Couple of questions/comments:

a) No tests? :(
b) Do all of the headers need to be in llvm/include rather than in llvm/lib? I have no real complaints or objection, just curious if they're going to be used outside or if it was just a "while I was there" sort of change.

Thanks!

-eric