This is an archive of the discontinued LLVM Phabricator instance.

[lib/Object] - Introduce Decompressor class.
ClosedPublic

Authored by grimar on Dec 25 2016, 6:12 AM.

Details

Summary

Decompressor intention is to reduce duplication of code.
Currently LLD has own implementation of decompressor
for compressed debug sections.

This class helps to avoid it and share the code.
LLD patch for reusing it is D28106

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 82471.Dec 25 2016, 6:12 AM
grimar retitled this revision from to [DWARF] - Introduce DWARFCompression class..
grimar updated this object.
grimar added reviewers: rafael, davide, dblaikie.
grimar added subscribers: llvm-commits, grimar, evgeny777.
davide edited edge metadata.Dec 25 2016, 8:58 AM

Overall decent, first round of comments inline.

include/llvm/DebugInfo/DWARF/DWARFCompression.h
37 ↗(On Diff #82471)

Use an uniform style for comments. Also, doxygen.

40 ↗(On Diff #82471)

Returns what? Probably you mean returns true?

lib/DebugInfo/DWARF/DWARFContext.cpp
608 ↗(On Diff #82471)

decompress instead of uncompress maybe?

614 ↗(On Diff #82471)

this deserves a comment or needs to be hidden behind and API

dblaikie added inline comments.Dec 26 2016, 1:53 PM
include/llvm/DebugInfo/DWARF/DWARFCompression.h
21 ↗(On Diff #82471)

Slightly odd/non-descript name (DWARFDecompressor? it seems specifically for decompressing)

31–32 ↗(On Diff #82471)

Could uncompress return Error instead of bool, and then the error message could be included there?

34–38 ↗(On Diff #82471)

As an aside/follow-up: once we have this abstraction, it might be worth considering adding a streaming API (eg: pass in a lambda that gets called with uncompressed bytes repeatedly until done). That way linkers (& dwp) could stream uncompressed bytes out and write them out to the output file (especially since they can know the uncompressed size from the header without actually uncompressing the bytes - so they can allocate the necessary size in the output (even update at least some of the gdb-index tables without inspecting the contents of the comperssed bytes too) and then just wait to decompress, in parallel, while streaming/writing the output)

43–44 ↗(On Diff #82471)

strange to have a mutable public member - should this be an accessor?

David, Davide, thanks for review !
I'll address comments for this in early 2017 (have a vacation + holidays here).

aprantl added inline comments.Jan 3 2017, 10:33 AM
include/llvm/DebugInfo/DWARF/DWARFCompression.h
23 ↗(On Diff #82471)

Please comment the parameters here.

29 ↗(On Diff #82471)

MutableArrayRef?

ruiu added a subscriber: ruiu.Jan 3 2017, 3:51 PM

I don't think DWARFCompression is a good name because you can compress not only debug sections but any types of sections. Likewise, it shouldn't be in lib/DebugInfo directory.

davide added a comment.Jan 4 2017, 4:53 AM
In D28105#634636, @ruiu wrote:

I don't think DWARFCompression is a good name because you can compress not only debug sections but any types of sections. Likewise, it shouldn't be in lib/DebugInfo directory.

Probably. lib/Object or lib/Support would be a better fit? (I vote for the former).

grimar updated this revision to Diff 83628.EditedJan 9 2017, 8:18 AM
grimar edited edge metadata.
grimar marked 10 inline comments as done.
  • Addressed review comments.
  • Moved to lib/Object
include/llvm/DebugInfo/DWARF/DWARFCompression.h
21 ↗(On Diff #82471)

Renamed to Decompressor, since it is lib/Object/Decompressor now

23 ↗(On Diff #82471)

Done.

29 ↗(On Diff #82471)

Done.

31–32 ↗(On Diff #82471)

Done.

37 ↗(On Diff #82471)

Done.

40 ↗(On Diff #82471)

Done.

43–44 ↗(On Diff #82471)

Made an accessor.

lib/DebugInfo/DWARF/DWARFContext.cpp
608 ↗(On Diff #82471)

Done.

614 ↗(On Diff #82471)

Added comment for ".z".

Note that line comes from original code, I just moved it below and added "z" handling.
Not sure why originally "_" was also dropped,
I never saw sections with name like "._xxx".
So I commented only my new logic for dropping "z".

grimar updated this revision to Diff 83799.Jan 10 2017, 5:27 AM
  • Addressed Rafael's comments.
grimar updated this revision to Diff 83933.Jan 11 2017, 2:04 AM
  • Addressed Rafael's review comments.
grimar retitled this revision from [DWARF] - Introduce DWARFCompression class. to [lib/Object] - Introduce Decompressor class..Jan 11 2017, 7:36 AM
grimar updated this object.
This revision was automatically updated to reflect the committed changes.