[PATCH] D31941: [ELF] - Implemented --compress-debug-sections option.

Wed Apr 12 05:40:21 PDT 2017

grimar added a comment.

In https://reviews.llvm.org/D31941#723864, @ruiu wrote:

> This patch doesn't use multiple cores to compress .debug section contents. In theory, if there are multiple .debug sections, we can compress them separately. Can you do that?

zlib format specification (http://www.zlib.org/rfc-zlib.html, 2.2. Data format) says that it has header, some data and checksum after, it not just a piece of raw bytes that we can
concatenate and have the same result so simple. At the same time one of zlib authors answered how that can be implemented in short:
http://stackoverflow.com/questions/30794053/how-to-use-multiple-threads-for-zlib-compression-same-input-source
It looks a bit tricky because requires fixups of results (dropping bytes etc), but definetely should be possible.

If so it may be reasonable to implement threading compression API for LLVM and reuse it in LLD.

That probably means we can go with simple streaming imprementation for start and replace code in OutputSection::finalize() with threading solution when it be implemented. What do you think ?

> The code that writes section contents into an in-memory buffer looks very similar to code that writes sections to the output file buffer. This needs fixing.

That should be solveable.

> The other approach I can think of is to do everything as a post-process. After writing everything to the output buffer, you scan .debug sections (that are at end of file), compress them in-place, and then shifts them to fill the holes created by data compression. Have you thought about that?

I thought about different post-process and droped idea (it was estimate somehow max compressed size and compress in place in writeTo. If estimated size value minus real size value would be something not too large,
it could work. Do not know how it can be possible to do such estimation and anyways output would be larger than possible).

And your suggestion doesn't look too clean either, does it ? That would mean that we do not know final layout even during writing, what a bit wierd. And then either we need to fixup sections headers and write them after other data. Then do shifting and after that all truncate output file, right ? That is a bit too tricky for my eye. That may degradate perfomance either, probably.

================
Comment at: ELF/Driver.cpp:560
+      return false;
+    if (S == "zlib" || S == "zlib-gabi")
+      return zlib::isAvailable();
----------------
ruiu wrote:
> ruiu wrote:
> > What is `-gabi`?
> I don't think `zlib` and `zlib-gabi` are the same. What you implemented in this patch is `zlib-gabi`, so don't handle `zlib` as a synonym for `zlib-gabi`.
http://man7.org/linux/man-pages/man1/ld.1.html says:
//--compress-debug-sections=zlib and
 --compress-debug-sections=zlib-gabi compress DWARF debug sections with SHF_COMPRESSED from the ELF ABI.  The default behaviour varies depending upon the target involved and the configure options used to build the toolchain.//

https://docs.oracle.com/cd/E53394_01/html/E54813/man-cds.html says that there are 3 options: none, zlib, zlib-gnu.
It does not mention what is zlib-gabi.

The only compression flag available now in generic ABI is ELFCOMPRESS_ZLIB:
http://www.sco.com/developers/gabi/latest/ch4.sheader.html

So my conclusion from above:
1) Since oracle does not mention zlib-gabi, I implemented zlib and not zlib-gabi.
2) Since ld manual says both zlib/zlib-gabi compresses sections with SHF_COMPRESSED from ABI and only available type is ELFCOMPRESS_ZLIB, then zlib and zlib-gabi is the same.

================
Comment at: ELF/Options.td:25

+def compress_debug_sections : Joined<["--"], "compress-debug-sections=">,
+  HelpText<"Compresses DWARF debug sections">;
----------------
ruiu wrote:
> Why don't you use `J`?
Copypaste from def O: Joined<["-"] will fix.

================
Comment at: ELF/OutputSections.cpp:74
+    return;
+  // SHF_COMPRESSED can not be used in conjunction with SHF_ALLOC flag.
+  if (!(Flags & SHF_ALLOC))
----------------
ruiu wrote:
> Why?
https://docs.oracle.com/cd/E36784_01/html/E36857/compressed_debug_sections.html
("To be a candidate for compression, a section must be non-allocable, and belong to ....")

https://docs.oracle.com/cd/E53394_01/html/E54813/section_compression.html
("SHF_COMPRESSED applies only to non-allocable sections, and cannot be used in conjunction with SHF_ALLOC. In addition, SHF_COMPRESSED cannot be applied to sections of type SHT_NOBITS.").

================
Comment at: ELF/OutputSections.cpp:110
+    std::unique_ptr<zlib::StreamCompression> Compressor =
+        check(zlib::StreamCompression::create(1024));
+    for (size_t I = 0, E = Sections.size(); I != E; ++I) {
----------------
ruiu wrote:
> What is `1024`?
> 
> Is the default compression level the best for us?
1024 is a size of buffer used for output in zlib::StreamCompression implementation currently. It may be worth to try different values and benchmark LLD separatelly.

Regarding second question, zlib::StreamCompression indeed currently uses default compression level. What is best may probably depend on what we want. Speed or size or something in the middle. That also requires additional benchmarking probably and additional discussion based on benchmarking results. In current implemenation compression class (D31943) does not yet have API for setting level.

================
Comment at: ELF/OutputSections.cpp:309-315
+  // If we have compressed output section here, we should write
+  // header and data that we already prepared in finilize().
+  // Sections compression header format is explaned here:
+  // https://docs.oracle.com/cd/E53394_01/html/E54813/section_compression.html
+  if (!CompressedData.empty()) {
+    writeCompressed(Buf, CompressedData, DecompressedSize, Alignment);
+    return;
----------------
ruiu wrote:
> You can do this in finalize(), right?
Do you mean adding compression header to buffer ? If so, yes, I can.
(Sure I can't write something to output file there, output is not exist at that point).

https://reviews.llvm.org/D31941