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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

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

> What I was saying was that you can compress multiple sections concurrently, right? I didn't say you should try to compress single stream of data using multiple threads. I meant, since we have multiple .debug_ sections, you could compress them separately.


OK, I understood differently. Thought you're talking about multithreaded compression for input .debug* sections.



================
Comment at: ELF/Driver.cpp:560
+      return false;
+    if (S == "zlib" || S == "zlib-gabi")
+      return zlib::isAvailable();
----------------
ruiu wrote:
> grimar wrote:
> > 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.
> When I give -compress-debug-sections=zlib and -compress-debug-sections=zlib-gabi, outputs from gold were actually different. But interestingly enough the difference wasn't meaningful. Gold seems to set garbage to ch_reserved members of compressed section headers. (Probably they forgot to initialize the header with zero before setting values to the members.)
> 
> So my conclusion is zlib-gabi and zlib are the same. But I don't see a reason to add two identical options at the moment. Please add just "zlib".
Done.


================
Comment at: ELF/Driver.cpp:562
+      return zlib::isAvailable();
+    // We support only default zlib compression style now.
+    error("unknown --compress-debug-sections value: " + S);
----------------
ruiu wrote:
> This is obvious, so remove this comment.
Done.


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


================
Comment at: ELF/OutputSections.cpp:72-76
+  if (!Config->CompressDebugSections || !Name.startswith(".debug"))
+    return;
+  // SHF_COMPRESSED can not be used in conjunction with SHF_ALLOC flag.
+  if (!(Flags & SHF_ALLOC))
+    this->Flags |= SHF_COMPRESSED;
----------------
ruiu wrote:
> The way this code is written is confusing. What you should state is that we want to compress .debug sections (which needless to say non-alloc sections).
> 
>   // If -compress-debug-sections is specified, we compress output debug sections.
>   if (Config->CompressDebugSections && Name.startswith(".debug_") && !(Flags & SHF_ALLOC))
>     this->Flags |= SHF_COMPRESSED;
> 
> However, this piece of code should probably be moved to finalize().
Ok. FWIW spec states that debug sections are ".debug*" and not ".debug_*"
(https://docs.oracle.com/cd/E53394_01/html/E54813/man-cds.html)
Though I think your way should work anyways.

It also mentiones other namings:
Debug sections are identified by having a .compcom, .line, .stab*, .debug*, or .zdebug* section name.

I would start only from .debug ones in this patch.


================
Comment at: test/ELF/compress-debug-sections.s:13-15
+# ZLIBCONTENT-NEXT:  0000 01000000 00000000 38000000 00000000
+# ZLIBCONTENT-NEXT:  0010 01000000 00000000 789c6360 200f0000
+# ZLIBCONTENT-NEXT:  0020 00380001
----------------
ruiu wrote:
> Don't check for the section contents because `zlib` doesn't really guarantee the output.
I reimplemented this test in a more smart way.


https://reviews.llvm.org/D31941