[PATCH] D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm liking the look of this now. Still got a few minor things still to work on though.



================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test:32
+# NOTE: Zlib compression makes things worse in this case.
+# SIZE-ORIGINAL: 637
+# SIZE-COMPRESSED: 656
----------------
jhenderson wrote:
> I think it probably makes more sense to get the section size and compare that, rather than the overall file size. You can drop the "wc" calls that way.
Don't do it this way. You can use the section size field directly (available in the sections dump from earlier), rather than extracting the sections.


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test:7-8
+# RUN: llvm-objdump -s %t.o -section=.debug_foo | FileCheck %s
+# RUN: llvm-objdump -s %t-compressed.o | FileCheck %s --check-prefix=CHECK-COMPRESSED
+# RUN: llvm-readobj -s %t-compressed.o | FileCheck %s --check-prefix=CHECK-FLAGS
+
----------------
Is there a particular reason you aren't doing the two checks in the same run of FileCheck?


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test:20
+# CHECK-FLAGS-NEXT: Flags [
+#
+# CHECK-FLAGS-NEXT: ]
----------------
Remove this line.


================
Comment at: tools/llvm-objcopy/Object.cpp:188
+  Offset = Sec.Offset;
+  Type = ELF::SHT_PROGBITS;
+
----------------
jakehehrlich wrote:
> plotfi wrote:
> > jhenderson wrote:
> > > This is extremely fragile: if we end up adding to the SectionBase list of members, this list will likely not be updated. Also, some of these fields may well be unset at the point of creation of this section (I'm thinking NameIndex specifically, but there may be others - I haven't bothered to check).
> > > 
> > > I think you might be able to get away with calling the base class's assignment method to set these fields, rather than manually copying each one.
> > Base class has no assignment method. It'll try and add one. Also base class has no constructor because it's abstract I believe. 
> @jhenderson There is a generic issue with the fragility of cloning the fields of other sections for the purposes of synthetic sections. So far we've kind of punted on this issue. (for instance, OwnedDataSection and DebugGnuLinkSection or whatever its called).
> 
> I think the defined operator is not the right way to go if C++ won't define that method for us. I think we should think about it more as "what fields need to be set right here and now". NameIndex for instance does not need to be set. I don't know...it's kind of unclear how this should work to me. Maybe a method that isn't '=' but is for copying the base fields that are always set that is inside the SectionBase class? Or perhaps the previous "fragile" solution is actually best with a few tweaks. Both seem equally fragile to me personally unless the compiler defines the method for us.
I think the use of the default copy constructor (as done by @plotfi now) does the trick - it means that everything will be initialised, regardless of how many fields are added.


================
Comment at: tools/llvm-objcopy/Object.h:17
 #include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/StringTableBuilder.h"
----------------
plotfi wrote:
> jhenderson wrote:
> > You can get rid of this from the header by forward-declaring the DebugCompressionType enum.
> You can not. Usage of DebugCompressionType::GNU etc makes this not possible unless you want to copy the enum class (which I would like not to).
You aren't using them in the header file, as far as I can see. Move the include into the .cpp if it is needed there. Same goes for Compression.h.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:416
+                             SectionPred &RemovePred) {
+  SmallVector<SectionBase *, 13> SectionsToRemove;
+  std::for_each(Obj.sections().begin(), Obj.sections().end(),
----------------
SectionsToRemove -> SectionsToCompress


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:417
+  SmallVector<SectionBase *, 13> SectionsToRemove;
+  std::for_each(Obj.sections().begin(), Obj.sections().end(),
+                [&SectionsToRemove](SectionBase &Section) {
----------------
Use std::copy_if and std::back_inserter to do this. It looks a bit cleaner, in my opinion.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:898
+  if (Config.CompressionType == DebugCompressionType::None &&
+      InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections) != "") {
+    error("Invalid or unsupported --compress-debug-sections format: " +
----------------
I'm not sure this is quite what you want. I think this means that the following command-line will be accepted but do no compression:
```
llvm-objcopy foo.o --compress-debug-sections
```

You probably want to use `hasArg` and\or change the default if there is no argument to --compress-debug-sections (e.g. --compress-debug-sections with no argument is the same as --compress-debug-sections=zlib).


Repository:
  rL LLVM

https://reviews.llvm.org/D49678