[llvm-objcopy] Adding support for compressed DWARF debug sections.

[plotfi]

Summary

First attempt at compressing debug sections with zlib-gnu and zlib. Right now it seems to work fine with gcc generated sections.
Just want some overall design feedback.

This patch only adds support for compressing (--compress-debug-sections=[ none, zlib, zlib-gnu ]).

[plotfi]

tools/llvm-objcopy/Object.cpp

190

Base class has no assignment method. It'll try and add one. Also base class has no constructor because it's abstract I believe.

tools/llvm-objcopy/Object.h

17

You can not. Usage of DebugCompressionType::GNU etc makes this not possible unless you want to copy the enum class (which I would like not to).

19–21

Still needs Compression.h

376

Jake wanted to always set it. I can not set it for the gnu case. However, gnu objcopy worked in gnu more.

tools/llvm-objcopy/llvm-objcopy.cpp

416

I had changed it because in some intermediate patches there was no compression going on at this stage (it was happening in the visitor).

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

173

It doesn't free that memory usually but you should probably act is if you can't be sure since there is no invariant that says that OriginalData must come from the original binary. For instance it should in theory be perfectly valid to compress a compressed section and in that case this would *not* be allowed.

Either way, I don't think OriginalData should be the uncompressed data. It should be compressed data since that was the effective data at creation time. This solves the lifetime issue and is, IMO, more sensible anyhow.

tools/llvm-objcopy/Object.h

365

I don't think you need this Data. OriginalData should be what CompressedData is.

376

I think James has this right actully. I find James's proposed case a very likely case. We should leave that alone in the GNU case.

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

190

@jhenderson There is a generic issue with the fragility of cloning the fields of other sections for the purposes of synthetic sections. So far we've kind of punted on this issue. (for instance, OwnedDataSection and DebugGnuLinkSection or whatever its called).

I think the defined operator is not the right way to go if C++ won't define that method for us. I think we should think about it more as "what fields need to be set right here and now". NameIndex for instance does not need to be set. I don't know...it's kind of unclear how this should work to me. Maybe a method that isn't '=' but is for copying the base fields that are always set that is inside the SectionBase class? Or perhaps the previous "fragile" solution is actually best with a few tweaks. Both seem equally fragile to me personally unless the compiler defines the method for us.

[jhenderson]

I'm liking the look of this now. Still got a few minor things still to work on though.

test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test

8–9

Is there a particular reason you aren't doing the two checks in the same run of FileCheck?

21

Remove this line.

33

Don't do it this way. You can use the section size field directly (available in the sections dump from earlier), rather than extracting the sections.

tools/llvm-objcopy/Object.cpp

190

I think the use of the default copy constructor (as done by @plotfi now) does the trick - it means that everything will be initialised, regardless of how many fields are added.

tools/llvm-objcopy/Object.h

17

You aren't using them in the header file, as far as I can see. Move the include into the .cpp if it is needed there. Same goes for Compression.h.

tools/llvm-objcopy/llvm-objcopy.cpp

418

SectionsToRemove -> SectionsToCompress

419

Use std::copy_if and std::back_inserter to do this. It looks a bit cleaner, in my opinion.

920

I'm not sure this is quite what you want. I think this means that the following command-line will be accepted but do no compression:

llvm-objcopy foo.o --compress-debug-sections

You probably want to use hasArg and\or change the default if there is no argument to --compress-debug-sections (e.g. --compress-debug-sections with no argument is the same as --compress-debug-sections=zlib).

[plotfi]

tools/llvm-objcopy/Object.cpp

190

Alex and I figured out how to get C++ to give us a proper default field copy constructor.

tools/llvm-objcopy/Object.h

365

But who owns this original data? It comes from another section that will be removed. Will make the change.

376

It's changed to set the flag only in the non-gnu case in the latest diff.

376

sounds good

tools/llvm-objcopy/llvm-objcopy.cpp

419

I agree, except this is not easy to do because Obj.sections() are unique_ptrs. I could use a move_iterator but we use RemovePreds to remove items from the sections vector.

[plotfi]

tools/llvm-objcopy/llvm-objcopy.cpp

419

I think ideally we'd want a transform_if but that doesn't exist.

[plotfi]

Added fixup for and testing for relocation sections.

[jakehehrlich]

This is looking pretty close to good to me. I have a couple more nits about how alignment and RemovePred should work but other than that I think I'm good. If those are fixed and someone else accepts this revision before I get back to this feel free to land it without further input from me.

tools/llvm-objcopy/Object.cpp

163

You can make the alignment of this section smaller by storing the alignment of the uncompressed section as a separate field and then just using an alignment of 8 for Sec.Align in the constructor. That will help decrease the size of most fields and avoid unaligned reads/writes for Chdr for sections that have less than 8 byte alignment.

For instance if a section had page alignment (I can't imagine that happening) you'd save up to almost a page of space by setting the alignment smaller. For debug sections I don't think it will save anything or generally even avoid unaligned reads/writes but it's my preference either way.

tools/llvm-objcopy/Object.h

365

Generally OriginalData is actully owned by the binary file and not any particular section. CompressedSection and OwnedDataSection are the only two for which that shouldn't always be the case and that's only true as of this change. For debug sections it should always be true but it's probably best we not relay on that.

tools/llvm-objcopy/llvm-objcopy.cpp

434

I think we can do this more efficiently now that we don't remove only if space is actually improved.

[jhenderson]

test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test

8–9

Make that four... I think you should be able to simplify the number of runs down by using multiple switches in the same command-line (e.g. -relocations and -s in llvm-readobj).

14

Could you also check the Info field, please, to show which section you now refer to.

tools/llvm-objcopy/Object.h

17

You should still forward declare the enum in the header and include MCTargetOptions.h in the cpp file.

tools/llvm-objcopy/llvm-objcopy.cpp

428

This is a bad way to do this. It means you'll iterate over the whole set of sections once for each compressed section again, just looking for a specific relocation section that may not even exist.

I think the better way to do it would be to make your vector a map, mapping relocation sections to their debug section, and then it can be done in the same loop as above, where you are already iterating over all sections. Something like this partial pseudo-code:

Map<SectionBase *, SmallVector<SectionBase*, 1>> ToCompress;
for (auto &Sec : Obj.sections()) {
  if (shouldCompress(Sec))
    ToCompress.insert(Sec);
  else if (auto RelocSec = dyn_cast<RelocationSection>(Sec)) {
    auto TargetSection = RelocSec->getSection();
    if (shouldCompress(TargetSection))
      ToCompress[TargetSection].push_back(RelocSection);
  }
}

Two things to watch out for: 1) it is technically legal to have multiple relocation sections targeting the same section, so it needs to be a vector not a single section. 2) It is possible for the relocation section to be before or after the target section in the section list.

429

This is one case where you can use auto, because the type is obvious from the same line of code.

434

I'm guessing you mean this should be simply return (isDebugSection(Sec) && Sec.Name != ".gdb_index") || RemovePred(Sec) and moved out of the loop. That's what I'd do.

[alexander-shaposhnikov]

tools/llvm-objcopy/Object.h

17

@jhenderson - khm, if we forward declare the enum the size of the enum is unknown (if the definition is not available), consequently, every single .cpp which includes Object.h will have to include MCTargetOptions.h. Just curious - what are the benefits of this / are there any pointers to the llvm style guide or any other docs suggesting this ? that would look kinda broken (one header (Object.h) implicitly depends on another one and can not be included without causing errors into an empty .cpp).

[alexander-shaposhnikov]

okay, I agree with James' and Jake's comments (except the comment regarding the enum, although maybe i'm missing smth), but to me this otherwise looks good. Hopefully those comments can be addressed + please, wait for the approval from Jake and James.

[plotfi]

test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test

8–9

Different tools?

tools/llvm-objcopy/Object.cpp

163

So Chdr->ch_addralign = original alignment
Sec.Align = 8

??

tools/llvm-objcopy/llvm-objcopy.cpp

428

For now I've added a more simple solution. In the top-level loop I collect the relocation sections that point to compresable debug sections and then in the next loop's inner loop I check for the relocation.

[jhenderson]

Unfortunately, I'm going to be on annual leave as of next week, so I won't be in a position to review anything further until a week on Monday. As long as my outstanding points are addressed, I don't mind somebody else giving approval. I'll take a look at the final version when I get back, and can always request post-commit changes. Just make sure to include the "Differential Revision" part in the commit message so that this diff gets automatically updated with the committed version.

test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test

8–9

I missed that originally, but you still don't need two calls to each tool. One each would suffice, assuming that the information we want isn't available in any single dump available via one of those. For example, you can do:

# RUN: llvm-readobj -relocations -s %t-compressed.o

to get the relocations and section headers.

33

I think it probably makes more sense to get the section size and compare that, rather than the overall file size. You can drop the "wc" calls that way.

Don't do it this way. You can use the section size field directly (available in the sections dump from earlier), rather than extracting the sections.

Why have you marked these as done? They aren't done.

tools/llvm-objcopy/Object.h

17

if we forward declare the enum the size of the enum is unknown (if the definition is not available), consequently, every single .cpp which includes Object.h will have to include MCTargetOptions.h.

Not true. This is an enum class which can be forward declared (defaults to int size unless explicitly stated). See https://en.cppreference.com/w/cpp/language/enum. Only clients of the code that actually want to access an enumeration value need to include the header.

It's bad practice to include more headers than needed in general, due to the increased compile time cost, and also creation of implicit dependencies that later could result in broken builds.

tools/llvm-objcopy/llvm-objcopy.cpp

428

I suppose this works, and it shouldn't cause a performance issue overall, but it seems clunky to do it this way, since you already have this information calculated in the first loop. You're just recalculating it again effectively.

[alexander-shaposhnikov]

tools/llvm-objcopy/Object.h

17

@jhenderson

1. Not true.

alexshap-mbp:~ alexshap$ cat main.cpp
enum F;
int main() {

return sizeof(F);

}
alexshap-mbp:~ alexshap$ clang -std=c++11 main.cpp -o main.exe
main.cpp:1:6: error: ISO C++ forbids forward references to 'enum' types
enum F;

^

main.cpp:3:10: error: invalid application of 'sizeof' to an incomplete type 'F'

return sizeof(F);
       ^     ~~~

main.cpp:1:6: note: forward declaration of 'F'
enum F;

^

2 errors generated.

2. Nobody suggested including more headers than needed, my point was different,

it seems like it would be strange if whenever someone wants to include <vector> and use smth from it (for example, sizeof(std::vector<int>)) he has to include <map> as well.

[alexander-shaposhnikov]

tools/llvm-objcopy/Object.h

17

I see, "enum class" is different, in this case it can be forward declared, I didn't notice that.
Yeah, in this case it's appropriate and @jhenderson is right.

[plotfi]

test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test

33

Missed what you meant in your comment. Sorry.

tools/llvm-objcopy/Object.h

17

This will force me to include #include "llvm/MC/MCTargetOptions.h" in both Object.cpp and llvm-objcopy.cpp FYI

[plotfi]

test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test

8–9

I hope this is reduced enough to your liking.

33

I believe the removed of these wc runs probably is what you would like. Let me know otherwise.

[plotfi]

tools/llvm-objcopy/llvm-objcopy.cpp

428

Yes, I know. Just keeping things simple.

[plotfi]

Adding alignment 8 for compressed sections.

[plotfi]

tools/llvm-objcopy/Object.cpp

163

I made changes addressing the alignment stuff you suggested. Let me know if this is what you had in mind.

[jakehehrlich]

I am satisfied that James' two remaining issues (as I understand them, more efficient looping in compressSections and minimal use of MCTargetOptions) have been solved and that mine have been solved.

Since this LGTM, James is going on leave, and Alex gave an LGTM, I think that means universal approval.

tools/llvm-objcopy/Object.cpp

173

LGTM

[phosek]

This change broke several LLVM builders as well as our downstream Linux and macOS toolchain builders, would it be possible to either quickly fix the issue or revert it?

[plotfi]

In D49678#1222498, @phosek wrote:

This change broke several LLVM builders as well as our downstream Linux and macOS toolchain builders, would it be possible to either quickly fix the issue or revert it?

Have your builders picked up r341343? Some of the ubsan failures (and I suspect others) came from an unaligned memory write that I hadn't caught locally which is fixed now. If your builders still fail after this go ahead and revert and let me know. Sorry about this.

[chandlerc]

In D49678#1222500, @plotfi wrote:

In D49678#1222498, @phosek wrote:

This change broke several LLVM builders as well as our downstream Linux and macOS toolchain builders, would it be possible to either quickly fix the issue or revert it?

Have your builders picked up r341343? Some of the ubsan failures (and I suspect others) came from an unaligned memory write that I hadn't caught locally which is fixed now. If your builders still fail after this go ahead and revert and let me know. Sorry about this.

No, all configurations are now broken for me as well, which is disrupting my development....

[chandlerc]

There are bots obviously still failing so I'm going to revert....

http://lab.llvm.org:8011/builders/clang-hexagon-elf/builds/19789/
http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/5822/

And note that the aarch64 bot is still failing:
http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/5823/

Not sure why the hexagon went green?

[chandlerc]

Reverted temporarily in r341360 to unbreak my build and the aarch64 build bot. Let me know if I can help with the investigation in any way.

[jakehehrlich]

Ah so I think this might be an issue with builders that don't build with zlib or that link zlib in in strange ways. I remember I mentioned (many weeks ago) that we'd need to #ifdef some of this stuff into compartments so that it didn't affect non zlib builds. I didn't remember to check for that before giving the LGTM. That is certainly killing at least some bots. I think the downstream bots that Petr mentioned are however actually linking in zlib. It isn't clear why this is breaking those but I'll try and look into that today.

[plotfi]

In D49678#1223480, @jakehehrlich wrote:

Ah so I think this might be an issue with builders that don't build with zlib or that link zlib in in strange ways. I remember I mentioned (many weeks ago) that we'd need to #ifdef some of this stuff into compartments so that it didn't affect non zlib builds. I didn't remember to check for that before giving the LGTM. That is certainly killing at least some bots. I think the downstream bots that Petr mentioned are however actually linking in zlib. It isn't clear why this is breaking those but I'll try and look into that today.

Thanks. I tried building with -DLLVM_ENABLE_ZLIB=OFF (ubuntu x64) and the tests seem to not run and come out as "unsupported" The weird thing is, the test failures seem to indicate that the flag was not set properly (which I'd think wouldn't depend on zlib).

[plotfi]

In D49678#1223480, @jakehehrlich wrote:

Ah so I think this might be an issue with builders that don't build with zlib or that link zlib in in strange ways. I remember I mentioned (many weeks ago) that we'd need to #ifdef some of this stuff into compartments so that it didn't affect non zlib builds. I didn't remember to check for that before giving the LGTM. That is certainly killing at least some bots. I think the downstream bots that Petr mentioned are however actually linking in zlib. It isn't clear why this is breaking those but I'll try and look into that today.

FWIW, I tried running these tests on a raspberrypi. They pass. This is armv7 however.

[plotfi]

In D49678#1222836, @chandlerc wrote:

Reverted temporarily in r341360 to unbreak my build and the aarch64 build bot. Let me know if I can help with the investigation in any way.

@jakehehrlich @chandlerc I need help reproing this. I finally got an aarch64 setup up and running and the tests pass. I don't understand why this would fail.

I do think it may be possible that the Chdr copy should be changed to:

Elf_Chdr_Impl<ELFT> Chdr;
Chdr.ch_type = ELF::ELFCOMPRESS_ZLIB;
Chdr.ch_size = Sec.DecompressedSize;
Chdr.ch_addralign = Sec.DecompressedAlign;
memcpy(Buf, &Chdr, sizeof(Chdr));

to avoid any alignment bugs.

PL

[plotfi]

• added zlib::isAvailable() checks to various places before compressing.
• fixed up potential alignment issues with Chdr writes.

[rupprecht]

In D49678#1226534, @plotfi wrote:

In D49678#1222836, @chandlerc wrote:

Reverted temporarily in r341360 to unbreak my build and the aarch64 build bot. Let me know if I can help with the investigation in any way.

@jakehehrlich @chandlerc I need help reproing this. I finally got an aarch64 setup up and running and the tests pass. I don't understand why this would fail.

Looks like I'm able to repro this on my machine. (I hit it while I was running my own objcopy tests and happened to be synced to when this was recommitted). I can work with you on this.

I'm wrapping up for the day so I don't have time to go much further, but one thing I did notice that was really funny is there are multiple sections with the same name, e.g. for compress-debug-sections-zlib.test.tmp-compressed.o:

Section {
  Index: 1
  Name: .debug_foo (20)
  Type: SHT_PROGBITS (0x1)
  Flags [ (0x0)
  ]
  Address: 0x0
  Offset: 0x40
  Size: 8
  Link: 0
  Info: 0
  AddressAlignment: 0
  EntrySize: 0
}
Section {
  Index: 2
  Name: .debug_foo (20)
  Type: SHT_PROGBITS (0x1)
  Flags [ (0x800)
    SHF_COMPRESSED (0x800)
  ]
  Address: 0x0
  Offset: 0x48
  Size: 35
  Link: 0
  Info: 0
  AddressAlignment: 8
  EntrySize: 0
}

In other words, the FileCheck test that .debug_foo has the SHF_COMPRESSED flag is matching the wrong one. (Of course, it's not FileCheck that's wrong, it's the fact that there are somehow two sections with the same name).

I don't think there's anything unique about my machine setup... it's just a regular x86-64 linux machine, cmake is configured to use a near-head clang to build things... I hope to have time to take a closer look at this tomorrow.

[plotfi]

So with clang-6 I get:

Contents of section .zdebug_foo:
0000 5a4c4942 00000000 00000008 789c6360 ZLIB........x.c`
0010 80000000 080001 .......
Contents of section .notdebug_foo:
0000 00000000 00000000 ........
Contents of section .rela.debug_foo:
0000 01000000 00000000 0a000000 00000000 ................
0010 00000000 00000000 ........
Contents of section .symtab:
0000 00000000 00000000 00000000 00000000 ................
0010 00000000 00000000 ........
Contents of section .strtab:
0000 0000 ..
Contents of section .shstrtab:
0000 002e7a64 65627567 5f666f6f 002e6e6f ..zdebug_foo..no
0010 74646562 75675f66 6f6f002e 72656c61 tdebug_foo..rela
0020 2e646562 75675f66 6f6f002e 73687374 .debug_foo..shst
0030 72746162 002e7374 72746162 002e7379 rtab..strtab..sy
0040 6d746162 00 mtab.

but as you noted, with clang-8 TOT (in the GNU Case) I get:

Contents of section .debug_foo:
0000 00000000 00000000 ........
Contents of section .zdebug_foo:
0000 00000000 00000000 00000008 789c6360 ............x.c`
0010 80000000 080001 .......
Contents of section .notdebug_foo:
0000 00000000 00000000 ........
Contents of section .rela.debug_foo:
0000 01000000 00000000 0a000000 00000000 ................
0010 00000000 00000000 ........
Contents of section .symtab:
0000 00000000 00000000 00000000 00000000 ................
0010 00000000 00000000 ........
Contents of section .strtab:
0000 0000 ..
Contents of section .shstrtab:
0000 002e7a64 65627567 5f666f6f 002e6e6f ..zdebug_foo..no
0010 74646562 75675f66 6f6f002e 72656c61 tdebug_foo..rela
0020 2e646562 75675f66 6f6f002e 73687374 .debug_foo..shst
0030 72746162 002e7374 72746162 002e7379 rtab..strtab..sy
0040 6d746162 00000000 00000000 00000000 mtab............

Something here does not look right.

PL

Sent with ProtonMail Secure Email.

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

• publickey - [e-mail] - 0xD3F77200.asc1 KBDownload
• signature.asc522 BDownload

[alexander-shaposhnikov]

Side note: I would double check that nothing shady is going on with the comparator struct SectionCompare used by std::set. If it's broken or gets broken along the way a bunch of things can blow up

[plotfi]

In D49678#1226825, @rupprecht wrote:

In D49678#1226534, @plotfi wrote:

In D49678#1222836, @chandlerc wrote:

Reverted temporarily in r341360 to unbreak my build and the aarch64 build bot. Let me know if I can help with the investigation in any way.

@jakehehrlich @chandlerc I need help reproing this. I finally got an aarch64 setup up and running and the tests pass. I don't understand why this would fail.

Looks like I'm able to repro this on my machine. (I hit it while I was running my own objcopy tests and happened to be synced to when this was recommitted). I can work with you on this.

I'm wrapping up for the day so I don't have time to go much further, but one thing I did notice that was really funny is there are multiple sections with the same name, e.g. for compress-debug-sections-zlib.test.tmp-compressed.o:

Section {
  Index: 1
  Name: .debug_foo (20)
  Type: SHT_PROGBITS (0x1)
  Flags [ (0x0)
  ]
  Address: 0x0
  Offset: 0x40
  Size: 8
  Link: 0
  Info: 0
  AddressAlignment: 0
  EntrySize: 0
}
Section {
  Index: 2
  Name: .debug_foo (20)
  Type: SHT_PROGBITS (0x1)
  Flags [ (0x800)
    SHF_COMPRESSED (0x800)
  ]
  Address: 0x0
  Offset: 0x48
  Size: 35
  Link: 0
  Info: 0
  AddressAlignment: 8
  EntrySize: 0
}

In other words, the FileCheck test that .debug_foo has the SHF_COMPRESSED flag is matching the wrong one. (Of course, it's not FileCheck that's wrong, it's the fact that there are somehow two sections with the same name).

I don't think there's anything unique about my machine setup... it's just a regular x86-64 linux machine, cmake is configured to use a near-head clang to build things... I hope to have time to take a closer look at this tomorrow.

So, problem seems to be any usage of ArrayRef like this:

ArrayRef<uint8_t> Magic = {'Z', 'L', 'I', 'B'};

So where I did:

ArrayRef<uint8_t> Magic = {'Z', 'L', 'I', 'B'};
std::copy(Magic.begin(), Magic.end(), Buf);

or:

ArrayRef<uint8_t> GnuPrefix = {'Z', 'L', 'I', 'B'};

return StringRef(Section.Name).startswith(".zdebug") ||
       (Section.OriginalData.size() > strlen("ZLIB") &&
        std::equal(GnuPrefix.begin(), GnuPrefix.end(),
                   Section.OriginalData.data())) ||
       (Section.Flags & ELF::SHF_COMPRESSED);

clang8 would misbehave and I think this was improper usage of ArrayRef in the first place.
I'm surprised ubsan did not catch this.

[plotfi]

Closing. Everything is in. (I think I messed up the case for "Differential Revision" in my commit)