[Support] Change zlib::compress to return void

[MaskRay]

Summary

With a sufficiently large output buffer, the only failure is Z_MEM_ERROR.
Check it and call the noreturn report_bad_alloc_error if applicable.
resize_for_overwrite may call report_bad_alloc_error as well.

Since there is now no other error type, we can replace the return type
with void and simplify call sites.

[MaskRay]

Next, I'll change zlib::compress to return void and simplify the ~7 call sites.

[joerg]

compressBound gives the maximum possible size for the output, so of course it can be larger than the input. In combination with resize_for_overwrite, that should be cheaper than possible multiple copies for the loop if there is even a slight chance that the data doesn't compress that well. The code complexity is certainly much higher. The only expected error from compress2 here is Z_MEM_ERROR when it can't allocate the dictionary etc. As such, we could convert that one into a fatal error easily.

[MaskRay]

In D121512#3377419, @joerg wrote:

compressBound gives the maximum possible size for the output, so of course it can be larger than the input. In combination with resize_for_overwrite, that should be cheaper than possible multiple copies for the loop if there is even a slight chance that the data doesn't compress that well. The code complexity is certainly much higher. The only expected error from compress2 here is Z_MEM_ERROR when it can't allocate the dictionary etc. As such, we could convert that one into a fatal error easily.

The chance that 0.5*N needs reallocation is very slow (for some large executables, all input .debug_* sections can compress to 55%-. Most are smaller than 40%). In most cases the allocated buffer will save more than one half.

[joerg]

But that doesn't really matter much as long as it is uninitialized virtual memory? This is almost always used for a very short time as well.

[MaskRay]

In D121512#3377511, @joerg wrote:

But that doesn't really matter much as long as it is uninitialized virtual memory? This is almost always used for a very short time as well.

I agree this complexity is unneeded. Then abandon this patch and replace return Res ? createError(convertZlibCodeToString(Res)) : Error::success(); with something like

if (Res == Z_MEM_ERROR)
  report_bad_alloc_error("Allocation failed"); // like llvm/include/llvm/Support/MemAlloc.h safe_malloc, which may be called by resize_for_overwrite if oom
return Error::success();

?

[joerg]

Explicit check for Z_MEM_ERROR and assert that it is otherwise Z_OK. Other return values would point to a programming error in zlib.

[MaskRay]

repurpose the patch to return void

[joerg]

LGTM

[ikudrin]

llvm/lib/ObjCopy/ELF/ELFObject.cpp

568 ↗

(On Diff #414896)

OutErr can now be removed to further simplify the calling code.

[MaskRay]

Simplify CompressedSection::CompressedSection in llvm-objcopy.

CompressedSection::create can be further simplified, but I'll leave that as a future change.

[ikudrin]

LGTM

llvm/lib/Support/Compression.cpp

static StringRef convertZlibCodeToString(int Code) {
   case Z_OK:
   default:
     llvm_unreachable("unknown or unexpected zlib status code");
   }
 }
 
 bool zlib::isAvailable() { return true; }
 
-Error zlib::compress(StringRef InputBuffer,
-                     SmallVectorImpl<char> &CompressedBuffer, int Level) {
-  unsigned long CompressedSize = ::compressBound(InputBuffer.size());
-  CompressedBuffer.resize_for_overwrite(CompressedSize);
-  int Res =
-      ::compress2((Bytef *)CompressedBuffer.data(), &CompressedSize,
-                  (const Bytef *)InputBuffer.data(), InputBuffer.size(), Level);
+Error zlib::compress(StringRef In, SmallVectorImpl<char> &Out, int Level) {
+  z_stream s = {};
+  // Default parameters with custom Level.
+  deflateInit2(&s, Level, Z_DEFLATED, 15, 8, Z_DEFAULT_STRATEGY);
+  s.next_in =
+      const_cast<uint8_t *>(reinterpret_cast<const uint8_t *>(In.data()));
+  s.avail_in = In.size();
+  Out.resize_for_overwrite(std::max<size_t>(In.size() / 2, 64));
+  size_t Pos = 0;
+  do {
+    if (Pos == Out.size())
+      Out.resize_for_overwrite(Out.size() * 3 / 2);
+    s.next_out = reinterpret_cast<uint8_t *>(Out.data()) + Pos;
+    s.avail_out = Out.size() - Pos;
+    (void)deflate(&s, Z_FINISH);
+    Pos = s.next_out - reinterpret_cast<uint8_t *>(Out.data());
+  } while (s.avail_out == 0);
+  assert(s.avail_in == 0);
+  Out.truncate(Pos);
   // Tell MemorySanitizer that zlib output buffer is fully initialized.
   // This avoids a false report when running LLVM with uninstrumented ZLib.
-  __msan_unpoison(CompressedBuffer.data(), CompressedSize);
-  CompressedBuffer.truncate(CompressedSize);
-  return Res ? createError(convertZlibCodeToString(Res)) : Error::success();
+  __msan_unpoison(Out.data(), Pos);
+  return Error::success();
 }
 
 Error zlib::uncompress(StringRef InputBuffer, char *UncompressedBuffer,
                        size_t &UncompressedSize) {
   int Res =
       ::uncompress((Bytef *)UncompressedBuffer, (uLongf *)&UncompressedSize,
                    (const Bytef *)InputBuffer.data(), InputBuffer.size());
   // Tell MemorySanitizer that zlib output buffer is fully initialized.