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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay 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.
----------------
dblaikie wrote:
> 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?
Will do. I wanted to check the wording here: whether `style` should be changed to `format` and how we should state `Use zlib if <format> is omitted`.

Just jumped the gun a bit and pushed a commit to remove zlib-gnu and switch to `format`.
`style` makes less sense now as the zlib-gnu style has been removed.


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:444
+    return createStringError(errc::invalid_argument,
+                             "failed to uncompress section '" + Sec.Name +
+                                 "': " + toString(std::move(Err)));
----------------
dblaikie wrote:
> `uncompress` V `decompress`? Seems most of the wording here uses decompress & by google search results it seems the more common term.
"decompress" makes sense to me. I used "uncompress" because the API and many compression libraries use the name.

https://datatracker.ietf.org/doc/html/rfc8478 says that "uncompressed" is related to the state and "decompress" seems preferred for the act of processing.


================
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)))
----------------
dblaikie wrote:
> 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?
This can be a future improvement when the API is available.


================
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");
----------------
dblaikie wrote:
> Was there no error handling before & any compression type was treated as zlib & decompressed using zlib?
Right. There was no test before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130458