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

Tue Aug 28 02:16:24 PDT 2018

jhenderson added inline comments.

================
Comment at: test/tools/llvm-objcopy/Inputs/compress-debug-sections.yaml:10
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_MERGE, SHF_STRINGS ]
+    Content:         416c6c2076697369626c65206f626a656374732c206d616e2c2061726520627574206173207061737465626f617264206d61736b732e
----------------
Why are these sections SHF_MERGE, SHF_STRINGS?

================
Comment at: test/tools/llvm-objcopy/Inputs/compress-debug-sections.yaml:11
+    Flags:           [ SHF_MERGE, SHF_STRINGS ]
+    Content:         416c6c2076697369626c65206f626a656374732c206d616e2c2061726520627574206173207061737465626f617264206d61736b732e
+  - Name:            .notdebug_foo
----------------
Why is this content more than just some tiny number of bytes, e.g. 4 (i.e. 00010203 or similar)?

================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test:27
+# CHECK-FLAGS: Name: .notdebug_foo
+# CHECK-FLAGS: Flags [ (0x30)
+
----------------
jhenderson wrote:
> I'd prefer it if this explicitly listed the section flags, without the number:
> ```
> # CHECK-FLAGS: Name: .notdebug_foo
> # CHECK-FLAGS: Flags [
> # CHECK-FLAGS-NEXT: SHF_MERGE // or whatever
> # CHECK-FLAGS-NEXT: SHF_STRINGS // or whatever
> # CHECK-FLAGS-NEXT: ]
> ```
> What could happen here is that if .notdebug_foo's flags changed, this could match the flags of a later section incorrectly.
Please use CHECK-NEXT as in my above explanation. What could happen here is for this test to fail spuriously if there is a later section with SHF_COMPRESSED in. Using CHECK-NEXT prevents this from happening.

It might be useful to have a separate test to show what happens to existing section flags when compressing, but I don't think that should be in the basic behaviour tests, because it confuses people as to what is important.

================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test:1
+# REQUIRES: zlib
+
----------------
This and the other test should show that the non-compressed version of the section (i.e. .debug_foo) has been removed.

================
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
----------------
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.

================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib.test:1
+# REQUIRES: zlib
+
----------------
Same comments apply to this test as to the gnu test.

================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:22
+                     MetaVarName<"[ none | zlib | zlib-gnu ]">,
+                     HelpText<"Enable zlib-gnu or zlib Compression of DWARF debug sections.">;
 def O : JoinedOrSeparate<["-"], "O">,
----------------
I'd rephrase this to: "compress DWARF debug sections using specified style. Supported styles: 'zlib-gnu' and 'zlib'"

================
Comment at: tools/llvm-objcopy/Object.cpp:171
+                                     DebugCompressionType CompressionType)
+    : Data(std::begin(Sec.OriginalData), std::end(Sec.OriginalData)),
+      CompressionType(CompressionType) {
----------------
Can't you just set this->OriginalData to Sec.OriginalData?

That would allow you to get rid of the Data member entirely.

================
Comment at: tools/llvm-objcopy/Object.cpp:173
+      CompressionType(CompressionType) {
+
+  Name = Sec.Name;
----------------
Don't have a blank line here.

================
Comment at: tools/llvm-objcopy/Object.cpp:188
+  Offset = Sec.Offset;
+  Type = ELF::SHT_PROGBITS;
+
----------------
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.

================
Comment at: tools/llvm-objcopy/Object.cpp:196
+          CompressedData))
+    objcopy::reportError(Name, std::move(E));
+
----------------
Nit: I don't think you need the explicit namespace here.

================
Comment at: tools/llvm-objcopy/Object.h:17
 #include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/StringTableBuilder.h"
----------------
You can get rid of this from the header by forward-declaring the DebugCompressionType enum.

================
Comment at: tools/llvm-objcopy/Object.h:20-22
+#include "llvm/Support/Compression.h"
+#include "llvm/Support/EndianStream.h"
+#include "llvm/Support/Errc.h"
----------------
I don't think you need any of these headers.

================
Comment at: tools/llvm-objcopy/Object.h:373
+
+  void finalize() override;
+  void accept(SectionVisitor &Visitor) const override;
----------------
You don't need this function - the base class provides a default of "do nothing" for it.

================
Comment at: tools/llvm-objcopy/Object.h:377
+    Flags |= Sec.Flags;
+    if (isGnuStyle)
+      return;
----------------
plotfi wrote:
> jakehehrlich wrote:
> > Does it cause problems with other tools if we set SHF_COMPRESSED for GNU style? If not I think I'd like to set it regardless.
> it might, but I dont remember off the top of my head. I will try out setting it and using gnu objcopy to see what happens. 
I'm not sure we can make any assumptions for all tools written out there, so I'm reluctant to support having the flag set for GNU-style compressed sections. In particular, I could imagine a case where a tool checks for the flag first, and if it sees it, decompresses it in ZLIB style, or tries to and fails, because it is actually in GNU format. Only if it doesn't see the flag might it then try to decompress as GNU.

================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:414
+
+static void appendCompressableSections(const CopyConfig &Config, Object &Obj,
+                                       SectionPred &RemovePred) {
----------------
Is there a reason you changed the name of this function? I feel like it was fine to be called `compressSections`. `appendCompressableSections` suggests that it just adds some sections to the ELF, but in fact, it's also updating the removal predicate to remove the to-be-compressed sections.

Repository:
  rL LLVM

https://reviews.llvm.org/D49678