[Support] Add llvm::compression::{getReasonIfUnsupported,compress,decompress}

[MaskRay]

Summary

as high-level API on top of llvm::compression::{zlib,zstd}::*.

• getReasonIfUnsupported: return nullptr if the specified format is supported, or (if unsupported) a string like LLVM was not built with LLVM_ENABLE_ZLIB ...
• compress: dispatch to zlib::uncompress or zstd::uncompress
• decompress: dispatch to zlib::uncompress or zstd::uncompress

Move llvm::DebugCompressionType from MC to Support to avoid Support->MC cyclic
dependency. There are 40+ uses in llvm-project.

Add another enum class llvm::compression::Format to represent supported
compression formats, which may be a superset of ELF compression formats.

See D130458 (llvm-objcopy --{,de}compress-debug-sections for zstd) for a use
case.

---

Note: this patch alone will cause -Wswitch to llvm/lib/ObjCopy/ELF/ELFObject.cpp

[dblaikie]

@ckissane sounded like you/your colleagues already had some outstanding patches/plans to generalize the compression support - should we be considering those patches/directions alongside this one?

I'd had in mind something like a compression algorithm base class, two (or three, if it's useful to have a default implementation for "None") derived classes, singleton instances of each and a function that takes the algorithm type and returns the handler - which can then be queried for "is available" and provides compress/decompress actions. That'd mean having only one switch instead of three, but the three are now so close together that it's not hugely painful to maintain (though having a class hierarchy would ensure the APIs were actually identical, whereas currently they're only identical by convention).

[MaskRay]

llvm/include/llvm/MC/MCTargetOptions.h

29

If llvm::CompressionFormat (or possibly under llvm::compression) looks better, I'll also rename Z to Zlib.

[ckissane]

In D130506#3677033, @dblaikie wrote:

@ckissane sounded like you/your colleagues already had some outstanding patches/plans to generalize the compression support - should we be considering those patches/directions alongside this one?

I'd had in mind something like a compression algorithm base class, two (or three, if it's useful to have a default implementation for "None") derived classes, singleton instances of each and a function that takes the algorithm type and returns the handler - which can then be queried for "is available" and provides compress/decompress actions. That'd mean having only one switch instead of three, but the three are now so close together that it's not hugely painful to maintain (though having a class hierarchy would ensure the APIs were actually identical, whereas currently they're only identical by convention).

@dblaikie indeed I do!
I have some work on https://github.com/ckissane/llvm-project/tree/ckissane.compression-class-simple that I will put into a patch shortly!
It does exactly what you said, makes a base class, a none class, and zstd and zlib classes for compression, and refactors code to take advantage of this.
https://reviews.llvm.org/D130516

[MaskRay]

In D130506#3677379, @ckissane wrote:

In D130506#3677033, @dblaikie wrote:

@ckissane sounded like you/your colleagues already had some outstanding patches/plans to generalize the compression support - should we be considering those patches/directions alongside this one?

I'd had in mind something like a compression algorithm base class, two (or three, if it's useful to have a default implementation for "None") derived classes, singleton instances of each and a function that takes the algorithm type and returns the handler - which can then be queried for "is available" and provides compress/decompress actions. That'd mean having only one switch instead of three, but the three are now so close together that it's not hugely painful to maintain (though having a class hierarchy would ensure the APIs were actually identical, whereas currently they're only identical by convention).

@dblaikie indeed I do!
I have some work on https://github.com/ckissane/llvm-project/tree/ckissane.compression-class that I will put into a patch shortly!
It does exactly what you said, makes a base class, a none class, and zstd and zlib classes for compression, and refactors code to take advantage of this.

(You may want to use rebase instead of merge. Merge makes it a bit difficult to see the real changes.)

[jhenderson]

llvm/include/llvm/Support/Compression.h

30

Since there's more than one member, it should be plural.

32

I don't like the magic -1 here. I think it should either be stored in a constant, or possibly this should be an Optional.

[MaskRay]

llvm/include/llvm/Support/Compression.h

32

I have thought about Optional. I pick -1 to be more similar with *::compress's Level and avoid an Optional include. Happy to switch though.

[jhenderson]

llvm/include/llvm/Support/Compression.h

32

I'm not too fussed either way - I'd be happy with -1, if it were a named constant, e.g. DefaultLevel or something like that.

[MaskRay]

address comments

[dblaikie]

llvm/include/llvm/Support/Compression.h

40–47

It seems to me somewhat unfortunate that these will generally be called somewhat together (isAvailable then compress or isAvailable then decompress) but don't share state/end up switching over compression type, etc, twice.

I've mentioned on the other reviews (maybe we could/should have a discourse thread about this, rather than spreading conversation between alternatives, preliminary patches, etc) that I think something like:

CompressionAlgorithm *C = getCompressionAlgorithm(DebugCompressionType::Zlib);
if (C) {
  C->compress(level, input, output);
}

Would be pretty good to me - possible variant (mentioned on one of the other reviews) would be that this returns non-null even if the algorithm isn't available (in which case it could return by reference) so it can communicate other things (like the name of the build flag needed to enable that algorithm - so it can be written/mapped from the enum in one place, and used in various error messages for various tools, etc)

llvm/lib/Support/Compression.cpp

46

llvm_unreachable should be preferred over assert false ( https://llvm.org/docs/CodingStandards.html#assert-liberally discusses this)

[MaskRay]

llvm/include/llvm/Support/Compression.h

40–47

I posted a small-write up here:
https://reviews.llvm.org/D130516#3688123

llvm/lib/Support/Compression.cpp

46

Thanks for mentioning the doc. For this place I hesitated while thinking about llvm_unreachable but picked assert because the user can call the function with any arguments and P.Format==DebugCompressionType::None reachable. The function is supposed to be unreachable from call sites, though...

[dblaikie]

llvm/lib/Support/Compression.cpp

46

In the same way that a user can call a function that takes a non-null pointer with a null pointer, resulting in an assertion - that's OK. It's an invariant of the function that P.Format != DebugCompressionType::None, so it's OK/suitable to use unreachable. (using an assert and then having some code "in case the assert fails" is inconsistent - it's defining behavior when an invariant is violated, but that behavior is then untested and unreliable)

But, yeah, guess we can leave this discussion for now while we resolve the larger design discussions.

[MaskRay]

redesign

[ckissane]

Looks like my concerns about the enum coupling and none vs unsupported semantics are addressed, one nit though.
LGTM, however, @dblaikie may want to take a pass as well. (Though I think this should be fine with him, because even if the final state of compression api in llvm is more OO, this regardless moves it closer in that direction than the original api.)

llvm/include/llvm/Support/Compression.h

26–27

I think a renaming would be good to keep this extra clear.
Not a blocker though
it could also be good to keep this enum closer to elf-related code.

[ckissane]

llvm/include/llvm/Support/Compression.h

26–27

Note that you state:

Move llvm::DebugCompressionType from MC to Support to avoid Support->MC cyclic
dependency. There are 40+ uses in llvm-project.

would moving both this enum and

inline Format formatFor(DebugCompressionType Type)

into MC also resolve this?

[MaskRay]

llvm/include/llvm/Support/Compression.h

26–27

The DebugCompressionType rename change is what I want to avoid. The type may be reused by another object file format and there are ~40 uses in llvm-project which would need updating.

MC depends on Support, so Support cannot depend on MC (cyclic). The type is therefore moved to the lower level: Support.

[dblaikie]

I still have reservations, but the discussion doesn't seem productive, so let's go with this. Some minor tweaks to make, if you like.

llvm/include/llvm/Support/Compression.h

26–27

I think it's probably reasonable to understand DebugCompressionType as an API level enum, not necessarily 1:1 with the actual value in the ELF format - it probably wouldn't be a bad idea to separate those at some point, but not the end of the world.

95–98

I don't think it's worth adding this type - DebugCompressionType seems fine, and some functions that consume it (like compress/decompress) can have a precondition that it is not None for those functions.

111–123

Not sure if this structure is carrying its weight? The level would only be a parameter to compress and not decompress, right? (so decompress should only take Format, not Params (& maybe just take DebugCompressionKind if Format is dropped)) & then if the struct is only used to group two parameters (format and level) for one function call, maybe that's not worth it and we can just take two parameters in compress?

[MaskRay]

llvm/include/llvm/Support/Compression.h

95–98

A separate type was @ckissane @leonardchan @phosek 's request.

(My original change was to reuse DebugCompressionType for all llvm-project compression. I just did not rename it.)

I think with the current code, this is a small-enough difference. Even if we decide to merge the types, the use sites will get minimum updates (just renaming).

111–123

I agree Level is not useful for decompress. Let me use Format for decompress.

I will keep this structure because compress will share another parameter expressing the degree of concurrency, so its 3 parameters for compress.

[MaskRay]

simplify decompress as suggested

[MaskRay]

llvm/include/llvm/Support/Compression.h

95–98

In offline chat with dblaikie:

"""
So the ELF uses would need a new enum that only provides Zstd/Zlib, and the other users could have a wider enum with this new third compression type?

Yeah, fair enough I guess - I'm not super fussed about addressing that until we cross that bridge I suppose.
"""

Yes, that is the case. I consider this resolved and will land this soon:) Thanks for being open.

[mstorsjo]

This broke compilation with GCC:

In file included from ../include/llvm/MC/MCTargetOptions.h:13,
                 from ../include/llvm/MC/MCAsmInfo.h:20,
                 from ../lib/MC/MCAsmStreamer.cpp:15:
../include/llvm/Support/Compression.h:100:10: error: declaration of ‘llvm::compression::Format llvm::compression::Params::Format’ changes meaning of ‘Format’ [-fpermissive]
  100 |   Format Format;
      |          ^~~~~~
../include/llvm/Support/Compression.h:78:12: note: ‘Format’ declared here as ‘enum class llvm::compression::Format’
   78 | enum class Format {
      |            ^~~~~~

llvm/include/llvm/MC/MCTargetOptions.h

 //===- MCTargetOptions.h - MC Target Options --------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_MC_MCTARGETOPTIONS_H
 #define LLVM_MC_MCTARGETOPTIONS_H
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/Compression.h"
 #include <string>
 #include <vector>
 
 namespace llvm {
 
 enum class ExceptionHandling {
   None,     ///< No exception support
   DwarfCFI, ///< DWARF-like instruction based exceptions
   SjLj,     ///< setjmp/longjmp based exceptions
   ARM,      ///< ARM EHABI
   WinEH,    ///< Windows Exception Handling
   Wasm,     ///< WebAssembly Exception Handling
   AIX,      ///< AIX Exception Handling
 };
 
-enum class DebugCompressionType {
-  None, ///< No compression
-  Z,    ///< zlib style complession
-};
-
 enum class EmitDwarfUnwindType {
   Always,          // Always emit dwarf unwind
   NoCompactUnwind, // Only emit if compact unwind isn't available
   Default,         // Default behavior is based on the target
 };
 
 class StringRef;
 

llvm/include/llvm/Support/Compression.h

 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/DataTypes.h"
 
 namespace llvm {
 template <typename T> class SmallVectorImpl;
 class Error;
 
+enum class DebugCompressionType {
+  None, ///< No compression
+  Z,    ///< zlib
+  Zstd, ///< Zstandard
+};
+
 namespace compression {
+struct Param {
+  DebugCompressionType Format = DebugCompressionType::None;
+  int Level = -1;
+  // This may support multi-threading for zstd in the future. Note that
+  // different threads may produce different output, so be careful if certain
+  // output determinism is desired.
+};
+
+// Return true if LLVM was built with support (LLVM_ENABLE_ZLIB,
+// LLVM_ENABLE_ZSTD) for the specified compression format.
+bool isAvailable(DebugCompressionType Type);
+
+// Compress Input with the specified format P.Format. If Level is -1, use
+// *::DefaultCompression for the format.
+void compress(Param P, ArrayRef<uint8_t> Input,
+              SmallVectorImpl<uint8_t> &Output);
+
+// Decompress Input. The uncompressed size must be available.
+Error uncompress(Param P, ArrayRef<uint8_t> Input,
+                 SmallVectorImpl<uint8_t> &Output, size_t UncompressedSize);
+
 namespace zlib {
 
 constexpr int NoCompression = 0;
 constexpr int BestSpeedCompression = 1;
 constexpr int DefaultCompression = 6;
 constexpr int BestSizeCompression = 9;
 
 bool isAvailable();
Error uncompress(ArrayRef<uint8_t> Input, uint8_t *UncompressedBuffer,
                  size_t &UncompressedSize);
 
 Error uncompress(ArrayRef<uint8_t> Input,
                  SmallVectorImpl<uint8_t> &UncompressedBuffer,
                  size_t UncompressedSize);
 
 } // End of namespace zstd
 
 } // End of namespace compression
 
 } // End of namespace llvm
 
 #endif

llvm/lib/ObjCopy/ELF/ELFObject.cpp

Error ELFSectionWriter<ELFT>::visit(const CompressedSection &Sec) {
   Elf_Chdr_Impl<ELFT> Chdr = {};
   switch (Sec.CompressionType) {
   case DebugCompressionType::None:
     std::copy(Sec.OriginalData.begin(), Sec.OriginalData.end(), Buf);
     return Error::success();
   case DebugCompressionType::Z:
     Chdr.ch_type = ELF::ELFCOMPRESS_ZLIB;
     break;
+  case DebugCompressionType::Zstd:
+    llvm_unreachable("unsupported");
   }
   Chdr.ch_size = Sec.DecompressedSize;
   Chdr.ch_addralign = Sec.DecompressedAlign;
   memcpy(Buf, &Chdr, sizeof(Chdr));
   Buf += sizeof(Chdr);
 
   std::copy(Sec.CompressedData.begin(), Sec.CompressedData.end(), Buf);
   return Error::success();

llvm/lib/Support/Compression.cpp

 #endif
 #if LLVM_ENABLE_ZSTD
 #include <zstd.h>
 #endif
 
 using namespace llvm;
 using namespace llvm::compression;
 
+bool compression::isAvailable(DebugCompressionType Format) {
+  switch (Format) {
+  case DebugCompressionType::None:
+    assert(false && "expected a compression type");
+    return false;
+  case DebugCompressionType::Z:
+    return zlib::isAvailable();
+  case DebugCompressionType::Zstd:
+    return zstd::isAvailable();
+  }
+}
+
+void compression::compress(Param P, ArrayRef<uint8_t> Input,
+                           SmallVectorImpl<uint8_t> &Output) {
+  switch (P.Format) {
+  case DebugCompressionType::None:
+    assert(false && "expected a compression type");
+    break;
+  case DebugCompressionType::Z:
+    zlib::compress(Input, Output,
+                   P.Level == -1 ? zlib::DefaultCompression : P.Level);
+    break;
+  case DebugCompressionType::Zstd:
+    zstd::compress(Input, Output,
+                   P.Level == -1 ? zstd::DefaultCompression : P.Level);
+    break;
+  }
+}
+
+Error compression::uncompress(Param P, ArrayRef<uint8_t> Input,
+                              SmallVectorImpl<uint8_t> &Output,
+                              size_t UncompressedSize) {
+  switch (P.Format) {
+  case DebugCompressionType::None:
+    assert(false && "expected a compression type");
+    return Error::success();
+  case DebugCompressionType::Z:
+    return zlib::uncompress(Input, Output, UncompressedSize);
+  case DebugCompressionType::Zstd:
+    return zstd::uncompress(Input, Output, UncompressedSize);
+  }
+}
+
 #if LLVM_ENABLE_ZLIB
 
 static StringRef convertZlibCodeToString(int Code) {
   switch (Code) {
   case Z_MEM_ERROR:
     return "zlib error: Z_MEM_ERROR";
   case Z_BUF_ERROR:
     return "zlib error: Z_BUF_ERROR";

llvm/unittests/Support/CompressionTest.cpp

 #if LLVM_ENABLE_ZLIB
 static void testZlibCompression(StringRef Input, int Level) {
   SmallVector<uint8_t, 0> Compressed;
   SmallVector<uint8_t, 0> Uncompressed;
   zlib::compress(arrayRefFromStringRef(Input), Compressed, Level);
 
   // Check that uncompressed buffer is the same as original.
   Error E = zlib::uncompress(Compressed, Uncompressed, Input.size());
-  consumeError(std::move(E));
+  EXPECT_FALSE(std::move(E));
+  EXPECT_EQ(Input, toStringRef(Uncompressed));
 
+  // uncompress with Z dispatches to zlib::uncompress.
+  Param P;
+  P.Format = DebugCompressionType::Z;
+  E = compression::uncompress(P, Compressed, Uncompressed, Input.size());
+  EXPECT_FALSE(std::move(E));
   EXPECT_EQ(Input, toStringRef(Uncompressed));
+
   if (Input.size() > 0) {
     // Uncompression fails if expected length is too short.
     E = zlib::uncompress(Compressed, Uncompressed, Input.size() - 1);
     EXPECT_EQ("zlib error: Z_BUF_ERROR", llvm::toString(std::move(E)));
   }
 }
 
 TEST(CompressionTest, Zlib) {

 #if LLVM_ENABLE_ZSTD
 static void testZstdCompression(StringRef Input, int Level) {
   SmallVector<uint8_t, 0> Compressed;
   SmallVector<uint8_t, 0> Uncompressed;
   zstd::compress(arrayRefFromStringRef(Input), Compressed, Level);
 
   // Check that uncompressed buffer is the same as original.
   Error E = zstd::uncompress(Compressed, Uncompressed, Input.size());
-  consumeError(std::move(E));
+  EXPECT_FALSE(std::move(E));
+  EXPECT_EQ(Input, toStringRef(Uncompressed));
 
+  // uncompress with Zstd dispatches to zstd::uncompress.
+  Param P;
+  P.Format = DebugCompressionType::Zstd;
+  E = compression::uncompress(P, Compressed, Uncompressed, Input.size());
+  EXPECT_FALSE(std::move(E));
   EXPECT_EQ(Input, toStringRef(Uncompressed));
+
   if (Input.size() > 0) {
     // Uncompression fails if expected length is too short.
     E = zstd::uncompress(Compressed, Uncompressed, Input.size() - 1);
     EXPECT_EQ("Destination buffer is too small", llvm::toString(std::move(E)));
   }
 }
 
 TEST(CompressionTest, Zstd) {