Page MenuHomePhabricator

Improving CodeView debug info type record's inline comments
AcceptedPublic

Authored by nilanjana_basu on Jul 29 2019, 6:46 PM.

Details

Reviewers
rnk
Summary

Improved inline comments for the following:

  1. Printing names of types (based on type index)
  2. Printing all enum names (e.g. for Calling convention, access specifier, method kinds etc)
  3. Printing all enabled flag names (e.g. enabled flags for Function options, modifier names, properties, attributes etc )

Diff Detail

Repository
rL LLVM

Event Timeline

nilanjana_basu created this revision.Jul 29 2019, 6:46 PM

Ran git clang-format

  1. Added an extra test case for MethodList.
  2. Added a more human-readable version of record & member kinds, in addition to their enum names
rnk added inline comments.Jul 30 2019, 11:44 AM
llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp
30

Please move these tables to EnumTables.cpp, add getFooNames methods for them to EnumTables.h, and use them here. Otherwise these tables will be duplicated in the final binary.

210

I think most members are very simple, and it would be better to keep the comment on a single line if possible. Something like:
.long ... # Attrs: Public, Virtual, Sealed
.long ... # Attrs: Private, Vanilla
.long ... # Attrs: Public, Vanilla, CompilerGenerated

It's not as explicit as the old comment format, but I think it's better to be more compact.

337

Please avoid computing these expensive strings when type record mapping is not being used in streaming mode. Formatting strings can be quite expensive, but serializing the records should be as fast as possible. When we aren't streaming, the comment can be an empty string or just "Attributes" or "Attrs".

370

I'm also a bit concerned that the vast majority of pointers don't set any of these flags. It would be nice to only mention the flags that are set. You can also look at the code in llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp, which emits flags like that.

nilanjana_basu marked 4 inline comments as done.

Changes made according to review feedback:

  1. Moved EnumTables to EnumTables.cpp
  2. Made attributes & properties print on the same line, in a more compact form (print only the ones that are enabled, & on the same line. Flags are "|" separated, & enums are comma-separated)
  3. Restricted string formatting related computation to Streamer mode

Question: Have been updating the test files manually, so that the difference is clearly visible in the diff. However, because of this it does not take into account a lot of other changes in output, that also should be verified. Do you recommend updating the test files by a direct copy of the ASM?

Updated 2 test cases, to match the exact type record directive & comment output, as is emitted with the current code

rnk accepted this revision.Aug 1 2019, 10:49 AM

lgtm, thanks! I think that's everything we need to remove the type dump comment block.

This revision is now accepted and ready to land.Aug 1 2019, 10:49 AM

Fixed address-sanitizer reported issue of using Stringref to return references to stack memory, which were used later when they were no longer valid.

rnk added a comment.Tue, Aug 20, 4:40 PM

The code feels very std::string-y now. :) I see two places where StringRef should work and would be more consistent with the rest of LLVM, but other than that I would leave it as you have it.

llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp
27

I don't think this needs to return std::string, it only return string literals, which live forever.

82

I don't think this needs to return std::string, since it only ever returns constant strings from tables or the empty string.

nilanjana_basu marked 2 inline comments as done.Tue, Aug 20, 6:10 PM
rnk accepted this revision.Wed, Aug 21, 3:19 PM

I see you marked the comments "done", but I don't see a new version of the patch. I'm happy to assume that you made those edits and to approve this to be committed. Ultimately, I have to trust that you commit the same code that is uploaded anyway. :)

Changed getLeafTypeName() & getEnumName() return types to StringRef, as per the reviewer comments.