Page MenuHomePhabricator

[DebugInfo] Do not truncate 64-bit values when dumping CIEs and FDEs.
ClosedPublic

Authored by ikudrin on Feb 3 2020, 6:16 AM.

Details

Summary

This fixes printing long values that might reside in CIE and FDE, including offsets, lengths, and addresses.

Diff Detail

Event Timeline

ikudrin created this revision.Feb 3 2020, 6:16 AM

Ping (and to reduce the noise, for all other revisions in the stack as well, please).

aprantl accepted this revision.Feb 10 2020, 4:19 PM
This revision is now accepted and ready to land.Feb 10 2020, 4:19 PM

Mechanically looks fine to me by @probinson should also take a look and see if the concerns in the other review were addressed.

This revision now requires review to proceed.Feb 10 2020, 4:20 PM

Apologies for the slow response. Overall this seems cleaner than D73714, now that I better understand the subtleties between .debug_frame and .eh_frame specifications.

The commit message should include a link to the Linux spec that you cited in the other review, which will help future readers understand the differences between the two formats.

A couple of minor things inline that I would like to see addressed.

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
191

Because this will not return the value from the data, it should have a comment along the lines of:
// Returns the CIE identifier to be used by the requested format.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
342

This looks like you are printing LinkedCIEOffset twice?

ikudrin updated this revision to Diff 244660.Feb 14 2020, 7:16 AM
  • Updated to reflect changes in the parent revisions.
ikudrin marked 2 inline comments as done.Feb 14 2020, 7:26 AM

The commit message should include a link to the Linux spec that you cited in the other review, which will help future readers understand the differences between the two formats.

This revision is mostly about 64-bit values. The links to the specs are added to the comment for getCIEId(), in D73886, which is a parent revision for this one, and to the commit message for D73627.

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
191

I've moved the method (now a function) getCIEId() to D73886. The comment is added there.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
342

Thanks for catching that! It was an issue in our implementation, I've prepared D74613 to fix that.

ikudrin retitled this revision from [DebugInfo] Do not cut 64-bit values when dumping CIEs and FDEs. to [DebugInfo] Do not truncate 64-bit values when dumping CIEs and FDEs..Feb 19 2020, 5:21 PM
ikudrin edited the summary of this revision. (Show Details)
jhenderson added inline comments.Feb 25 2020, 2:42 AM
llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp
20

For completeness, you probably want DWARF32 versions of these too.

22

clang-format-recognised style is /*IsDWARF64=*/true.

Same principle applies throughout.

23

I believe there's no need for the ULL suffix here and elsewhere below.

50

As the TestCIE block is practically identical in all test cases, it might be good to pull it into a function e.g. createCIE(uint64_t Length, uint64_t Offset). This will also make clearer what is actually important to each test case.

ikudrin updated this revision to Diff 246643.Feb 26 2020, 12:56 AM

Thanks, @jhenderson!

  • Simplified tests by moving similar code to helper functions.
  • Added a test for DWARF32 CIE.
  • Removed unneeded suffixes.
  • Removed spaces in the comments for arguments.
ikudrin marked an inline comment as done.Feb 26 2020, 1:08 AM
ikudrin added inline comments.
llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp
22

A side note. It looks like clang-format recognizes both styles, and both are used in the LLVM codebase, but the condensed variant is way more widespread and also is mentioned in the LLVM Coding Standards (not as a requirement, just as an example).

jhenderson accepted this revision.Feb 26 2020, 1:13 AM

I don't claim to have checked all the functionality matches the spec, but otherwise this looks good to me.

This revision is now accepted and ready to land.Feb 26 2020, 1:13 AM

P.S. please remember to mark inline comments as "Done" using the checkbox once you address something. If you do that before uploading a patch, they will get automatically submitted when the patch is uploaded.

llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp
22

When you say "recognises" I think what happens is that it treats the old style as a normal inline comment, so applies the normal comment formatting rules. That is based purely on observation though, so I could be completely wrong!

ikudrin marked 8 inline comments as done.Feb 26 2020, 1:50 AM
ikudrin added inline comments.
llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp
22

Well, maybe it does not need to recognize them in a special way if the result of the formatting is the same as expected. Anyway, I agree that it is better to stick with a more widespread style despite my personal taste.

This revision was automatically updated to reflect the committed changes.
ikudrin marked an inline comment as done.