Page MenuHomePhabricator

[llvm-dwarfdump] - Teach dwarfdump to decompress debug sections in zlib style.
ClosedPublic

Authored by grimar on May 20 2016, 7:55 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 57931.May 20 2016, 7:55 AM
grimar retitled this revision from to [llvm-dwarfdump] - Teach dwarfdump to decompress debug sections in zlib style..
grimar updated this object.
grimar added reviewers: rafael, davide, dblaikie.
grimar added subscribers: llvm-commits, grimar.

Commit message typo: "depricated" should be "deprecated". Probably also
missing a "we" before [or use "it is" instead of] "are" in the last line.

Mats

dblaikie edited edge metadata.May 20 2016, 9:48 AM

Generally looks good. I'm not sure about renaming the zdebug section to debug in the output, but I'm not set against it either.

lib/DebugInfo/DWARF/DWARFContext.cpp
626 ↗(On Diff #57931)

You could just increment Offset (by sizeof(Elf64_Word)) here, I think?

681–685 ↗(On Diff #57931)

This involves making extra copies of the data. Is there a reason you went with this over the original code that decompressed straight into the UncompressedSections?

grimar updated this revision to Diff 58034.May 21 2016, 2:16 AM
grimar edited edge metadata.
  • Addressed review comments.
lib/DebugInfo/DWARF/DWARFContext.cpp
626 ↗(On Diff #57931)

Right.

681–685 ↗(On Diff #57931)

My point was to avoid possible calls of pop_back right after resize if decompression fails.
That should be rare case (I think decompression fail is infrequent, except cases of disabled zlib (now check for that is inside tryDecompress)), but code itself looks bit overcomplicated.

I just forgot to use std::move() to avoid extras when rewrote. Fixed. There should be no more extra heap allocations here.

grimar added a comment.EditedMay 23 2016, 2:11 AM

Except this defeats/comes at a cost of the small vector optimization - std::move on a small vector in small mode is the same as a copy.

Sorry, perhaps I should've commented the original code to explain/justify it better.

  • David

Small mode for small vector in this place is when it's size less than 32 bytes, right ?
Should we really care of this for the sake of simplicity of code ? I mean that for debug sections 32 bytes
is probably almost never a option, they usually should be greater anyways I think.

Just in case - I plan to revert to initial logic of this soon and update the patch, as it does not affect the main logic
and we can land patch separatelly and return to this place later if we want to.

George.

grimar updated this revision to Diff 58084.May 23 2016, 4:56 AM
  • Restored original way to work with UncompressedSections vector, it is unrelated to this patch.
dblaikie accepted this revision.May 23 2016, 8:17 AM
dblaikie edited edge metadata.

Looks good - thanks!

test/DebugInfo/Inputs/dwarfdump-test-zlib.cc
24 ↗(On Diff #58084)

Might be worth including in the comment some detail about how you verified that the output did actually contain compressed sections (just that the size of the output is smaller, for example) in both cases? (I realize this was missing originally as well, but some paper trail seems helpful to add)

This revision is now accepted and ready to land.May 23 2016, 8:17 AM
This revision was automatically updated to reflect the committed changes.