[PATCH] D20211: [ELF] - Support of compressed debug sections creation(draft).

[Rui Ueyama via llvm-commits]

ruiu added a comment.

Please add tests to verify that you are producing compressed sections that can be read by other tools.


================
Comment at: ELF/Options.td:33
@@ +32,3 @@
+def compress_debug_sections : Joined<["--"], "compress-debug-sections=">,
+  HelpText<"Force undefined symbol during linking">;
+
----------------
This help text is out of context.

================
Comment at: ELF/OutputSections.cpp:833-837
@@ +832,7 @@
+  // compression.
+  if (Config->CompressDebugSections == "zlib-gabi" ||
+    Config->CompressDebugSections == "zlib") {
+    Header.sh_flags |= SHF_COMPRESSED;
+  }
+  else {
+    // Compress all candidate sections using ZLIB compression, using the GNU
----------------
Clang-format.

================
Comment at: ELF/OutputSections.cpp:842
@@ +841,3 @@
+    // renamed to start with.zdebug to identify the use of compression.
+    StringSaver Saver(ScriptConfig->Alloc);
+    this->Name = Saver.save(Twine(".z") + Name.drop_front(1));
----------------
Please don't use an unrelated object's members.

================
Comment at: ELF/OutputSections.cpp:859-862
@@ +858,6 @@
+template <class ELFT> void OutputSection<ELFT>::finalizeCompressed() {
+  std::vector<uint8_t> Uncompressed(getSize());
+  uint8_t *Data = Uncompressed.data();
+  for (InputSection<ELFT> *C : Sections)
+    C->writeTo(Data);
+
----------------
This code creates an uncompressed a debug section in memory and then compress it at once. It consumes as much memory as the total size of the debug section. It is probably unacceptable.

zlib supports "streaming" compression, right? I think you can pass pieces of data to zlib object and let it construct compressed data incrementally. Please do that way so that the maximum memory is bounded to the largest input section size instead of the largest output section size.

================
Comment at: ELF/OutputSections.cpp:869-874
@@ +868,8 @@
+
+  // If not successfully compressed or total size is greater than
+  // size before applying compression, then cancel compressed output.
+  size_t TotalSize = Compressed.size() + getCompressionHeaderSize<ELFT>();
+  Compress = Success == zlib::Status::StatusOK && TotalSize < getSize();
+  if (!Compress)
+    return;
+
----------------
I think we don't have to care about this edge case. Unless you are compressing random bits, it usually shrink.

================
Comment at: ELF/OutputSections.cpp:1014-1025
@@ +1013,14 @@
+  const endianness E = ELFT::TargetEndianness;
+  if (Config->CompressDebugSections == "zlib-gnu") {
+    // 4 bytes magic + size of uncompressed data in big endian.
+    memcpy(Buf, "ZLIB", 4);
+    write64be(Buf + 4, UncompressedSize);
+  } else {
+    // "zlib-gabi" or "zlib".
+    write32<E>(Buf, ELFCOMPRESS_ZLIB);
+    if (ELFT::Is64Bits)
+      write64<E>(Buf + 2 * sizeof(Elf64_Word), UncompressedSize);
+    else
+      write32<E>(Buf + sizeof(Elf32_Word), UncompressedSize);
+  }
+
----------------
Why we have two types of headers? If one is obsolete, we probably don't want to implement it.

================
Comment at: ELF/Writer.cpp:1161-1163
@@ -1158,2 +1160,5 @@
 
 template <class ELFT>
+static bool isCompressionCandidate(InputSectionBase<ELFT> *C,
+                                   StringRef OutSecName) {
+  if (Config->CompressDebugSections == "none")
----------------
You can move this to OutputSection class, no?

================
Comment at: ELF/Writer.cpp:1172
@@ +1171,3 @@
+    return false;
+  return zlib::isAvailable();
+}
----------------
This condition should be at the beginning of this function.


http://reviews.llvm.org/D20211