[PATCH] D130458: [llvm-objcopy] Support --{,de}compress-debug-sections for zstd

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objcopy.rst:302
- Compress DWARF debug sections in the output, using the specified style.
- Supported styles are `zlib-gnu` and `zlib`. Defaults to `zlib` if no style is
  specified.
----------------
Might be worth removing zlib-gnu in a separate patch before this one, so if zstd is reverted we don't lose the fix to remove zlib-gnu?


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:437-446
-template <class ELFT>
-static std::tuple<uint64_t, uint64_t>
-getDecompressedSizeAndAlignment(ArrayRef<uint8_t> Data) {
-  const uint64_t DecompressedSize =
-      reinterpret_cast<const Elf_Chdr_Impl<ELFT> *>(Data.data())->ch_size;
-  const uint64_t DecompressedAlign =
-      reinterpret_cast<const Elf_Chdr_Impl<ELFT> *>(Data.data())->ch_addralign;
----------------
Could probably do this refactor/inlining in a separate commit.


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:444
+    return createStringError(errc::invalid_argument,
+                             "failed to uncompress section '" + Sec.Name +
+                                 "': " + toString(std::move(Err)));
----------------
`uncompress` V `decompress`? Seems most of the wording here uses decompress & by google search results it seems the more common term.


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:449-457
+    if (Error E = compression::zlib::uncompress(Compressed, Decompressed,
+                                                static_cast<size_t>(Sec.Size)))
+      return Report(std::move(E));
+    break;
+  case ELFCOMPRESS_ZSTD:
+    if (Error E = compression::zstd::uncompress(Compressed, Decompressed,
                                                 static_cast<size_t>(Sec.Size)))
----------------
sounded like the folks contributing zstd support had some plans to make modular compression algorithm wrappers - that I'd guess would look more like "get the compression handler, given the compression type, then use it" so wouldn't need to have two separate calls to uncompress here?


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:459-462
     return createStringError(errc::invalid_argument,
-                             "'" + Sec.Name + "': " + toString(std::move(Err)));
+                             "--decompress-debug-sections: ch_type (" +
+                                 Twine(Sec.ChType) + ") of section '" +
+                                 Sec.Name + "' is unsupported");
----------------
Was there no error handling before & any compression type was treated as zlib & decompressed using zlib?


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:543-544
+    break;
+  default:
+    llvm_unreachable("unsupported CompressionType should have been errored");
+  }
----------------
Might be better to add the case for `DebugCompressionType::None` here so that we get a compilation error (uncovered switch) next time a compression algorithm is added to the enum?


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:733-749
       if (Config.CompressionType == DebugCompressionType::None)
         return createStringError(
             errc::invalid_argument,
             "invalid or unsupported --compress-debug-sections format: %s",
             InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections_eq)
                 .str()
                 .c_str());
----------------
Maybe make these a switch over `DebugCompressionType` so we can catch this as a compilation error if a new one is added?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130458/new/

https://reviews.llvm.org/D130458