Page MenuHomePhabricator

Changing representation of .cv_def_range directives in Codeview debug info assembly format for better readability
AcceptedPublic

Authored by nilanjana_basu on Jul 23 2019, 6:21 PM.

Details

Reviewers
rnk
Summary

.cv_def_range information for symbol records in codeview debug info used to be represented as a serialized series of bytes in assembly format. This change will display the def_range info in human-readable format.

Diff Detail

Repository
rL LLVM

Event Timeline

nilanjana_basu created this revision.Jul 23 2019, 6:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2019, 6:21 PM
rnk added inline comments.Jul 30 2019, 11:53 AM
llvm/include/llvm/MC/MCStreamer.h
38

Please avoid using namespace in header files, since it will affect everyone who includes the header.

llvm/lib/MC/MCAsmStreamer.cpp
1398

I suppose this can be factored out as well.

1447–1453

Please factor this repeated code into a helper method.

1455

Ideally, registers should be printed textually as rbp, ebp, etc. Given time constraints, I think we can leave that as a future improvement, though.

llvm/lib/MC/MCParser/AsmParser.cpp
3882

I don't see a case for this enum... does it still work? I have to run to lunch, but I want to send these review comments now rather than later.

nilanjana_basu marked 5 inline comments as done.Jul 30 2019, 6:10 PM
nilanjana_basu added inline comments.
llvm/lib/MC/MCAsmStreamer.cpp
1455

Will try to do this in the next patch before I leave.

llvm/lib/MC/MCParser/AsmParser.cpp
3882

This is a placeholder enum, like DK_NO_DIRECTIVE in DirectiveKind. If the defrange type is not found, it is set to this value, & its executed in the default case. I have only implemented the 4 types that are implemented in CodeViewDebug.cpp.

nilanjana_basu marked 2 inline comments as done.
  1. Removed use of namespace from MCStreamer.h
  2. Added helper function for factoring repeated code for printing def_range prefix
rnk added inline comments.Jul 30 2019, 7:35 PM
llvm/lib/MC/MCParser/AsmParser.cpp
3882

I saw that you left the asm printer method that prints the binary string version of .cv_defrange, so I thought maybe you wanted to keep support for that and handle it that way here. We can either completely remove support for the string variant of the directive, or add support for it here. Either way is fine.

3890

I realize you should write tests for the error handling paths. You can add a cv-defrange-errors.s test file that exercises each of these corner cases, and then FileCheck for the errors.

nilanjana_basu marked 4 inline comments as done.
  1. Removed the functionality for the unimplemented (in CodeView) flag CVDR_DEFRANGE in the MCAsmStreamer.
  2. Added new negative test cases for all errors reported for the current change
  3. Fixed test cases involving CodeView def_range in MC/COFF
rnk accepted this revision.Aug 2 2019, 11:24 AM

lgtm

This revision is now accepted and ready to land.Aug 2 2019, 11:24 AM
  1. Changed two more test files for lld & lldb that used def_range directives
  2. Fixed the endianness bug that arose because of changing the structure format of frame pointer relative def range structure.