[PATCH] D130724: [MC] Support writing ELFCOMPRESS_ZSTD compressed debug info sections

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/MC/ELFObjectWriter.cpp:861-879
+  ArrayRef<uint8_t> Uncompressed =
       makeArrayRef(reinterpret_cast<uint8_t *>(UncompressedData.data()),
-                   UncompressedData.size()),
-      Compressed);
+                   UncompressedData.size());
+
+  SmallVector<uint8_t, 128> Compressed;
+  uint32_t ChType;
+  switch (MAI->compressDebugSections()) {
----------------
Proliferation of these sort of switch statements is why I'd like to resolve the design discussions before further patches are committed. They're not hugely expensive to fix/change later, but neither are we in a rush to add this functionality, nor is the design discussion especially hard to resolve.
Code something like:
```
if (CompressionAlgorithm *C = getCompressionAlgorithm(MAI->compressDebugSections()) {
  ChType = C->getELFType(); // alternatively have an array to lookup ELF type from the enum type
  C->compress(Uncompressed, Compressed);
}
```
Seems like it'd be quite achievable without great complexity & simplify consumers.

Possibly even having 'compressDebugSections' carry the algorithm pointer directly? (since the checking's already done up-front, and the pointer would be to an immutable/thread safe/singleton/eternal object anyway)


================
Comment at: llvm/tools/llvm-mc/llvm-mc.cpp:404-424
+  switch (CompressDebugSections) {
+  case DebugCompressionType::None:
+    break;
+  case DebugCompressionType::Z:
+    if (!compression::zlib::isAvailable()) {
+      WithColor::error(errs(), ProgName)
+          << "build LLVM with LLVM_ENABLE_ZLIB to enable "
----------------
This could potentially be replaced by something like:
```
CompressionAlgorithm *C = getCompressionAlgorithm(CompressDebugSections);
if (C && !C->isAvailable()) {
  WithColor::error(errs(), ProgName) << "build LLVM with " << C->getBuildFlag() << " to enable " << CompressDebugSections.is_there_some_way_to_query_the_opts_full_spelling();
  C = nullptr;
}
MAI->setCompressDebugSections(C);
```
I guess the desire to get the build flag name might be enough to justify `getCompressionAlgorithm` returning non-null even for non-available algorithms, so it can provide that info? (unlike my previous comment/example above that implies `getCompressionAlgorithm` returns null for non-available algorithms)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130724