Page MenuHomePhabricator

[WIP] Add Zstd ELF support
Needs ReviewPublic

Authored by ckissane on Jun 27 2022, 10:05 AM.

Details

Summary
  • Would add ZSTD compression as a elf section compression option with enum value 2

Diff Detail

Event Timeline

ckissane created this revision.Jun 27 2022, 10:05 AM
ckissane requested review of this revision.Jun 27 2022, 10:05 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 27 2022, 10:05 AM
ckissane updated this revision to Diff 440296.Jun 27 2022, 10:15 AM

simplifed codeflow

ckissane edited the summary of this revision. (Show Details)Jun 27 2022, 10:26 AM
MaskRay added a comment.EditedJun 27 2022, 10:41 AM

The lld/ change should be separate. And it needs tests. We should have several tests such as mixed zlib and zstd input, malformed zstd input, compressed level, etc. OutputSections should not have a new member. The parallel output part will be tricky, too (see D117853). I can handle the lld/ELF part and leave other stuff to you.

llvm-objcopy should probably be a separate patch as well.

  • clang (Driver, cc1as) and MC (including llvm-mc --compress-debug-sections) in one patch
  • lld/ELF
  • llvm-objcopy

Context not available.

See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface "To make reviews easier, please always include as much context as possible with your diff!"

llvm/include/llvm/BinaryFormat/ELF.h
1772

Zstandard algorithm

llvm/lib/MC/ELFObjectWriter.cpp
149

Having two variables is ugly. Just pass MAI->compressDebugSections() as an argument.

884

if (ZstdStyle)

llvm/lib/ObjCopy/ELF/ELFObject.cpp
565

clang-format

llvm/lib/Object/Decompressor.cpp
24

clang-format

Please check the prevailing style first.

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
744

clang-format

1014

clang-format

ckissane updated this revision to Diff 440321.Jun 27 2022, 11:09 AM

remove extra struct member

ckissane updated this revision to Diff 440323.Jun 27 2022, 11:11 AM

add more diff context

The lld/ change should be separate. And it needs tests. We should have several tests such as mixed zlib and zstd input, malformed zstd input, compressed level, etc. OutputSections should not have a new member.

Got it, I have also removed the extra member.

The parallel output part will be tricky, too (see D117853). I can handle the lld/ELF part and leave other stuff to you.

Sounds good, and thank you for your offer to help!

llvm-objcopy should probably be a separate patch as well.

got it.

  • clang (Driver, cc1as) and MC (including llvm-mc --compress-debug-sections) in one patch
  • lld/ELF
  • llvm-objcopy

Context not available.

See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface "To make reviews easier, please always include as much context as possible with your diff!"

Doing so now

Tips: you may run git diff -U0 --no-color --relative 'HEAD^' -- | path/to/llvm-project/clang/tools/clang-format/clang-format-diff.py -p1 -i to clang-format your local change.

lld/ELF/Driver.cpp
957
965
lld/ELF/InputSection.h
389

Wwe should not increase the size of InputSection which may have great impact on memory usage. So I mention that I will prefer handling the lld/ELF part.

ckissane marked 2 inline comments as done.Jun 27 2022, 11:27 AM
ckissane updated this revision to Diff 440343.Jun 27 2022, 11:51 AM

improve style on ELFWriter::maybeWriteCompression
fix typo in comment on zstd value for elf enum

ckissane marked 3 inline comments as done.Jun 27 2022, 11:52 AM
ckissane marked an inline comment as done.
MaskRay added a comment.EditedJun 27 2022, 11:59 AM

llvm-objcopy --decompress-debug-sections clang.zstd clang.zstd.uncompress doesn't work.

% /tmp/out/custom1/bin/llvm-objcopy --decompress-debug-sections clang.zstd clang.zstd.uncompress
/tmp/out/custom1/bin/llvm-objcopy: error: 'clang.zstd': '.debug_loc': zlib error: Z_DATA_ERROR

Add a new test llvm/test/tools/llvm-objcopy/compress-debug-sections-zstd.test and check existing tests for zlib (SHF_COMPRESSED).
I'll create a patch to remove .zdebug support to simplify the code.

Tip: you can nullify a compression/uncompression place, then run ninja check-llvm to find all places in llvm/ where a test may be relevant.

llvm-objcopy --decompress-debug-sections clang.zstd clang.zstd.uncompress doesn't work.

% /tmp/out/custom1/bin/llvm-objcopy --decompress-debug-sections clang.zstd clang.zstd.uncompress
/tmp/out/custom1/bin/llvm-objcopy: error: 'clang.zstd': '.debug_loc': zlib error: Z_DATA_ERROR

Add a new test llvm/test/tools/llvm-objcopy/compress-debug-sections-zstd.test and check existing tests for zlib (SHF_COMPRESSED).
I'll create a patch to remove .zdebug support to simplify the code.

Tip: you can nullify a compression/uncompression place, then run ninja check-llvm to find all places in llvm/ where a test may be relevant.

it seems that --decompress-debug-sections assumes always Z type compression. I can update the patch to fix this

Yeah, probably worth splitting this up a bit - somewhat hard to split up generation+parsing in LLVM itself (eg: llvm-mc+llvm-objdump), so maybe they're grouped together (alternatively the objdump support gets checked in first with precompiled/binary test files - I forget what I did when I added the compression support initially) - but lld at least can be separated out.

lld/ELF/InputSection.cpp
78–80

Perhaps we can delay this failure/error until the type of compressed section is parsed, then error if the specific kind of compression isn't available - rather than giving the user a more vague error here that doesn't tell them which kind of compression they need to be built into the linker.

(but yeah, if @MaskRay is going to take the lld support into a separate patch the feedback/discussion can happen there)

jhenderson added inline comments.Jun 27 2022, 11:48 PM
llvm/lib/ObjCopy/ELF/ELFObject.cpp
567–568

This StringRef construction is identical in both parts of the if, so should be pulled out of the if statement into a local variable.

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
1009–1019

This is identical (I think?) to a block further up in the file. Could we pull it out into a helper function, please? Something like the following:

Error checkCompressionAvailability() {
   /* moved code here */
}

// Usage would look like:
if (Error err = checkCompressionAvailability())
  return err;

The lld/ change should be separate. And it needs tests. We should have several tests such as mixed zlib and zstd input, malformed zstd input, compressed level, etc. OutputSections should not have a new member. The parallel output part will be tricky, too (see D117853). I can handle the lld/ELF part and leave other stuff to you.

llvm-objcopy should probably be a separate patch as well.

  • clang (Driver, cc1as) and MC (including llvm-mc --compress-debug-sections) in one patch
  • lld/ELF
  • llvm-objcopy

Context not available.

See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface "To make reviews easier, please always include as much context as possible with your diff!"

@MaskRay I would gladly accept your help on lld/ELF and I'll break this patch up into 2 patches:

  • clang (Driver, cc1as) and MC (including llvm-mc --compress-debug-sections) in one patch
  • llvm-objcopy

as suggested.
Also, I believe you requested a git repo for the whole picture on adding zstd: https://github.com/ckissane/llvm-project/tree/ckissane.add-zstd-compression

MaskRay added a comment.EditedMon, Jul 18, 12:37 PM

edit: ELFCOMPRESS_ZSTD https://groups.google.com/g/generic-abi/c/satyPkuMisk has been approved.

ckissane edited the summary of this revision. (Show Details)Wed, Jul 20, 1:09 PM
MaskRay added inline comments.Sun, Jul 24, 11:13 PM
llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
1015

For --decompress-debug-sections, the zstd error should be reported only when a compressed debug section using zstd is found. zlib is similar.

I cleaned up legacy zdebug code from llvm-objcopy, fixed an uninitialized ch_reserved bug. Created D130458 as an alternative patch.