[PATCH] D51841: [llvm-objcopy] Dwarf decompression support.

Fri Sep 14 02:23:07 PDT 2018

jhenderson added a reviewer: jhenderson.
jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

Please remember to include me as a reviewer on all llvm-objcopy reviews.

================
Comment at: test/tools/llvm-objcopy/compress-debug-sections.test:5
+
+# RUN: llvm-objcopy --compress-debug-sections=zlib     %t.o %tz.o
+# RUN: llvm-objcopy --compress-debug-sections=zlib-gnu %t.o %tzg.o
----------------
As with other tests, this test (or possibly the specific compress and decompress tests) should show that the inputs are not modified when using the specified switch.

================
Comment at: test/tools/llvm-objcopy/decompress-debug-sections-zlib-gnu.test: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 -r %t-compressed.o | FileCheck %s --check-prefix=CHECK-FLAGS
----------------
Why is this check and the following one operating on the compressed object? That's tested elsewhere. In fact, I'm not sure this test is really testing the decompression at all. It looks like almost a copy-and-paste job from the compressed test.

================
Comment at: test/tools/llvm-objcopy/decompress-debug-sections-zlib-gnu.test:9
+# RUN: llvm-objdump -s %t-compressed.o | FileCheck %s --check-prefix=CHECK-COMPRESSED
+# RUN: llvm-readobj -s -r %t-compressed.o | FileCheck %s --check-prefix=CHECK-FLAGS
+# RUN: llvm-objdump -s %t-decompressed.o -section=.debug_foo | FileCheck %s
----------------
This check-prefix is badly named, since we are testing more than flags with it.

================
Comment at: test/tools/llvm-objcopy/decompress-debug-sections-zlib-gnu.test:10
+# RUN: llvm-readobj -s -r %t-compressed.o | FileCheck %s --check-prefix=CHECK-FLAGS
+# RUN: llvm-objdump -s %t-decompressed.o -section=.debug_foo | FileCheck %s
+
----------------
What is this check achieving?

================
Comment at: test/tools/llvm-objcopy/decompress-debug-sections-zlib.test:1
+# REQUIRES: zlib
+
----------------
The same comments apply to this test as the other test.

================
Comment at: tools/llvm-objcopy/Object.cpp:155
+  const char *Magic = "ZLIB";
+  return Section.OriginalData.size() > strlen(Magic) &&
+         !strncmp(reinterpret_cast<const char *>(Section.OriginalData.data()),
----------------
No need to use `strlen` on a string literal. In general, use char arrays and sizeof. However, in this case, make `Magic` an ArrayRef, and use the size method and std::equals or similar to compare, rather than strncmp (assuming that ArrayRefs don't have a startswith method). It will read much better, and avoids unnecessary casting from uint8_t to const char.

================
Comment at: tools/llvm-objcopy/Object.cpp:163
+  uint8_t *Buf = Out.getBufferStart() + Sec.Offset;
+
+  const unsigned dataOffset =
----------------
Get rid of the blank lines in this function. They don't really improve readability, in my opinion.

================
Comment at: tools/llvm-objcopy/Object.cpp:164
+
+  const unsigned dataOffset =
+      isCompressedGnuContents(Sec)
----------------
Don't use `unsigned`. Use `size_t`, since that's the return type of sizeof.

`dataOffset` -> `DataOffset`
Please remember to follow LLVM style.

================
Comment at: tools/llvm-objcopy/Object.cpp:166
+      isCompressedGnuContents(Sec)
+          ? (strlen("ZLIB") + sizeof(Sec.DecompressedSize))
+          : sizeof(Elf_Chdr_Impl<ELFT>);
----------------
As "ZLIB" and its size are used everywhere, I suggest making it a constant global variable that you can query with .size(), as required. (See also my comments elsewhere about making it a uint8_t array/ArrayRef).

================
Comment at: tools/llvm-objcopy/Object.cpp:173
+
+  SmallVector<char, 128> DecompressedContent;
+  if (Error E = zlib::uncompress(CompressedContent, DecompressedContent,
----------------
This may not be your fault, but SmallVector is clearly the wrong thing for this here, since this won't be Small for the vast majority of uses.

================
Comment at: tools/llvm-objcopy/Object.cpp:175
+  if (Error E = zlib::uncompress(CompressedContent, DecompressedContent,
+                                 (size_t)Sec.DecompressedSize))
+    reportError(Sec.Name, std::move(E));
----------------
Is this explicit cast necessary? If it is (e.g. we support 32-bit hosts still), use static_cast.

================
Comment at: tools/llvm-objcopy/Object.cpp:1022
     return Obj.addSection<Section>(Data);
   default:
     Data = unwrapOrError(ElfFile.getSectionContents(&Shdr));
----------------
Please add braces here to this case, so that the local variables scope is tied to the case (see e.g. case SHT_SYMTAB_SHNDX).

================
Comment at: tools/llvm-objcopy/Object.cpp:1026
+    const bool IsGnuDebug =
+        StringRef((const char *)Data.data(), Data.size()).startswith("ZLIB");
+    if (IsGnuDebug || (Shdr.sh_flags & ELF::SHF_COMPRESSED)) {
----------------
Don't use C-style casts. Also, this is the wrong thing to do here anyway. Compare the bytes against an array of uint8_t, rather than converting it to a string. Note that uint8_t does not necessarily convert trivially to char.

================
Comment at: tools/llvm-objcopy/Object.cpp:1027
+        StringRef((const char *)Data.data(), Data.size()).startswith("ZLIB");
+    if (IsGnuDebug || (Shdr.sh_flags & ELF::SHF_COMPRESSED)) {
+
----------------
I feel like most of this new code should be in a different function, which would add the section to the object,  the call to which would be guarded by this if.

================
Comment at: tools/llvm-objcopy/Object.cpp:1028
+    if (IsGnuDebug || (Shdr.sh_flags & ELF::SHF_COMPRESSED)) {
+
+      const uint64_t DecompressedSize =
----------------
Remove this blank line.

================
Comment at: tools/llvm-objcopy/Object.h:405
+        DecompressedAlign(Sec.DecompressedAlign) {
+    Size = DecompressedSize;
+    Flags = (Flags & ~ELF::SHF_COMPRESSED);
----------------
plotfi wrote:
> Oops, I think I need to add Align = DecompressedAlign here. What do you think @jakehehrlich 
What is DecompressedAlign actually used for? You should certainly be setting Align.

================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:436
+    const CopyConfig &Config, Object &Obj, SectionPred &RemovePred,
+    std::function<bool(const SectionBase &)> doReplacement,
+    std::function<SectionBase *(const SectionBase *)> addSection) {
----------------
`doReplacement` isn't clear what it means from the name alone. Perhaps `shouldReplace` would be clearer.

================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:452
+  for (SectionBase *S : ToReplace) {
+    SectionBase *NS = addSection(S);

----------------
NS? Use a clearer name, please, e.g. `NewSection`

================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:676

-  if (Config.CompressionType != DebugCompressionType::None)
-    compressSections(Config, Obj, RemovePred);
+  if (Config.CompressionType != DebugCompressionType::None) {
+    replaceDebugSections(Config, Obj, RemovePred, isCompressable,
----------------
No need for the '{' and '}' here and in the else.

================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:1019-1020
+      Config.CompressionType != DebugCompressionType::None) {
+    error("Cannot specify --compress-debug-sections as well as "
+          "--decompress-debug-sections at the same time");
+  }
----------------
"... as well as --decompress-debug-sections at the same time" -> "... at the same time as --decompress-debug-sectons"

Repository:
  rL LLVM

https://reviews.llvm.org/D51841