This is an archive of the discontinued LLVM Phabricator instance.

[codeview] Add a type visitor to help abstract away type stream handling
ClosedPublic

Authored by rnk on May 3 2016, 7:46 PM.

Details

Summary

Port the dumper in llvm-readobj over to it.

I'm planning to use this visitor to power type stream merging.

While we're at it, try to switch from StringRef to ArrayRef<uint8_t> in some
places.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 56088.May 3 2016, 7:46 PM
rnk retitled this revision from to [codeview] Add a type visitor to help abstract away type stream handling.
rnk updated this object.
rnk added reviewers: zturner, amccarth.
rnk added a subscriber: llvm-commits.
pzread added a subscriber: pzread.May 3 2016, 9:09 PM
pzread removed a subscriber: pzread.
amccarth added inline comments.May 4 2016, 8:57 AM
include/llvm/DebugInfo/CodeView/CVTypeVisitor.h
39 ↗(On Diff #56088)

s/does nothing/they do nothing/

40 ↗(On Diff #56088)

delete comma

58 ↗(On Diff #56088)

I would avoid these repeated casts by doing it once and saving the result:

auto DerivedThis = static_cast<Derived *>(this);
66 ↗(On Diff #56088)

I'd put the default either first or last.

70 ↗(On Diff #56088)

I realize the mechanical style rules call for this level of indentation, but it seems the intent would be clearer if this were indented to match the other cases.

78 ↗(On Diff #56088)
#undef TYPE_RECORD

Oh, but that might mess up the other visitor loops below. Yuck. This macro looks tightly scoped, but it really isn't.

83 ↗(On Diff #56088)

Is there any harm in just calling skipPadding here? We do that already for field lists.

Also, this comment is copied from the iterator. It seems like it should be in only one place, and I think it makes most sense in the iterator code.

107 ↗(On Diff #56088)

I'm not clear what you mean by "By default." This looks like it always visits the members one by one. Do you mean that the derived class may override this method to get other behavior?

129 ↗(On Diff #56088)
#undef MEMBER_RECORD
137 ↗(On Diff #56088)

For the benefit of people making a derived class, I'd make this comment emphasize that processing will stop after the first unknown member. "Action to take on unknown members" suggests that this will be called for each unknown member.

tools/llvm-readobj/COFFDumper.cpp
1284 ↗(On Diff #56088)

Typo: remove the t in the midst of decodeNumerictLeaf.

1999 ↗(On Diff #56088)

It makes me sad that this formatting detail is back out at this level.

zturner added inline comments.May 4 2016, 10:26 AM
include/llvm/DebugInfo/CodeView/CVTypeVisitor.h
30–36 ↗(On Diff #56088)

In the future we could get rid of this entirely and use the StreamReader interface in DebugInfoPDB. It wouldn't work right now and some refactoring would have to happen, but I think the design of it is flexible enough to support this in theory.

69 ↗(On Diff #56088)

How about moving this #define before the switch statement so it doesn't mess up the indentation?

include/llvm/DebugInfo/CodeView/TypeRecords.def
20–22 ↗(On Diff #56088)

Why do we need these _ALIAS macros? They seem to do nothing, why not just use TYPE_RECORD() everywhere?

rnk marked 4 inline comments as done.May 4 2016, 11:42 AM

Thanks! New patch coming

include/llvm/DebugInfo/CodeView/CVTypeVisitor.h
30–36 ↗(On Diff #56088)

I agree, we need something that lets us read records from a possibly non-contiguous stream like those in PDBs.

66 ↗(On Diff #56088)

done, I didn't like it after the macro goo

78 ↗(On Diff #56088)

It is tightly scoped, it's undef'd by the .def file. It's confusing, but it is the standard pattern you see in places like include/llvm/IR/Metadata.def.

83 ↗(On Diff #56088)

Oops, yeah that's the responsibility of the iterator.

107 ↗(On Diff #56088)

That was the idea, but I don't think there's a real use case for it. I should rewrite this comment to remove that implication.

129 ↗(On Diff #56088)

not needed

include/llvm/DebugInfo/CodeView/TypeRecords.def
20–22 ↗(On Diff #56088)

I do this with them when stamping out declarations of the visit##ClassType methods:

#define TYPE_RECORD_ALIAS(ClassName, LeafEnum)

Otherwise we end up re-declaring things like void visitBaseClass(... const BaseClass *Rec, ...) multiple times.

The alternative to this might be variadic macros, which would let us emit cases like this, but I felt like that was too much metaprogramming:

case LF_BCLASS:
case LF_IBCLASS: { ... }
tools/llvm-readobj/COFFDumper.cpp
1284 ↗(On Diff #56088)

That's a pre-existing issue. I thought maybe David Majnemer did it intentionally, but I'm not sure. I'll ask him and fix this separately, either way.

1999 ↗(On Diff #56088)

Well, the magic isn't part of the stream in the PDB, so we were going to need to do this eventually.

I should use the consumeUInt32 helper though...

rnk updated this revision to Diff 56181.May 4 2016, 11:45 AM
  • Address review comments
amccarth accepted this revision.May 4 2016, 11:54 AM
amccarth edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 4 2016, 11:54 AM
This revision was automatically updated to reflect the committed changes.