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

[George Rimar via llvm-commits]

grimar added inline comments.

================
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));
----------------
ruiu wrote:
> Please don't use an unrelated object's members.
Yep, I know, that is just because it was a draft patch.
I was thinking about it when did that. What is the good way ?
Actually I do not like how it is done now: we have lots of StringSaver instances in many places in lld.
Are there reasons for that instead of having one StringSaver for all with single Alloc and just global reference to it ?

================
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;
+
----------------
ruiu wrote:
> I think we don't have to care about this edge case. Unless you are compressing random bits, it usually shrink.
I would leave it as is. That is mentioned in docs: "The .debug_line and .debug_pubnames sections would be larger compressed than in their original uncompressed form, and have therefore been left uncompressed." (https://docs.oracle.com/cd/E53394_01/html/E54813/section_compression.html#scrolltoc) and done both by gold/bfd and very transparent in implementation. 
In addition I think that if feature is used then motivation and expected result is to consume less space, I do not think it is acceptable to consume more than without it, it is confusing and opposite to its description.

================
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);
+  }
+
----------------
ruiu wrote:
> Why we have two types of headers? If one is obsolete, we probably don't want to implement it.
There are 2 different approaches used (https://docs.oracle.com/cd/E53394_01/html/E54813/section_compression.html):
1) gnu-style renames sections to ".z*[NAME]" and do not uses SHF_COMPRESSED.
2) zlib style. Sections have SHF_COMPRESSED flag and are not renamed.

"Unless there is a specific requirement to use the zlib-gnu style, the more general default zlib style is recommended." says https://docs.oracle.com/cd/E53394_01/html/E54813/man-cds.html, but it is not mentioned if that depricated or something.
Not supporting of zlib-gnu style would make implementation a bit cleaner.

It is not hard to support both though.

================
Comment at: ELF/Writer.cpp:1172
@@ +1171,3 @@
+    return false;
+  return zlib::isAvailable();
+}
----------------
ruiu wrote:
> This condition should be at the beginning of this function.
Actually I assumed that is low user path to have zlib not available.
btw, since I am on windows I had some problems with it, it is well described at https://llvm.org/bugs/show_bug.cgi?id=19403.
So windows users currently will not able to use that feature atm, all *nix ones probably already have zlib installed.

But ok, I`ll move that since it is also logical for me that we first check if feature available at first before any other conditions.


http://reviews.llvm.org/D20211