Page MenuHomePhabricator

[DebugInfo] Support 64-bit DWARF for .debug_names.
ClosedPublic

Authored by ikudrin on Jan 16 2020, 11:38 PM.

Diff Detail

Event Timeline

ikudrin created this revision.Jan 16 2020, 11:38 PM

Some parts of this could be extracted into a separate NFC (or mostly NFC) patch:

  • The Dwarf.h part
  • In DWARFAcceleratorTable.h, merging the Header and HeaderPOD structs, and removing the Padding field. (Adding the Format field would still be part of the functional patch.)
  • In DWARFAcceleratorTable.cpp, eliminating the Padding field, and introducing CommonHeaderSize in DWARFDebugNames::Header::extract()

That would make the rest of the patch a little more focused. (Which overall looks pretty reasonable, although I'd prefer to wait for the next round before approving.)

llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
380

Could this be constexpr ?

625

Is this line too long? (Did you run clang-format-diff?)

ikudrin updated this revision to Diff 239102.Jan 20 2020, 6:00 AM
  • Extract accompanying fixes into separate patches.
  • Make expressions constexpr where possible.
ikudrin marked 3 inline comments as done.Jan 20 2020, 6:04 AM
ikudrin added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
380

Done. Thanks!

625

It is exactly 80 characters long. And yes, it is clang-format which made this formatting.

ikudrin updated this revision to Diff 239805.Jan 23 2020, 1:13 AM
ikudrin marked an inline comment as done.
  • Rebase, update.

DWARF64HeaderFixedPartSize cannot be constexpr because dwarf::getUnitLengthFieldByteSize() connot be constexpr because not all compilers allow llvm_unreachanble() to be used in a constexpr function.

dblaikie added inline comments.Jan 25 2020, 12:06 AM
llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
403–408

Perhaps this could be refactored to common the length test?

uint64_t StartingOffset = *Offset);
UnitLength = ...;
Format = UnitLength == dwarf::DW_LENGTH_DWARF64 ? dwarf::DWARF64 : dwarf::DWARF32;
if (!AS.isValidOffsetForDataOfSize(StartingOffset, dwarf::getUnitLengthFieldByteSize(Format) + CommonHeaderSize))
  return createStringError(...);
if (Format == dwarf::DWARF64) {
  UnitLength = AS.getU64(Offset);
} else if (...) {
  return createStringError(...);
}

I'm not super convinced it's so much better - but it does avoid the quirky "- 4" and such.

Alternatively we could skip all this and use the Error second parameter to the DataExtractor functions, and handle the error after the read attempts? More like the way DWARFUnitHeader::extract works, for example.

ikudrin updated this revision to Diff 240532.Jan 27 2020, 5:15 AM
  • Avoid the quirky "- 4" by using StartingOffset.

@dblaikie, what do you think about this variant?

ikudrin marked an inline comment as done.Jan 27 2020, 5:26 AM
ikudrin added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
403–408

I like the idea to check the remaining length before reading more than using the Error parameter. That makes the code more straightforward and allows producing more expressive error messages. So, for my taste, I would better rework DWARFUnitHeader::extract().

ikudrin updated this revision to Diff 241100.Jan 29 2020, 3:49 AM
  • Make the checking for the remaining length closer to what @dblaikie suggested.

David, which variant looks better to you?

dblaikie accepted this revision.Jan 30 2020, 3:52 PM

Close enough. Whole thing'll hopefully be revisited/refactored with a constrained DWARFDataExtractor at some point soon (@labath - FYI)

This revision is now accepted and ready to land.Jan 30 2020, 3:52 PM
This revision was automatically updated to reflect the committed changes.