[PATCH] D28105: [DWARF] - Introduce DWARFCompression class.

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 26 13:53:14 PST 2016


dblaikie added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFCompression.h:21
+/// This class helps to handle decompression of compressed debug sections.
+class DWARFCompression {
+public:
----------------
Slightly odd/non-descript name (DWARFDecompressor? it seems specifically for decompressing)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFCompression.h:31-32
+
+  /// Used to get text representation of error if any happened.
+  StringRef getErrorMessage() { return ErrorMsg; }
+
----------------
Could uncompress return Error instead of bool, and then the error message could be included there?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFCompression.h:34-38
+  /// Returns if section is compressed, including gnu-styled case.
+  static bool isCompressed(const object::SectionRef &Section);
+
+  /// Using section name and flags returns if section is a ELF compressed one.
+  static bool isCompressedELFSection(uint64_t Flags, StringRef Name);
----------------
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)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFCompression.h:43-44
+
+  /// Contains memory buffer size required for decompression.
+  uint64_t UncompressedSize;
+
----------------
strange to have a mutable public member - should this be an accessor?


https://reviews.llvm.org/D28105





More information about the llvm-commits mailing list