This is an archive of the discontinued LLVM Phabricator instance.

[CodeView] Isolate the parsing code for various types of CV debug info fragments
ClosedPublic

Authored by zturner on Apr 26 2017, 10:54 AM.

Details

Summary

There are many different types of debug info about modules, such as line information, checksum information, symbol info, etc. in the existing code, we have a single "visitor" class which knows how to parse all the different types, and invokes a callback interface with the results.

Once we begin trying to write this information, this monolithic model becomes difficult to work with, because reading and writing share enough similarities that they should be grouped together whenever possible, but enough differences that you can't always do an exact mapping of the code.

For this reason, it would help if the knowledge of how to parse each type of module debug info fragment were isolated from the others in its own class. This way we could provide useful accessors on these classes for querying fields from the parsed format, or iterating over fields with ranged based fors, or methods to add new items to a list.

In short, the individual fragment types are complicated enough that it makes sense to have their knowledge wrapped up in individual classes designed to be able to query / manipulate / deserialize / serialize them.

In doing so, I found an instance where parsing code was duplicated. Once in llvm-readobj which had its own code for parsing the FileChecksums fragment, and once in llvm-pdbdump where it used the code in DebugInfo/CodeView to do the same thing. When the llvm-readobj code to use the CodeView code (which made the code much simpler), this surfaced a bug in the CodeView implementation where we did not account for alignment, so this has been fixed as well.

Diff Detail

Event Timeline

zturner created this revision.Apr 26 2017, 10:54 AM
amccarth edited edge metadata.Apr 26 2017, 2:37 PM

Looks nice. I noticed just minor stuff.

llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h
1

ModuleDebugFileChecksumFragment.h

30

It seems weird to close the llvm namespace, re-open it, close it again, open it again... Is this an intentional style decision?

36

Should that last parameter be ContextType *Ctx?

llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFragment.h
14

What's Casting.h for in this file?

llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFragmentRecord.h
1

ModuleDebugFragmentRecord.h

53

ContextType *Ctx?

llvm/lib/DebugInfo/CodeView/ModuleDebugLineFragment.cpp
2

Bad wrap.

34

Size, LineInfoSize, and HasColumn could all be const, which might be nice since you can't scope their lifetimes any tighter.

llvm/lib/DebugInfo/CodeView/ModuleDebugUnknownFragment.cpp
11

No newline at end of file

zturner added inline comments.Apr 26 2017, 3:10 PM
llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h
30

This class can't go in the codeview namespace, only the llvm namespace. When multiple levels of namespace nesting are involved, I often find it difficult to follow which one I'm looking at if I partially close and reopen at various points in the file. So I did it this way so that anyone looking at this block of code can immediately figure out the entire namespace hierarchy. In any case, I'm not strongly tied to it, so I can change it.

36

No reason it couldn't be, mostly just that it was a bit more to type.

llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFragment.h
14

Since this entire hierarchy is designed to work with the llvm casting operators, I included it here so that no matter which subclass's header file you include, you always have access to the casting operators. I could leave it up to the individual header files, this is just a little convenience for the users.

amccarth accepted this revision.Apr 26 2017, 3:27 PM

I'm satisfied, but you might want to wait to hear from at least one other reviewer.

llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h
30

I'm fine with it. I just noticed that at least one other file in this patch doesn't do this.

36

I was just going for consistency (see LineColumnExtractor), and because it makes the point of the ContextType typedef obvious.

This revision is now accepted and ready to land.Apr 26 2017, 3:27 PM
rnk accepted this revision.Apr 26 2017, 4:39 PM

I'm satisfied, but you might want to wait to hear from at least one other reviewer.

Looks good, at a really really high level. I'd say when you get to the writing portion, try to get more feedback from Rui and Bob, since it's useful to have a linker perspective for that.

This revision was automatically updated to reflect the committed changes.