This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] - SImplify `isCompressable` and fix the issue relative.
ClosedPublic

Authored by grimar on Mar 4 2019, 8:07 AM.

Details

Summary

When --compress-debug-sections is given, llvm-objcopy do not compress
sections that have "ZLIB" header in data. Normally this signature is used
in zlib-gnu compression format. But if zlib-gnu used then the name of the compressed
section should start from .z* (e.g .zdebug_info). If it does not, then it is not
a zlib-gnu format and section should be treated as a normal uncompressed section.

It is a probably unlikely case that some debug sections start from "ZLIB",
but I think we still want to be correct here and also this patch allows to simplify a logic
slightly.

Diff Detail

Event Timeline

grimar created this revision.Mar 4 2019, 8:07 AM

This is for llvm-objcopy, not llvm-objdump as claimed in the header and description, right?

This is for llvm-objcopy, not llvm-objdump as claimed in the header and description, right?

Oops. Right.

grimar retitled this revision from [llvm-objdump] - SImplify `isCompressable` and fix the issue relative. to [llvm-objcopy] - SImplify `isCompressable` and fix the issue relative..Mar 4 2019, 10:00 AM
grimar edited the summary of this revision. (Show Details)Mar 4 2019, 10:03 AM
grimar updated this revision to Diff 189161.Mar 4 2019, 10:07 AM
  • Fixed llvm-objdump -> llvm-objcopy in the test case.
grimar edited the summary of this revision. (Show Details)Mar 4 2019, 10:10 AM
MaskRay added inline comments.Mar 4 2019, 11:20 PM
test/tools/llvm-objcopy/ELF/compress-debug-sections-zlib-header.test
8

The description may be a bit verbose. I think we can just say that non-.zdebug_ sections whose data does not start with "ZLIB" should not be considered compressed.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp
230

isDebugSection(Section) also checks ".zdebug". How about just using Section.Name.startswith(".debug_") ?

grimar updated this revision to Diff 189284.Mar 5 2019, 12:54 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
test/tools/llvm-objcopy/ELF/compress-debug-sections-zlib-header.test
8

I reduced the comment, thanks!

tools/llvm-objcopy/ELF/ELFObjcopy.cpp
230

Actually yes, that would be better here I think. Applied your suggestion.

MaskRay accepted this revision.Mar 5 2019, 1:20 AM
This revision is now accepted and ready to land.Mar 5 2019, 1:20 AM
jhenderson added inline comments.Mar 5 2019, 2:06 AM
test/tools/llvm-objcopy/ELF/compress-debug-sections-zlib-header.test
11

"from" -> "with" here and the line below.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp
227–231

Now that you've changed this further, this can be even simpler, I think:

return !(Section.Flags & ELF::SHF_COMPRESSED) &&
           StringRef(Section.Name).startswith(".debug");
grimar updated this revision to Diff 189295.Mar 5 2019, 3:48 AM
grimar marked 3 inline comments as done.
  • Addressed review comments.
tools/llvm-objcopy/ELF/ELFObjcopy.cpp
227–231

Yep. Thanks!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2019, 5:07 AM