This is an archive of the discontinued LLVM Phabricator instance.

Move CodeViewTypeStream and iterators to DebugInfo/CodeView
ClosedPublic

Authored by zturner on May 3 2016, 12:58 PM.

Details

Summary

These classes should be reusable outside of the CV dumper, so moving them into the library.

Diff Detail

Event Timeline

zturner updated this revision to Diff 56052.May 3 2016, 12:58 PM
zturner retitled this revision from to Move CodeViewTypeStream and iterators to DebugInfo/CodeView.
zturner updated this object.
zturner added reviewers: rnk, amccarth.
zturner added a subscriber: llvm-commits.

I also changed the iterator to work with ranged based for syntax as well.

dblaikie added inline comments.
include/llvm/DebugInfo/CodeView/TypeStream.h
34

static->inline as this code is in a header (otherwise you get ODR violations, code duplication, etc) - same goes for any other non-member static things here (consumeObject above)

123–129

'inline' now that these are in a header, otherwise you'll get duplicate symbols when two object files using that include this header are linked together.

Moving these definitions into their friend declarations would make them implicitly inline and they're quite small anyway, so that's probably not a bad idea.

131

I'm not sure about this type, with regard to our previous discussions on "where to put the magic" and how to fail the ctor.

If this type is just for range-for convenience, should probably just use llvm::iterator_range to put the two TypeIterators together.

But if we want to use it to make the magic access a bit more natural/remove the extra memory footprint from the iterators, then I'd expect to see the knowledge of magic removed from the iterator and more of the initialization work to be done in TypeStream's ctor instead, which seems OK. Then maybe the iterators would be a private implementation detail of TypeStream, or at least have some private API that (via friendship) only TypeStream can use (for construction, etc).

(still missing the error handling here - I'd imagine TypeStream would have a named ctor that returns ErrorOr<TypeStream> - not sure about errors during iteration (beyond the initial construction). That gets trickier)

133–136

Prefer to use the initializer list to initialize members (& you shouldn't need to initialize 'End' at all in that case, since it's default constructed)

141

Difficult question about style: while begin/end need to be spelled that way, magic here could match the LLVM coding conventions and be called 'getMagic' or similar.

amccarth edited edge metadata.May 3 2016, 1:24 PM

I agree with dblaikie's suggestions, but I don't see any issues beyond those.

Regarding the magic, I honestly don't even think this class should handle the magic. It should be handled at a higher level. The type stream in a PDB file doesn't have this magic anyway, it's just a list of records.

This class started out as the higher-level thing, and it grew into an
iterator (and the input got lower level: StringRef instead of SectionRef).

zturner updated this revision to Diff 56064.May 3 2016, 2:43 PM
zturner edited edge metadata.

Various changes as suggested by previous reviews.

The TypeStream is no longer its own class, you can just create an iterator pair. The magic is not part of a PDB's type stream, and it seems to be specific to parsing a .debug$T section of an object file, so that logic is extracted into the dumper while the codeview library assumes that the first byte of the stream is the first byte of the first type record. Updated DebugInfoPDB to use this new iterator logic from DebugInfoCodeview rather than hand parsing the codeview type stream.

I'm not thrilled with the backsliding with respect to the parsing of the magic number. The magic number is part of the format of the type stream, and the original intent was to abstract the parsing of the typestream so that multiple users didn't have to know about the details. Now we're exposing this detail again and making the callers know that they have to pass a subset of the typestream to construct the iterators.

Consider what happens in the future if there's a newer typestream format, and the indicator is in the magic number field. This would completely break the abstraction. The createCodeViewTypeStream function (which you removed) would probably have been a better place to handle the magic number).

That said, I won't object because I know this is still a work in progress, so I'm confident we'll find the right level of abstraction eventually.

I'm not thrilled with the backsliding with respect to the parsing of the magic number. The magic number is part of the format of the type stream, and the original intent was to abstract the parsing of the typestream so that multiple users didn't have to know about the details. Now we're exposing this detail again and making the callers know that they have to pass a subset of the typestream to construct the iterators.

Consider what happens in the future if there's a newer typestream format, and the indicator is in the magic number field. This would completely break the abstraction. The createCodeViewTypeStream function (which you removed) would probably have been a better place to handle the magic number).

That said, I won't object because I know this is still a work in progress, so I'm confident we'll find the right level of abstraction eventually.

Well I guess what I'm finding is that the magic number is not part of the format of a codeview type stream, but rather part of the format of a .debug$T section of an object file. I'm open to putting it somewhere else, but I think we need some kind of abstraction that simply iterates a raw sequence of types, since that's the lowest common denominator that all type stream containers hold.

amccarth accepted this revision.May 3 2016, 2:59 PM
amccarth edited edge metadata.
This revision is now accepted and ready to land.May 3 2016, 2:59 PM

BTW, I found out that the magic is actually equal to COFF::DEBUG_SECTION_MAGIC, and printCodeViewSymbolSection has the same check inside of it. So I added a safety check to the code view type dumper that errors out if the magic is not equal to COFF::DEBUG_SECTION_MAGIC.

zturner closed this revision.May 3 2016, 4:19 PM