Page MenuHomePhabricator

[Support] Change zlib::compress to return void
ClosedPublic

Authored by MaskRay on Mar 11 2022, 9:46 PM.

Details

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.

Diff Detail

Event Timeline

MaskRay created this revision.Mar 11 2022, 9:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2022, 9:46 PM
MaskRay requested review of this revision.Mar 11 2022, 9:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2022, 9:46 PM

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

joerg added a subscriber: joerg.Mar 12 2022, 1:22 PM

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 added a comment.EditedMar 12 2022, 1:39 PM

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 added a comment.Mar 12 2022, 3:31 PM

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.

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 added a comment.Mar 12 2022, 4:03 PM

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 updated this revision to Diff 414896.Mar 12 2022, 4:56 PM
MaskRay retitled this revision from [Support] zlib::compress: replace compress2 with iterative deflate to [Support] Change zlib::compress to return void.
MaskRay edited the summary of this revision. (Show Details)

repurpose the patch to return void

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 12 2022, 4:56 PM
ikudrin added inline comments.Mar 14 2022, 10:53 AM
llvm/lib/ObjCopy/ELF/ELFObject.cpp
561

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

MaskRay updated this revision to Diff 415167.Mar 14 2022, 11:13 AM
MaskRay marked an inline comment as done.

Simplify CompressedSection::CompressedSection in llvm-objcopy.

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

This revision is now accepted and ready to land.Mar 14 2022, 11:17 AM
This revision was landed with ongoing or failed builds.Mar 14 2022, 11:38 AM
This revision was automatically updated to reflect the committed changes.