[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 ]).

[jhenderson]

tools/llvm-objcopy/llvm-objcopy.cpp

430

I don't think we have a choice - it appears that GNU-style compression uses the name only as an indicator of compression. I could be persuaded that we should add the SHF_COMPRESSED flag in either case, since relying on section names is wrong and archaic, when we have other mechanisms to communicate the same thing, but we will at least have to be able to handle the case where a compressed section, compressed via GNU's tools, is fed to us.

[plotfi]

lib/Object/Compressor.cpp

17 ↗

(On Diff #161327)

Yeah, this is in the object library which is the same place Decompressor is.

tools/llvm-objcopy/llvm-objcopy.cpp

416

I think in this case, it's a little bit unreasonable to require this because the Decompressor library is not templated. I will go and refactor the Decompressor in another commit but the Decompressor library is used elsewhere in llvm and I do not want to change it in this commit but I also really would like to avoid duplicating what it does too. I want to implement support for decompression because it is handy for verifying that compression worked correctly. Can we have a compromise here, please?

430

GNU style uses the name, this is not my design choice.

446

In objcopy or elsewhere in llvm?

[jakehehrlich]

Ok so a few higher level comments

1. I think I agree with Alex that we shouldn't worry about compressing only if an improvement is shown. It's just far too difficult to do that at the moment. We can come back to that later. This seems to be a new case of "replace section" that I had been aware of the need for before but hand't properly implemented.
2. We should start simple and build up to parity and only build up to parity based on demand. In this case we shouldn't aim for full parity. Just start simple and if that isn't enough we can improve more later. If something isn't ideal that's ok for now.
3. Along those lines, I think we should handle compression and decompression in different diffs. We really need to rain in the complexity here.
4. The whole compressor decompressor thing in libObject feels misguided to me personally. Namely it isn't handling the subtle issue of size and endianess correctly (in fact, I think llvm-objcopy's whole handling of size is currently incorrect but this isn't helping). Luckily for you I have little to no say in the matter because this is outside of llvm-objcopy. Now that I realize this is in libObject (whooooops, my bad guys) I'd like that part to be in a separate change and reviewed by other people. You can cc me or even add me as a reviewer. If I were a reviewer for it I would like to see evidence for its need outside of llvm-objcopy since I don't think it's needed here.
5. This change is simply far more complex that it needs to be to accomplish the task. It's getting better and better though. We've been trying to make micro-pushes in the right direction, and we're getting there, but this really needs to be restructured somewhat instead. For instance compressSections and decompressSections are starting to look like something that should go in the final code, all the argument handling seems good to me as well. The means by which compressed sections are being handled is not in that shape. In order to express the massive change I think I'd like to see I've written the following code:

class CompressedSection {
private:
  uint64_t UncompressedSize;
  uint64_t UncompressedAlign;
  uint32_t Type;
  SmallVector<char, 0> CompressedData;
  bool GNU;
public:
  CompressedSection(Twine NewName, const SectionBase& ToCompress, bool GNU) : GNU(GNU) {
    Name = NewName;
    zlib::compress(toStringRef(ToCompress.OriginalData),  CompressedData);
    Size = CompressedData.size() + <some constant to account for size of header> + (GNU ? 4 : 0); // Needed mostly because I didn't handle Size correctly in llvm-objcopy. Sorry. I plan on getting back to llvm-objcopy to fix many things this included in a few weeks.
    Flags = ToCompress.Flags | SHF_COMPRESS;
    Align = 8; // Also I think alignment also changes depending on size here and an alignment of 4 would be better for the 32-bit case.
    // Assign all the other things from ToCompress except size and align
    UncompressedSize = ToCompress.Size;
    UncompressedAlign = ToCompress.Align;
  }
}

...

template <class ELFT>
void ELFSectionWriter<ELFT>::visit(const CompressedSection &Sec) {
  uint8_t *Buf = Out.getBufferStart();
  Buf += Sec.Offset;
  auto Chdr = reinterpret_cast<typename ELFT::Chdr*>(Buf);
  Chdr->ch_type = ELFCOMPRESS_ZLIB;
  Chdr->ch_size = Sec.UncompressedSize;
  Chdr->ch_align = Sec.UncompressedAlign;
  Buf += sizeof(*Chdr)
  if (Sec.GNU) {
    StringRef Magic = "ZLIB";
    Buf = std::copy(Magic.begin(), Magic.end(), Buf);
  }
  std::copy(Sec.CompressedData.begin(), Sec.CompressedData.end(), Buf);
}

This means you'll not need to mess with ELFT at all beyond that one function, it follows what we've been doing with these sorts of issues, and it eliminates the need for Compressor. Decompression is a different issue mind you. There is currently an issue with how reading works. I want to improve that. In a nutshell though it's going to need to be done in the ELFBuilder.

The major pain point here is just that size has to be overestimated due to poor design on my part. Align also seems to suffer from this issue which isn't something I was previously aware of. This should avoid the need for 90% of the code written.

lib/Object/Compressor.cpp

17 ↗

(On Diff #161327)

Ohhhh. Yeah I think I want to put the kibosh on doing this in this change. For the purposes of llvm-objcopy and this change I don't think we want or need this. If this is truly needed at large then someone else should review it.

tools/llvm-objcopy/llvm-objcopy.cpp

416

My preference would be to not use the whole Compressor thing. If the CompressedSection type handles these internally then ELFWriter has zero issues handeling this and the normal compression code that already exists in llvm will handle it.

430

We have the option to just not support this to be honest. Compressed sections don't really even have a use case as far as I'm concerned. That said I'll not worry about this. That seems like an unfortunate but necessary conclusion of supporting this.

439

Ah I see now. In that case I now concur that we should do what you said and add the sections to be added to a vector and add them at the end. Thanks!

446

I don't seem to be able to mark my own comment as done. But consider this one done.

477

I see. I have now gone though a similar set of realizations. We should probably just compress unconditionally.

[plotfi]

In D49678#1209775, @jakehehrlich wrote:

Ok so a few higher level comments

1. I think I agree with Alex that we shouldn't worry about compressing only if an improvement is shown. It's just far too difficult to do that at the moment. We can come back to that later. This seems to be a new case of "replace section" that I had been aware of the need for before but hand't properly implemented.
2. We should start simple and build up to parity and only build up to parity based on demand. In this case we shouldn't aim for full parity. Just start simple and if that isn't enough we can improve more later. If something isn't ideal that's ok for now.
3. Along those lines, I think we should handle compression and decompression in different diffs. We really need to rain in the complexity here.
4. The whole compressor decompressor thing in libObject feels misguided to me personally. Namely it isn't handling the subtle issue of size and endianess correctly (in fact, I think llvm-objcopy's whole handling of size is currently incorrect but this isn't helping). Luckily for you I have little to no say in the matter because this is outside of llvm-objcopy. Now that I realize this is in libObject (whooooops, my bad guys) I'd like that part to be in a separate change and reviewed by other people. You can cc me or even add me as a reviewer. If I were a reviewer for it I would like to see evidence for its need outside of llvm-objcopy since I don't think it's needed here.
5. This change is simply far more complex that it needs to be to accomplish the task. It's getting better and better though. We've been trying to make micro-pushes in the right direction, and we're getting there, but this really needs to be restructured somewhat instead. For instance compressSections and decompressSections are starting to look like something that should go in the final code, all the argument handling seems good to me as well. The means by which compressed sections are being handled is not in that shape. In order to express the massive change I think I'd like to see I've written the following code:

class CompressedSection {
private:
  uint64_t UncompressedSize;
  uint64_t UncompressedAlign;
  uint32_t Type;
  SmallVector<char, 0> CompressedData;
  bool GNU;
public:
  CompressedSection(Twine NewName, const SectionBase& ToCompress, bool GNU) : GNU(GNU) {
    Name = NewName;
    zlib::compress(toStringRef(ToCompress.OriginalData),  CompressedData);
    Size = CompressedData.size() + <some constant to account for size of header> + (GNU ? 4 : 0); // Needed mostly because I didn't handle Size correctly in llvm-objcopy. Sorry. I plan on getting back to llvm-objcopy to fix many things this included in a few weeks.
    Flags = ToCompress.Flags | SHF_COMPRESS;
    Align = 8; // Also I think alignment also changes depending on size here and an alignment of 4 would be better for the 32-bit case.
    // Assign all the other things from ToCompress except size and align
    UncompressedSize = ToCompress.Size;
    UncompressedAlign = ToCompress.Align;
  }
}

...

template <class ELFT>
void ELFSectionWriter<ELFT>::visit(const CompressedSection &Sec) {
  uint8_t *Buf = Out.getBufferStart();
  Buf += Sec.Offset;
  auto Chdr = reinterpret_cast<typename ELFT::Chdr*>(Buf);
  Chdr->ch_type = ELFCOMPRESS_ZLIB;
  Chdr->ch_size = Sec.UncompressedSize;
  Chdr->ch_align = Sec.UncompressedAlign;
  Buf += sizeof(*Chdr)
  if (Sec.GNU) {
    StringRef Magic = "ZLIB";
    Buf = std::copy(Magic.begin(), Magic.end(), Buf);
  }
  std::copy(Sec.CompressedData.begin(), Sec.CompressedData.end(), Buf);
}

This means you'll not need to mess with ELFT at all beyond that one function, it follows what we've been doing with these sorts of issues, and it eliminates the need for Compressor. Decompression is a different issue mind you. There is currently an issue with how reading works. I want to improve that. In a nutshell though it's going to need to be done in the ELFBuilder.

The major pain point here is just that size has to be overestimated due to poor design on my part. Align also seems to suffer from this issue which isn't something I was previously aware of. This should avoid the need for 90% of the code written.

So your preference here is to taking code from the Compressor and sticking it in the visitor? I'm fine with altering the structure to always compress in the visitor rather than making a determination based on size improvements in HandleArgs but I would prefer if the Compressor library is still separate (which is templated by ELFT) so that I can use it to refactor redundant code in other parts of llvm.

lib/Object/Compressor.cpp

17 ↗

(On Diff #161327)

Alright, I will put the compressor library in a separate review diff. I do not want to write compression in llvm-objcopy because I see a lot of potential for refactoring redundant code all over llvm for this.

So what exactly is your preference specifically?

[jakehehrlich]

lib/Object/Compressor.cpp

17 ↗

(On Diff #161327)

It isn't up to me weather or not this goes into libObject. If you want to use it in this change then the final form of it that winds up in libObject may have use here, we'll see.

[plotfi]

lib/Object/Compressor.cpp

17 ↗

(On Diff #161327)

Ah fair. How about I add the Compressor class to objcopy to start, and move it out in a another review / commit?

[jakehehrlich]

lib/Object/Compressor.cpp

17 ↗

(On Diff #161327)

You'll have to convince me that adding the Compressor class here is better than what I've proposed to get my blessing. I'll also accept what Alex and James accept on that matter even if it contradicts what I think.

[plotfi]

This is a more stripped down more simple version of my patch.

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

167

I think this should be in the endieness if the target, not always big. If it should be big, then ELFT::Chdr should handle that. This is the sort of bug I'd like to avoid. Thus far we have had zero bugs with size and endianess. LLD has had similar success using the same strategy. Also, it's just what should be done to be consistent with the code base.

194

The instance on using Writer really makes this code worse IMO. The rest of llvm-objcopy does not do this and you don't have to do it here. This is all much simplified IMO if you just don't use Writer.

tools/llvm-objcopy/Object.h

333

That's not really necessarily true TBH. It's naive to think that just because you see a similar field that you should remove duplication.

337–338

Don't change this constructor. OriginalData should already work if you rebase.

349

Now that it has been decided on as the correct solution in various other reviews. Could you instead just convert SectionBase to use std::string?

353

Again, this is not how original data should be set. You don't need to use OwnedDataSection at all.

tools/llvm-objcopy/llvm-objcopy.cpp

416

The Decompressor library isn't necessarily useful here just FYI. Deduplication is a noble goal but if the abstraction doesn't exist or hasn't been implemented properly you shouldn't kludge a solution in to place. I'm not saying there are lots of kludges in llvm-objcopy that I want to fix. I'm just saying that hamfisting something that isn't working right is the wrong way to go about things, step back and request a redesign...or you know implement the solution that exists. I'm frankly skeptical that the kind of deduplication that you're trying to achieve with Decompressor is helpful.

[plotfi]

tools/llvm-objcopy/Object.cpp

167

For gnu style, as far as I’ve seen in every other place in llvm it’s always big endian.

194

This code all came from the Compressor library I originally wrote. I intend to refactor it all back into that library eventually. All similar code I found in llvm that writes out compressed sections uses the writer so that’s why it is written using Writer.

tools/llvm-objcopy/Object.h

337–338

I did rebase, I’ll double check.

tools/llvm-objcopy/llvm-objcopy.cpp

416

I’ve dropped decompressing in this patch. I’ve given up on that here for now. Forget it.

[plotfi]

This is much simpler version of the patch with no use of Writer or Compressor class. It only handles compressing, decompressing will come in another patch later.

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

156

Do we know that this is 64-bits even on 32-bit instances?

tools/llvm-objcopy/Object.h

368

The Size needs to be set in the constructor. Prior to all the fancy changes that people are making now. Most Sizes could be quite trivially determined. Size is used in layout. Because I didn't foresee Size needing to be determined dynamically for different systems, it's just a single variable when it really needs to be a virtual function that takes some magical argument that will allow it to know the proper size for a given output format. The issues here haven't been fully fleshed out but I think a solution that Alex and James proposed to solve other issues and not just this one)could be made to solve this. For the time being, the best option is to use an upper bound. It sucks; I dislike it, but I don't have a better recommendation.

I should finally get the time I need to refactor this in a couple weeks. I should be able to solve a lot of these overarching issues then.

In the meantime, I think this needs to compress the data within the section so that it can get a reasonable upper bound on the size.

376

Does it cause problems with other tools if we set SHF_COMPRESSED for GNU style? If not I think I'd like to set it regardless.

tools/llvm-objcopy/llvm-objcopy.cpp

293–294

I think find works from anywhere. I'd prefer if the behavior was still as it is with startswith. I think StringRef(Sec.Name).startswith(...) would work or in the other patch std::equal(S2.begin(), S2.end(), S1.begin()); was used with an auxiliary StartsWith function: https://reviews.llvm.org/D50463

either way I'd just prefer we keep the 'startswith' behavior

410

Can we add a check for "ZLIB" at the start of OriginalData if ".zdebug" is set but SHF_COMPRESSED is not?

416

Can this be a range based for loop?

592

ditto on starts with.

[plotfi]

tools/llvm-objcopy/Object.cpp

156

Yes. It’s alwaya 64bit big endian as I said in earlier comments.

https://gnu.wildebeest.org/blog/mjw/2016/01/13/elf-libelf-compressed-sections-and-elfutils/

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

156

Awesome. Acknowledged.

[plotfi]

tools/llvm-objcopy/Object.h

345

I was actually encouraged to inherit from OwnedDataSection in a previous review. What exactly should I do here? I need more clarity here in-order to move forward.

349

As far as I can tell, this is not comitted yet. Should I wait for this to land or take it from the diferential you sent?

353

I think I may just not use OwnedDataSection in implementing CompressedDataSection. It wasn't like that before.

362

This is what I had in previous diffs. Will change things back and leave OwnedDataSection the way it is.

368

Oh you're saying that CompressedSection::Size should be the compressed size, not the decompressed size??? At the moment I am doing compression in the visitor.

I still dont understand why size needs to be passed as a constructor parameter. Isn't that going to come from the Section I am passing in anyways?

376

it might, but I dont remember off the top of my head. I will try out setting it and using gnu objcopy to see what happens.

tools/llvm-objcopy/llvm-objcopy.cpp

293–294

Ah, good stuff. Alright, I wasn't sure if you would prefer StringReg(std::string) or not. Unfortunately all the nice startswith/endswith methods for std::string are in C++20.

[plotfi]

tools/llvm-objcopy/llvm-objcopy.cpp

700

Cool. Lets discuss something like this for a future patch. At the moment the only purpose of ElfType is to route the ELFT from executeElfObjcopyOnBinary to CreateWriter.

[jakehehrlich]

tools/llvm-objcopy/Object.h

345

I'll leave it to your judgement then. I don't have strong feelings about it. It is my estimation that using OwnedDataSection will make it so that it is more difficult to avoid unnecessary copies in data. Ideally I'd like to just have the input data as an ArrayRef that comes from the mapped file and then have the compression library write directly into the storage we use for CompressedSection. If that turns out not to be true then I'm more than happy for OwnedDataSection to be used. It might even be helpful to rework OwnedDataSection if turns out that I'm right. This is not the sort of issue that should block you.

349

Given that Paul hasn't worked out that and how close this patch is to getting an LGTM from me I think you should be the one to make that change in this patch.

368

Yep. To be precise. It's the field that becomes st_size in the section header so it has to be correct for that. Unfortunately what st_size is is not simply dependent on the size of compressed data but also on the size of of Chdr which you can't actually know the way things are. The current contract states that Size is the one field which must be correct at all times. This is because layout has to happen prior to finalization but layout needs to know the size. Thinking about it now, alignment actually must obey the same contract but that was less obvious since alignment is normally just unmodified.

There are two sizes that matter here, the size of the original data and the size after compression plus the header and magic string. Overestimating is acceptable, but not ideal.

You don't need to pass in the size of the original data since you already pass in the section and the OriginalData field contains that information. You *can't* pass in the compressed size because you don't yet know that information. Since Size must be set correctly on the exit of each method that also means that size must be correct after construction. So I don't really see how to get around compressing the data in the constructor. Plus I think that helps solve the problem of minimizing the number of copies that have to be made (namely, you can avoid ever copying the data)

[plotfi]

tools/llvm-objcopy/Object.h

349

I took a look at this patch (D50463). As far as I can tell the one addition is the StartsWith method. I am not sure we need it here unless you have a big preference to add it.

[plotfi]

tools/llvm-objcopy/Object.h

368

Problem is, I cant exactly get the new size without the ELFT template parameter. I could always over estimate based on the upper-bound of the compressed size.

[jakehehrlich]

tools/llvm-objcopy/Object.h

349

Sound fine to me. I think we use it enough places and will continue to use it that it might be helpful to have a function that does something like that for us. If you want to use StringRef(...).startswith(...) I'm also fine with that for now.

368

Right. I've been trying to call attention to that as a problem of how Size currently works. This is a fundamental design issue I overlooked that I plan to fix (and should be able to in a couple weeks). Overestimating by the difference between 32-bit and 64-bit Chdr isn't a huge loss. Overestimating by the difference between compressed and uncompressed is something I'd like to avoid.

[plotfi]

tools/llvm-objcopy/Object.h

368

Alright, my solution is probably going to look pretty rough for the size. Lemme know what you think of this:

+    Size = CompressedData.size();
+    // Increment by Chdr size.
+    if (isGnuStyle) {
+      constexpr size_t ChdrSize = sizeof("ZLIB") - 1 + sizeof(uint64_t);
+      Size += ChdrSize;
+    } else {
+      constexpr size_t ChdrSize =
+          max(max(sizeof(object::Elf_Chdr_Impl<object::ELF64LE>),
+                  sizeof(object::Elf_Chdr_Impl<object::ELF64BE>)),
+              max(sizeof(object::Elf_Chdr_Impl<object::ELF32LE>),
+                  sizeof(object::Elf_Chdr_Impl<object::ELF32BE>)));
+      Size += ChdrSize;
+    }

[plotfi]

Moved compression and size approximation (dont know ELFT so I have to do the max of all possible ELFT Chdr sizes) to the CompressedSection constructor.

[plotfi]

tools/llvm-objcopy/Object.h

368

Great. My latest patch has added that approximation.

[alexander-shaposhnikov]

I've added a couple of inline comments, but I'm really glad to see this is moving forward and to me this looks very close to smth committable, many thanks for working on this and many thanks for your patience! Please, also address the remaining comments by Jake and James, I really hope we can wrap it up in the nearest future (~days) assuming that the decompression will be implemented in a follow-up patch.

tools/llvm-objcopy/Object.cpp

170

add a blank line between these two methods, please (for better readability)

tools/llvm-objcopy/Object.h

376

since it's quite large - move it to .cpp + initialize as many fields as possible before the ctor body

[jhenderson]

test/tools/llvm-objcopy/Inputs/compress-debug-sections.yaml

11

Why are these sections SHF_MERGE, SHF_STRINGS?

12

Why is this content more than just some tiny number of bytes, e.g. 4 (i.e. 00010203 or similar)?

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

2

This and the other test should show that the non-compressed version of the section (i.e. .debug_foo) has been removed.

28

Please use CHECK-NEXT as in my above explanation. What could happen here is for this test to fail spuriously if there is a later section with SHF_COMPRESSED in. Using CHECK-NEXT prevents this from happening.

It might be useful to have a separate test to show what happens to existing section flags when compressing, but I don't think that should be in the basic behaviour tests, because it confuses people as to what is important.

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.

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

2

Same comments apply to this test as to the gnu test.

tools/llvm-objcopy/ObjcopyOpts.td

22

I'd rephrase this to: "compress DWARF debug sections using specified style. Supported styles: 'zlib-gnu' and 'zlib'"

tools/llvm-objcopy/Object.cpp

171

Can't you just set this->OriginalData to Sec.OriginalData?

That would allow you to get rid of the Data member entirely.

173

Don't have a blank line here.

188

This is extremely fragile: if we end up adding to the SectionBase list of members, this list will likely not be updated. Also, some of these fields may well be unset at the point of creation of this section (I'm thinking NameIndex specifically, but there may be others - I haven't bothered to check).

I think you might be able to get away with calling the base class's assignment method to set these fields, rather than manually copying each one.

196

Nit: I don't think you need the explicit namespace here.

tools/llvm-objcopy/Object.h

17

You can get rid of this from the header by forward-declaring the DebugCompressionType enum.

20–22

I don't think you need any of these headers.

373

You don't need this function - the base class provides a default of "do nothing" for it.

376

I'm not sure we can make any assumptions for all tools written out there, so I'm reluctant to support having the flag set for GNU-style compressed sections. In particular, I could imagine a case where a tool checks for the flag first, and if it sees it, decompresses it in ZLIB style, or tries to and fails, because it is actually in GNU format. Only if it doesn't see the flag might it then try to decompress as GNU.

tools/llvm-objcopy/llvm-objcopy.cpp

414

Is there a reason you changed the name of this function? I feel like it was fine to be called compressSections. appendCompressableSections suggests that it just adds some sections to the ELF, but in fact, it's also updating the removal predicate to remove the to-be-compressed sections.

[plotfi]

tools/llvm-objcopy/Object.cpp

171

No I dont think so. Unless removal of the section doesn't free up that data.

tools/llvm-objcopy/Object.h

376

I can't initialize these as far as I can see. The base class has no default constructor.

[plotfi]

tools/llvm-objcopy/Object.cpp

188

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).

20–22

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

414

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

171

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

188

@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

188

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

416

SectionsToRemove -> SectionsToCompress

417

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

898

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

188

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

417

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

417

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

161

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

432

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

426

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.

427

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

432

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

161

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

??

tools/llvm-objcopy/llvm-objcopy.cpp

426

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

426

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

426

Yes, I know. Just keeping things simple.

[plotfi]

Adding alignment 8 for compressed sections.

[plotfi]

tools/llvm-objcopy/Object.cpp

161

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

171

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)

test/tools/llvm-objcopy/Inputs/compress-debug-sections.yaml

+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .debug_foo
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_MERGE, SHF_STRINGS ]
+    Content:         416c6c2076697369626c65206f626a656374732c206d616e2c2061726520627574206173207061737465626f617264206d61736b732e
+  - Name:            .notdebug_foo
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_MERGE, SHF_STRINGS ]
+    Content:         416c6c2076697369626c65206f626a656374732c206d616e2c2061726520627574206173207061737465626f617264206d61736b732e
+...

test/tools/llvm-objcopy/compress-debug-sections-invalid-format.test

+# RUN: yaml2obj %p/Inputs/compress-debug-sections.yaml -o %t.o
+# RUN: not llvm-objcopy --compress-debug-sections=zlib-fake %t.o 2>&1 | FileCheck %s
+
+# CHECK: Invalid or unsupported --compress-debug-sections format: zlib-fake.
+

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

+# REQUIRES: zlib
+
+# RUN: yaml2obj %p/Inputs/compress-debug-sections.yaml -o %t.o
+# RUN: llvm-objcopy --compress-debug-sections=zlib-gnu %t.o %t-compressed.o
+# RUN: wc -c %t.o | FileCheck %s -check-prefix=SIZE-ORIGINAL
+# RUN: wc -c %t-compressed.o | FileCheck %s -check-prefix=SIZE-COMPRESSED
+
+# RUN: llvm-objdump -s %t.o -section=.debug_foo | FileCheck %s
+# RUN: llvm-objdump -s %t-compressed.o | FileCheck %s --check-prefix=CHECK-COMPRESSED
+# RUN: llvm-readobj -s %t-compressed.o | FileCheck %s --check-prefix=CHECK-FLAGS
+
+# CHECK: .debug_foo:
+# CHECK: All visible obj
+
+# CHECK-COMPRESSED: .notdebug_foo:
+# CHECK-COMPRESSED: All visible obj
+# CHECK-COMPRESSED: .zdebug_foo:
+# CHECK-COMPRESSED: ZLIB
+
+# CHECK-FLAGS: Name: .notdebug_foo
+# CHECK-FLAGS: Flags [ (0x30)
+
+# CHECK-FLAGS: Name: .zdebug_foo
+# CHECK-FLAGS-NOT: Flags [ (0x8
+# CHECK-FLAGS-NOT: SHF_COMPRESSED (0x800)
+
+# NOTE: Zlib compression makes things worse in this case.
+# SIZE-ORIGINAL: 637
+# SIZE-COMPRESSED: 640
+

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

+# REQUIRES: zlib
+
+# RUN: yaml2obj %p/Inputs/compress-debug-sections.yaml -o %t.o
+# RUN: llvm-objcopy --compress-debug-sections=zlib %t.o %t-compressed.o
+# RUN: wc -c %t.o | FileCheck %s -check-prefix=SIZE-ORIGINAL
+# RUN: wc -c %t-compressed.o | FileCheck %s -check-prefix=SIZE-COMPRESSED
+
+# RUN: llvm-objdump -s %t.o -section=.debug_foo | FileCheck %s
+# RUN: llvm-objdump -s %t-compressed.o | FileCheck %s --check-prefix=CHECK-COMPRESSED
+# RUN: llvm-readobj -s %t-compressed.o | FileCheck %s --check-prefix=CHECK-FLAGS
+
+# CHECK: .debug_foo:
+# CHECK: All visible obj
+
+# CHECK-COMPRESSED: .notdebug_foo:
+# CHECK-COMPRESSED: All visible obj
+# CHECK-COMPRESSED: .debug_foo:
+
+# CHECK-FLAGS: Name: .notdebug_foo
+# CHECK-FLAGS: Flags [ (0x30)
+
+# CHECK-FLAGS: Name: .debug_foo
+# CHECK-FLAGS: Flags [ (0x8
+# CHECK-FLAGS-NEXT: SHF_COMPRESSED (0x800)
+# CHECK-FLAGS-NEXT: SHF_MERGE (0x10)
+# CHECK-FLAGS-NEXT: SHF_STRINGS (0x20)
+# CHECK-FLAGS-NEXT: ]
+
+# NOTE: Zlib compression makes things worse in this case.
+# SIZE-ORIGINAL: 637
+# SIZE-COMPRESSED: 640
+

tools/llvm-objcopy/ObjcopyOpts.td

def B : JoinedOrSeparate<["-"], "B">,
         Alias<binary_architecture>;
 defm input_target : Eq<"input-target">,
                     HelpText<"Format of the input file">,
                     Values<"binary">;
 def I : JoinedOrSeparate<[ "-" ], "I">, Alias<input_target>;
 defm output_target : Eq<"output-target">,
                      HelpText<"Format of the output file">,
                      Values<"binary">;
+defm compress_debug_sections : Eq<"compress-debug-sections">,
+                     MetaVarName<"[ none | zlib | zlib-gnu ]">,
+                     HelpText<"Enable zlib-gnu or zlib Compression of DWARF debug sections.">;
 def O : JoinedOrSeparate<["-"], "O">,
         Alias<output_target>;
 defm split_dwo : Eq<"split-dwo">,
                  MetaVarName<"dwo-file">,
                  HelpText<"Equivalent to extract-dwo on the input file to <dwo-file>, then strip-dwo on the input file">;
 
 def preserve_dates : Flag<[ "-", "--" ], "preserve-dates">,
                      HelpText<"Preserve access and modification timestamps">;

tools/llvm-objcopy/Object.h

 //===- Object.h -------------------------------------------------*- C++ -*-===//
 //
 //                      The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_OBJCOPY_OBJECT_H
 #define LLVM_TOOLS_OBJCOPY_OBJECT_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/StringTableBuilder.h"
 #include "llvm/Object/ELFObjectFile.h"
+#include "llvm/Support/Compression.h"
+#include "llvm/Support/EndianStream.h"
+#include "llvm/Support/Errc.h"
 #include "llvm/Support/FileOutputBuffer.h"
 #include "llvm/Support/JamCRC.h"
 #include <cstddef>
 #include <cstdint>
 #include <functional>
 #include <memory>
 #include <set>
 #include <vector>
 
 namespace llvm {
 namespace objcopy {
 
 class Buffer;
 class SectionBase;
 class Section;
 class OwnedDataSection;
 class StringTableSection;
 class SymbolTableSection;
 class RelocationSection;
 class DynamicRelocationSection;
 class GnuDebugLinkSection;
 class GroupSection;
 class SectionIndexSection;
+class CompressedSection;
 class Segment;
 class Object;
 struct Symbol;
 
 class SectionTableRef {
   MutableArrayRef<std::unique_ptr<SectionBase>> Sections;
 
 public:
   using iterator = pointee_iterator<std::unique_ptr<SectionBase> *>;
 
   explicit SectionTableRef(MutableArrayRef<std::unique_ptr<SectionBase>> Secs)
       : Sections(Secs) {}
   SectionTableRef(const SectionTableRef &) = default;
 
   iterator begin() { return iterator(Sections.data()); }
   iterator end() { return iterator(Sections.data() + Sections.size()); }
 
public:
   virtual void visit(const OwnedDataSection &Sec) = 0;
   virtual void visit(const StringTableSection &Sec) = 0;
   virtual void visit(const SymbolTableSection &Sec) = 0;
   virtual void visit(const RelocationSection &Sec) = 0;
   virtual void visit(const DynamicRelocationSection &Sec) = 0;
   virtual void visit(const GnuDebugLinkSection &Sec) = 0;
   virtual void visit(const GroupSection &Sec) = 0;
   virtual void visit(const SectionIndexSection &Sec) = 0;
+  virtual void visit(const CompressedSection &Sec) = 0;
 };
 
 class SectionWriter : public SectionVisitor {
 protected:
   Buffer &Out;
 
 public:
   virtual ~SectionWriter(){};
 
   void visit(const Section &Sec) override;
   void visit(const OwnedDataSection &Sec) override;
   void visit(const StringTableSection &Sec) override;
   void visit(const DynamicRelocationSection &Sec) override;
   virtual void visit(const SymbolTableSection &Sec) override = 0;
   virtual void visit(const RelocationSection &Sec) override = 0;
   virtual void visit(const GnuDebugLinkSection &Sec) override = 0;
   virtual void visit(const GroupSection &Sec) override = 0;
   virtual void visit(const SectionIndexSection &Sec) override = 0;
+  virtual void visit(const CompressedSection &Sec) override = 0;
 
   explicit SectionWriter(Buffer &Buf) : Out(Buf) {}
 };
 
 template <class ELFT> class ELFSectionWriter : public SectionWriter {
 private:
   using Elf_Word = typename ELFT::Word;
   using Elf_Rel = typename ELFT::Rel;
   using Elf_Rela = typename ELFT::Rela;
   using Elf_Sym = typename ELFT::Sym;
 
 public:
   virtual ~ELFSectionWriter() {}
   void visit(const SymbolTableSection &Sec) override;
   void visit(const RelocationSection &Sec) override;
   void visit(const GnuDebugLinkSection &Sec) override;
   void visit(const GroupSection &Sec) override;
   void visit(const SectionIndexSection &Sec) override;
+  void visit(const CompressedSection &Sec) override;
 
   explicit ELFSectionWriter(Buffer &Buf) : SectionWriter(Buf) {}
 };
 
 #define MAKE_SEC_WRITER_FRIEND                                                 \
   friend class SectionWriter;                                                  \
   template <class ELFT> friend class ELFSectionWriter;
 
 class BinarySectionWriter : public SectionWriter {
 public:
   virtual ~BinarySectionWriter() {}
 
   void visit(const SymbolTableSection &Sec) override;
   void visit(const RelocationSection &Sec) override;
   void visit(const GnuDebugLinkSection &Sec) override;
   void visit(const GroupSection &Sec) override;
   void visit(const SectionIndexSection &Sec) override;
+  void visit(const CompressedSection &Sec) override;
 
   explicit BinarySectionWriter(Buffer &Buf) : SectionWriter(Buf) {}
 };
 
 // The class Buffer abstracts out the common interface of FileOutputBuffer and
 // WritableMemoryBuffer so that the hierarchy of Writers depends on this
 // abstract interface and doesn't depend on a particular implementation.
 // TODO: refactor the buffer classes in LLVM to enable us to use them here
public:
   virtual void finalize() = 0;
   virtual void write() = 0;
 
   Writer(Object &O, Buffer &B) : Obj(O), Buf(B) {}
 };
 
 template <class ELFT> class ELFWriter : public Writer {
 private:
   using Elf_Addr = typename ELFT::Addr;
   using Elf_Shdr = typename ELFT::Shdr;
   using Elf_Phdr = typename ELFT::Phdr;
   using Elf_Ehdr = typename ELFT::Ehdr;
 
   void initEhdrSegment();
 
   void writeEhdr();
   void writePhdr(const Segment &Seg);
   void writeShdr(const SectionBase &Sec);
 
   void writePhdrs();
   void writeShdrs();
   void writeSectionData();
 
   void assignOffsets();
 
   std::unique_ptr<ELFSectionWriter<ELFT>> SecWriter;
 
   size_t totalSize() const;
 
 public:
   virtual ~ELFWriter() {}
   bool WriteSectionHeaders = true;
 
   void finalize() override;
   void write() override;
   ELFWriter(Object &Obj, Buffer &Buf, bool WSH)
       : Writer(Obj, Buf), WriteSectionHeaders(WSH) {}
 };
 
 class BinaryWriter : public Writer {
 private:
   std::unique_ptr<BinarySectionWriter> SecWriter;
 
   uint64_t TotalSize;
 
 public:
   ~BinaryWriter() {}
   void finalize() override;
   void write() override;
   BinaryWriter(Object &Obj, Buffer &Buf) : Writer(Obj, Buf) {}
 };
 
 class SectionBase {
 public:
-  StringRef Name;
+  std::string Name;
   Segment *ParentSegment = nullptr;
   uint64_t HeaderOffset;
   uint64_t OriginalOffset = std::numeric_limits<uint64_t>::max();
   uint32_t Index;
   bool HasSymbol = false;
 
   uint64_t Addr = 0;
   uint64_t Align = 1;
   uint32_t EntrySize = 0;
   uint64_t Flags = 0;
   uint64_t Info = 0;
   uint64_t Link = ELF::SHN_UNDEF;
   uint64_t NameIndex = 0;
   uint64_t Offset = 0;
   uint64_t Size = 0;
   uint64_t Type = ELF::SHT_NULL;
   ArrayRef<uint8_t> OriginalData;
 
   virtual ~SectionBase() = default;
 
   virtual void initialize(SectionTableRef SecTable);
   virtual void finalize();
   virtual void removeSectionReferences(const SectionBase *Sec);
   virtual void removeSymbols(function_ref<bool(const Symbol &)> ToRemove);
   virtual void accept(SectionVisitor &Visitor) const = 0;
   virtual void markSymbols();
 };
 
 class Segment {
 private:
   struct SectionCompare {
     bool operator()(const SectionBase *Lhs, const SectionBase *Rhs) const {
       // Some sections might have the same address if one of them is empty. To
       // fix this we can use the lexicographic ordering on ->Addr and the
       // address of the actully stored section.
       if (Lhs->OriginalOffset == Rhs->OriginalOffset)
         return Lhs < Rhs;
       return Lhs->OriginalOffset < Rhs->OriginalOffset;
     }
public:
 
   void removeSection(const SectionBase *Sec) { Sections.erase(Sec); }
   void addSection(const SectionBase *Sec) { Sections.insert(Sec); }
 };
 
 class Section : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
 
   ArrayRef<uint8_t> Contents;
   SectionBase *LinkSection = nullptr;
 
 public:
   explicit Section(ArrayRef<uint8_t> Data) : Contents(Data) {}
 
   void accept(SectionVisitor &Visitor) const override;
   void removeSectionReferences(const SectionBase *Sec) override;
   void initialize(SectionTableRef SecTable) override;
   void finalize() override;
 };
 
 class OwnedDataSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
 
   std::vector<uint8_t> Data;
 
 public:
   OwnedDataSection(StringRef SecName, ArrayRef<uint8_t> Data)
       : Data(std::begin(Data), std::end(Data)) {
-    Name = SecName;
+    Name = SecName.str();
     Type = ELF::SHT_PROGBITS;
     Size = Data.size();
     OriginalOffset = std::numeric_limits<uint64_t>::max();
   }
 
   void accept(SectionVisitor &Sec) const override;
 };
 
+class CompressedSection : public SectionBase {
+  MAKE_SEC_WRITER_FRIEND
+
+  std::vector<uint8_t> Data;
+  DebugCompressionType CompressionType;
+
+public:
+  CompressedSection(const SectionBase &Sec,
+                    DebugCompressionType CompressionType)
+      : Data(std::begin(Sec.OriginalData), std::end(Sec.OriginalData)),
+        CompressionType(CompressionType) {
+    const bool isGnuStyle = (CompressionType == DebugCompressionType::GNU);
+    Name = (isGnuStyle ? ".z" : ".") + Sec.Name.substr(1);
+    OriginalData = ArrayRef<uint8_t>(Data.data(), Data.size());
+    Align = Sec.Align;
+    Flags |= Sec.Flags;
+    Type = ELF::SHT_PROGBITS;
+    Size = Data.size();
+    Flags |= ELF::SHF_COMPRESSED;
+  }
+
+  void finalize() override;
+  void accept(SectionVisitor &Visitor) const override;
+};
+
 // There are two types of string tables that can exist, dynamic and not dynamic.
 // In the dynamic case the string table is allocated. Changing a dynamic string
 // table would mean altering virtual addresses and thus the memory image. So
 // dynamic string tables should not have an interface to modify them or
 // reconstruct them. This type lets us reconstruct a string table. To avoid
 // this class being used for dynamic string tables (which has happened) the
 // classof method checks that the particular instance is not allocated. This
 // then agrees with the makeSection method used to construct most sections.
 class StringTableSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
 
   StringTableBuilder StrTabBuilder;
 
 public:
   StringTableSection() : StrTabBuilder(StringTableBuilder::ELF) {
     Type = ELF::SHT_STRTAB;
   }
 
   void addString(StringRef Name);
   uint32_t findIndex(StringRef Name) const;

 enum SymbolShndxType {
   SYMBOL_SIMPLE_INDEX = 0,
   SYMBOL_ABS = ELF::SHN_ABS,
   SYMBOL_COMMON = ELF::SHN_COMMON,
   SYMBOL_HEXAGON_SCOMMON = ELF::SHN_HEXAGON_SCOMMON,
   SYMBOL_HEXAGON_SCOMMON_2 = ELF::SHN_HEXAGON_SCOMMON_2,
   SYMBOL_HEXAGON_SCOMMON_4 = ELF::SHN_HEXAGON_SCOMMON_4,
   SYMBOL_HEXAGON_SCOMMON_8 = ELF::SHN_HEXAGON_SCOMMON_8,
   SYMBOL_XINDEX = ELF::SHN_XINDEX,
 };
 
 struct Symbol {
   uint8_t Binding;
   SectionBase *DefinedIn = nullptr;
   SymbolShndxType ShndxType;
   uint32_t Index;
   std::string Name;

tools/llvm-objcopy/Object.cpp

 void SectionBase::removeSymbols(function_ref<bool(const Symbol &)> ToRemove) {}
 void SectionBase::initialize(SectionTableRef SecTable) {}
 void SectionBase::finalize() {}
 void SectionBase::markSymbols() {}
 
 template <class ELFT> void ELFWriter<ELFT>::writeShdr(const SectionBase &Sec) {
   uint8_t *B = Buf.getBufferStart();
   B += Sec.HeaderOffset;
   Elf_Shdr &Shdr = *reinterpret_cast<Elf_Shdr *>(B);
   Shdr.sh_name = Sec.NameIndex;
   Shdr.sh_type = Sec.Type;
   Shdr.sh_flags = Sec.Flags;
   Shdr.sh_addr = Sec.Addr;
   Shdr.sh_offset = Sec.Offset;
   Shdr.sh_size = Sec.Size;
   Shdr.sh_link = Sec.Link;
   Shdr.sh_info = Sec.Info;

 void BinarySectionWriter::visit(const GroupSection &Sec) {
   error("Cannot write '" + Sec.Name + "' out to binary");
 }
 
 void SectionWriter::visit(const Section &Sec) {
   if (Sec.Type == SHT_NOBITS)
     return;
   uint8_t *Buf = Out.getBufferStart() + Sec.Offset;
   std::copy(std::begin(Sec.Contents), std::end(Sec.Contents), Buf);
 }
 
 void Section::accept(SectionVisitor &Visitor) const { Visitor.visit(*this); }
 
 void SectionWriter::visit(const OwnedDataSection &Sec) {
   uint8_t *Buf = Out.getBufferStart() + Sec.Offset;
   std::copy(std::begin(Sec.Data), std::end(Sec.Data), Buf);
 }
 
 void OwnedDataSection::accept(SectionVisitor &Visitor) const {
   Visitor.visit(*this);
 }
 
+void BinarySectionWriter::visit(const CompressedSection &Sec) {
+  error("Cannot write compressed section '" + Sec.Name + "' ");
+}
+
+template <class ELFT>
+void ELFSectionWriter<ELFT>::visit(const CompressedSection &Sec) {
+  uint8_t *Buf = Out.getBufferStart();
+  Buf += Sec.Offset;
+
+  const uint64_t DecompressedSize = Sec.OriginalData.size();
+  if (Sec.CompressionType == DebugCompressionType::GNU) {
+    ArrayRef<uint8_t> Magic = {'Z', 'L', 'I', 'B'};
+    std::copy(Magic.begin(), Magic.end(), Buf);
+    Buf += Magic.size();
+    uint64_t *DecompressedSizePtr = reinterpret_cast<uint64_t *>(Buf);
+    *DecompressedSizePtr = support::endian::read64be(&DecompressedSize);
+    Buf += sizeof(DecompressedSize);
+  } else {
+    auto Chdr = reinterpret_cast<Elf_Chdr_Impl<ELFT> *>(Buf);
+    Chdr->ch_type = ELF::ELFCOMPRESS_ZLIB;
+    Chdr->ch_size = DecompressedSize;
+    Chdr->ch_addralign = Sec.Align;
+    Buf += sizeof(*Chdr);
+  }
+
+  StringRef Contents(reinterpret_cast<const char *>(Sec.OriginalData.data()),
+                     Sec.OriginalData.size());
+  SmallVector<char, 128> CompressedBuffer;
+  if (Error E = zlib::compress(Contents, CompressedBuffer))
+    reportError(Sec.Name, std::move(E));
+  std::copy(CompressedBuffer.begin(), CompressedBuffer.end(), Buf);
+}
+
+void CompressedSection::finalize() {}
+void CompressedSection::accept(SectionVisitor &Visitor) const {
+  Visitor.visit(*this);
+}
+
 void StringTableSection::addString(StringRef Name) {
   StrTabBuilder.add(Name);
   Size = StrTabBuilder.getSize();
 }
 
 uint32_t StringTableSection::findIndex(StringRef Name) const {
   return StrTabBuilder.getOffset(Name);
 }
 
 void StringTableSection::finalize() { StrTabBuilder.finalize(); }
 
 void SectionWriter::visit(const StringTableSection &Sec) {
   Sec.StrTabBuilder.write(Out.getBufferStart() + Sec.Offset);
 }
 
 void StringTableSection::accept(SectionVisitor &Visitor) const {
   Visitor.visit(*this);
 }
 
 template <class ELFT>
 void ELFSectionWriter<ELFT>::visit(const SectionIndexSection &Sec) {
   uint8_t *Buf = Out.getBufferStart() + Sec.Offset;
   auto *IndexesBuffer = reinterpret_cast<Elf_Word *>(Buf);
   std::copy(std::begin(Sec.Indexes), std::end(Sec.Indexes), IndexesBuffer);
 }
 
void DynamicRelocationSection::accept(SectionVisitor &Visitor) const {
   Visitor.visit(*this);
 }
 
 void Section::removeSectionReferences(const SectionBase *Sec) {
   if (LinkSection == Sec) {
     error("Section " + LinkSection->Name +
           " cannot be removed because it is "
           "referenced by the section " +
           this->Name);
   }
 }
 
 void GroupSection::finalize() {
   this->Info = Sym->Index;
   this->Link = SymTab->Index;
 }
 
 void GroupSection::removeSymbols(function_ref<bool(const Symbol &)> ToRemove) {
LinkSection =
         SecTable.getSection(Link, "Link field value " + Twine(Link) +
                                       " in section " + Name + " is invalid");
     if (LinkSection->Type == ELF::SHT_SYMTAB)
       LinkSection = nullptr;
   }
 }
 
 void Section::finalize() { this->Link = LinkSection ? LinkSection->Index : 0; }
 
 void GnuDebugLinkSection::init(StringRef File, StringRef Data) {
   FileName = sys::path::filename(File);
   // The format for the .gnu_debuglink starts with the file name and is
   // followed by a null terminator and then the CRC32 of the file. The CRC32
   // should be 4 byte aligned. So we add the FileName size, a 1 for the null
   // byte, and then finally push the size to alignment and add 4.
   Size = alignTo(FileName.size() + 1, 4) + 4;
   // The CRC32 will only be aligned if we align the whole section.

 // supplied starting Offset is used for the starting offset of any section that
 // does not have a ParentSegment. It returns either the offset given if all
 // sections had a ParentSegment or an offset one past the last section if there
 // was a section that didn't have a ParentSegment.
 template <class Range>
 static uint64_t LayoutSections(Range Sections, uint64_t Offset) {
   // Now the offset of every segment has been set we can assign the offsets
   // of each section. For sections that are covered by a segment we should use
   // the segment's original offset and the section's original offset to compute
   // the offset from the start of the segment. Using the offset from the start
   // of the segment we can assign a new offset to the section. For sections not
   // covered by segments we can just bump Offset to the next valid location.
   uint32_t Index = 1;
   for (auto &Section : Sections) {
     Section.Index = Index++;
     if (Section.ParentSegment != nullptr) {
       auto Segment = *Section.ParentSegment;
       Section.Offset =
           Segment.Offset + (Section.OriginalOffset - Segment.OriginalOffset);
     } else {
       Offset = alignTo(Offset, Section.Align == 0 ? 1 : Section.Align);
       Section.Offset = Offset;
       if (Section.Type != SHT_NOBITS)
         Offset += Section.Size;
     }
template <class ELFT> void ELFWriter<ELFT>::write() {
   writeEhdr();
   writePhdrs();
   writeSectionData();
   if (WriteSectionHeaders)
     writeShdrs();
   if (auto E = Buf.commit())
     reportError(Buf.getName(), errorToErrorCode(std::move(E)));
 }
 
 template <class ELFT> void ELFWriter<ELFT>::finalize() {
   // It could happen that SectionNames has been removed and yet the user wants
   // a section header table output. We need to throw an error if a user tries
   // to do that.
   if (Obj.SectionNames == nullptr && WriteSectionHeaders)
     error("Cannot write section header table because section header string "
           "table was removed.");
 
if (NeedsLargeIndexes) {
     // references to it.
     if (Obj.SectionIndexTable != nullptr) {
       Obj.removeSections([this](const SectionBase &Sec) {
         return &Sec == Obj.SectionIndexTable;
       });
     }
   }
 
   // Make sure we add the names of all the sections. Importantly this must be
   // done after we decide to add or remove SectionIndexes.
   if (Obj.SectionNames != nullptr)
     for (const auto &Section : Obj.sections()) {
       Obj.SectionNames->addString(Section.Name);
     }
 
   initEhdrSegment();
   // Before we can prepare for layout the indexes need to be finalized.
   uint64_t Index = 0;
   for (auto &Sec : Obj.sections())
     Sec.Index = Index++;
 

tools/llvm-objcopy/llvm-objcopy.cpp

struct CopyConfig {
   bool StripAll = false;
   bool StripAllGNU = false;
   bool StripDWO = false;
   bool StripDebug = false;
   bool StripNonAlloc = false;
   bool StripSections = false;
   bool StripUnneeded = false;
   bool Weaken = false;
+  DebugCompressionType CompressionType = DebugCompressionType::None;
 };
 
 using SectionPred = std::function<bool(const SectionBase &Sec)>;
 
 enum SectionFlag {
   SecNone = 0,
   SecAlloc = 1 << 0,
   SecLoad = 1 << 1,
if (Flags & SectionFlag::SecMerge)
       *SR.NewFlags |= ELF::SHF_MERGE;
     if (Flags & SectionFlag::SecStrings)
       *SR.NewFlags |= ELF::SHF_STRINGS;
   }
 
   return SR;
 }
 
 static bool isDebugSection(const SectionBase &Sec) {
-  return Sec.Name.startswith(".debug") || Sec.Name.startswith(".zdebug") ||
-         Sec.Name == ".gdb_index";
+  return StringRef(Sec.Name).startswith(".debug") ||
+         StringRef(Sec.Name).startswith(".zdebug") || Sec.Name == ".gdb_index";
 }
 
 static bool isDWOSection(const SectionBase &Sec) {
-  return Sec.Name.endswith(".dwo");
+  return StringRef(Sec.Name).endswith(".dwo");
 }
 
 static bool onlyKeepDWOPred(const Object &Obj, const SectionBase &Sec) {
   // We can't remove the section header string table.
   if (&Sec == Obj.SectionNames)
     return false;
   // Short of keeping the string table we want to keep everything that is a DWO
   // section and remove everything else.
   return !isDWOSection(Sec);
 }
 
 static const StringMap<MachineInfo> ArchMap{
     // Name, {EMachine, 64bit, LittleEndian}
     {"aarch64", {EM_AARCH64, true, true}},
     {"arm", {EM_ARM, false, true}},
     {"i386", {EM_386, false, true}},
     {"i386:x86-64", {EM_X86_64, true, true}},
     {"powerpc:common64", {EM_PPC64, true, true}},
     {"sparc", {EM_SPARC, false, true}},
     {"x86-64", {EM_X86_64, true, true}},

 static ElfType getOutputElfType(const MachineInfo &MI) {
   // Infer output ELF type from the binary arch specified
   if (MI.Is64Bit)
     return MI.IsLittleEndian ? ELFT_ELF64LE : ELFT_ELF64BE;
   else
     return MI.IsLittleEndian ? ELFT_ELF32LE : ELFT_ELF32BE;
 }
 
 static std::unique_ptr<Writer> createWriter(const CopyConfig &Config,
                                             Object &Obj, Buffer &Buf,
                                             ElfType OutputElfType) {
   if (Config.OutputFormat == "binary") {
     return llvm::make_unique<BinaryWriter>(Obj, Buf);
   }
   // Depending on the initial ELFT and OutputFormat we need a different Writer.
   switch (OutputElfType) {
   case ELFT_ELF32LE:
DWOFile->removeSections(
       [&](const SectionBase &Sec) { return onlyKeepDWOPred(*DWOFile, Sec); });
   FileBuffer FB(File);
   auto Writer = createWriter(Config, *DWOFile, FB, OutputElfType);
   Writer->finalize();
   Writer->write();
 }
 
 static Error dumpSectionToFile(StringRef SecName, StringRef Filename,
                                Object &Obj) {
   for (auto &Sec : Obj.sections()) {
     if (Sec.Name == SecName) {
       if (Sec.OriginalData.size() == 0)
         return make_error<StringError>("Can't dump section \"" + SecName +
                                            "\": it has no contents",
                                        object_error::parse_failed);
       Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
           FileOutputBuffer::create(Filename, Sec.OriginalData.size());
       if (!BufferOrErr)
         return BufferOrErr.takeError();
       std::unique_ptr<FileOutputBuffer> Buf = std::move(*BufferOrErr);
       std::copy(Sec.OriginalData.begin(), Sec.OriginalData.end(),
                 Buf->getBufferStart());
       if (Error E = Buf->commit())
         return E;
       return Error::success();
     }
   }
   return make_error<StringError>("Section not found",
                                  object_error::parse_failed);
 }
 
+static bool isCompressed(const SectionBase &Section) {
+  return StringRef(Section.Name).startswith(".zdebug") ||
+         (Section.Flags & ELF::SHF_COMPRESSED);
+}
+
+static void appendCompressableSections(const CopyConfig &Config, Object &Obj,
+                                       SectionPred &RemovePred) {
+  std::vector<SectionBase *> SectionsToRemove;
+  std::for_each(Obj.sections().begin(), Obj.sections().end(),
+                [&SectionsToRemove](SectionBase &Section) {
+                  if (!isCompressed(Section) && isDebugSection(Section) &&
+                      Section.Name != ".gdb_index") {
+                    SectionsToRemove.push_back(&Section);
+                  }
+                });
+
+  for (auto S : SectionsToRemove) {
+    const SectionBase &Section = *S;
+    Obj.addSection<CompressedSection>(Section, Config.CompressionType);
+    RemovePred = [RemovePred, &Section](const SectionBase &Sec) {
+      return &Sec == &Section || RemovePred(Sec);
+    };
+  }
+}
 // This function handles the high level operations of GNU objcopy including
 // handling command line options. It's important to outline certain properties
 // we expect to hold of the command line operations. Any operation that "keeps"
 // should keep regardless of a remove. Additionally any removal should respect
 // any previous removals. Lastly whether or not something is removed shouldn't
 // depend a) on the order the options occur in or b) on some opaque priority
 // system. The only priority is that keeps/copies overrule removes.
 static void handleArgs(const CopyConfig &Config, Object &Obj,
                        const Reader &Reader, ElfType OutputElfType) {
 
   if (!Config.SplitDWO.empty()) {
     splitDWOToFile(Config, Reader, Config.SplitDWO, OutputElfType);
   }
 
   // TODO: update or remove symbols only if there is an option that affects
   // them.
   if (Obj.SymbolTable) {
     Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
       if ((Config.LocalizeHidden &&
            (Sym.Visibility == STV_HIDDEN || Sym.Visibility == STV_INTERNAL)) ||
           (!Config.SymbolsToLocalize.empty() &&
            is_contained(Config.SymbolsToLocalize, Sym.Name)))
         Sym.Binding = STB_LOCAL;
 
       // Note: these two globalize flags have very similar names but different
       // meanings:
       //
       // --globalize-symbol: promote a symbol to global
       // --keep-global-symbol: all symbols except for these should be made local
       //
       // If --globalize-symbol is specified for a given symbol, it will be
       // global in the output file even if it is not included via
       // --keep-global-symbol. Because of that, make sure to check
       // --globalize-symbol second.
       if (!Config.SymbolsToKeepGlobal.empty() &&
           !is_contained(Config.SymbolsToKeepGlobal, Sym.Name))
         Sym.Binding = STB_LOCAL;
 
       if (!Config.SymbolsToGlobalize.empty() &&
           is_contained(Config.SymbolsToGlobalize, Sym.Name))
         Sym.Binding = STB_GLOBAL;
 
       if (!Config.SymbolsToWeaken.empty() &&
           is_contained(Config.SymbolsToWeaken, Sym.Name) &&
           Sym.Binding == STB_GLOBAL)
         Sym.Binding = STB_WEAK;
 
       if (Config.Weaken && Sym.Binding == STB_GLOBAL &&
           Sym.getShndx() != SHN_UNDEF)
         Sym.Binding = STB_WEAK;
 
       const auto I = Config.SymbolsToRename.find(Sym.Name);
       if (I != Config.SymbolsToRename.end())
         Sym.Name = I->getValue();
 
       if (!Config.SymbolsPrefix.empty() && Sym.Type != STT_SECTION)
         Sym.Name = (Config.SymbolsPrefix + Sym.Name).str();
     });
 
     // The purpose of this loop is to mark symbols referenced by sections
     // (like GroupSection or RelocationSection). This way, we know which
     // symbols are still 'needed' and wich are not.
     if (Config.StripUnneeded) {
       for (auto &Section : Obj.sections())
         Section.markSymbols();
     }
 
     Obj.removeSymbols([&](const Symbol &Sym) {
       if ((!Config.SymbolsToKeep.empty() &&
            is_contained(Config.SymbolsToKeep, Sym.Name)) ||
           (Config.KeepFileSymbols && Sym.Type == STT_FILE))
         return false;
 
       if (Config.DiscardAll && Sym.Binding == STB_LOCAL &&
           Sym.getShndx() != SHN_UNDEF && Sym.Type != STT_FILE &&
           Sym.Type != STT_SECTION)
         return true;
 
       if (Config.StripAll || Config.StripAllGNU)
         return true;
 
       if (!Config.SymbolsToRemove.empty() &&
           is_contained(Config.SymbolsToRemove, Sym.Name)) {
         return true;
       }
 
       if (Config.StripUnneeded && !Sym.Referenced &&
           (Sym.Binding == STB_LOCAL || Sym.getShndx() == SHN_UNDEF) &&
           Sym.Type != STT_FILE && Sym.Type != STT_SECTION)
         return true;
 
       return false;
     });
if (Config.StripNonAlloc)
     };
 
   if (Config.StripAll)
     RemovePred = [RemovePred, &Obj](const SectionBase &Sec) {
       if (RemovePred(Sec))
         return true;
       if (&Sec == Obj.SectionNames)
         return false;
-      if (Sec.Name.startswith(".gnu.warning"))
+      if (StringRef(Sec.Name).startswith(".gnu.warning"))
         return false;
       return (Sec.Flags & SHF_ALLOC) == 0;
     };
 
   // Explicit copies:
   if (!Config.OnlyKeep.empty()) {
     RemovePred = [&Config, RemovePred, &Obj](const SectionBase &Sec) {
       // Explicitly keep these sections regardless of previous removes.
       if (is_contained(Config.OnlyKeep, Sec.Name))
         return false;
if ((!Config.SymbolsToKeep.empty() || Config.KeepFileSymbols) &&
       Obj.SymbolTable && !Obj.SymbolTable->empty()) {
     RemovePred = [&Obj, RemovePred](const SectionBase &Sec) {
       if (&Sec == Obj.SymbolTable || &Sec == Obj.SymbolTable->getStrTab())
         return false;
       return RemovePred(Sec);
     };
   }
 
+  if (Config.CompressionType != DebugCompressionType::None)
+    appendCompressableSections(Config, Obj, RemovePred);
+
   Obj.removeSections(RemovePred);
 
   if (!Config.SectionsToRename.empty()) {
     for (auto &Sec : Obj.sections()) {
       const auto Iter = Config.SectionsToRename.find(Sec.Name);
       if (Iter != Config.SectionsToRename.end()) {
         const SectionRename &SR = Iter->second;
         Sec.Name = SR.NewName;
         if (SR.NewFlags.hasValue()) {
           // Preserve some flags which should not be dropped when setting flags.
           // Also, preserve anything OS/processor dependant.
           const uint64_t PreserveMask = ELF::SHF_COMPRESSED | ELF::SHF_EXCLUDE |
                                         ELF::SHF_GROUP | ELF::SHF_LINK_ORDER |
                                         ELF::SHF_MASKOS | ELF::SHF_MASKPROC |
                                         ELF::SHF_TLS | ELF::SHF_INFO_LINK;
           Sec.Flags = (Sec.Flags & PreserveMask) |
                       (SR.NewFlags.getValue() & ~PreserveMask);
         }
       }
     }
   }
 
   if (!Config.AddSection.empty()) {
     for (const auto &Flag : Config.AddSection) {
       auto SecPair = Flag.split("=");
       auto SecName = SecPair.first;
       auto File = SecPair.second;
       auto BufOrErr = MemoryBuffer::getFile(File);
       if (!BufOrErr)
         reportError(File, BufOrErr.getError());
       auto Buf = std::move(*BufOrErr);
       auto BufPtr = reinterpret_cast<const uint8_t *>(Buf->getBufferStart());
       auto BufSize = Buf->getBufferSize();
       Obj.addSection<OwnedDataSection>(SecName,
                                        ArrayRef<uint8_t>(BufPtr, BufSize));
     }
   }
 
   if (!Config.DumpSection.empty()) {
     for (const auto &Flag : Config.DumpSection) {
       std::pair<StringRef, StringRef> SecPair = Flag.split("=");
       StringRef SecName = SecPair.first;
       StringRef File = SecPair.second;
       if (Error E = dumpSectionToFile(SecName, File, Obj))
         reportError(Config.InputFilename, std::move(E));
     }
   }
 
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink);
 }
 
 static void executeElfObjcopyOnBinary(const CopyConfig &Config, Reader &Reader,
                                       Buffer &Out, ElfType OutputElfType) {
   std::unique_ptr<Object> Obj = Reader.create();
 
   handleArgs(Config, *Obj, Reader, OutputElfType);
 
   std::unique_ptr<Writer> Writer =
       createWriter(Config, *Obj, Out, OutputElfType);
   Writer->finalize();
   Writer->write();
 }
 
 // For regular archives this function simply calls llvm::writeArchive,
 // For thin archives it writes the archive file itself as well as its members.
 static Error deepWriteArchive(StringRef ArcName,
                               ArrayRef<NewArchiveMember> NewMembers,
                               bool WriteSymtab, object::Archive::Kind Kind,
                               bool Deterministic, bool Thin) {
   Error E =
       writeArchive(ArcName, NewMembers, WriteSymtab, Kind, Deterministic, Thin);
   if (!Thin || E)
     return E;
   for (const NewArchiveMember &Member : NewMembers) {
for (const NewArchiveMember &Member : NewMembers) {
     std::copy(Member.Buf->getBufferStart(), Member.Buf->getBufferEnd(),
               FB.getBufferStart());
     if (auto E = FB.commit())
       return E;
   }
   return Error::success();
 }
 
 static void executeElfObjcopyOnArchive(const CopyConfig &Config,
                                        const Archive &Ar) {
   std::vector<NewArchiveMember> NewArchiveMembers;
   Error Err = Error::success();
   for (const Archive::Child &Child : Ar.children(Err)) {
     Expected<std::unique_ptr<Binary>> ChildOrErr = Child.getAsBinary();
     if (!ChildOrErr)
       reportError(Ar.getFileName(), ChildOrErr.takeError());
     Binary *Bin = ChildOrErr->get();
 
     Expected<StringRef> ChildNameOrErr = Child.getName();
static CopyConfig parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   Config.OutputFormat = InputArgs.getLastArgValue(OBJCOPY_output_target);
   if (Config.InputFormat == "binary") {
     auto BinaryArch = InputArgs.getLastArgValue(OBJCOPY_binary_architecture);
     if (BinaryArch.empty())
       error("Specified binary input without specifiying an architecture");
     Config.BinaryArch = getMachineInfo(BinaryArch);
   }
 
+  Config.CompressionType =
+      StringSwitch<DebugCompressionType>(
+          InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections))
+          .Case("zlib-gnu", DebugCompressionType::GNU)
+          .Case("zlib", DebugCompressionType::Z)
+          .Default(DebugCompressionType::None);
+
+  if (Config.CompressionType == DebugCompressionType::None &&
+      InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections) != "") {
+    error("Invalid or unsupported --compress-debug-sections format: " +
+          InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections));
+  }
+
   Config.SplitDWO = InputArgs.getLastArgValue(OBJCOPY_split_dwo);
   Config.AddGnuDebugLink = InputArgs.getLastArgValue(OBJCOPY_add_gnu_debuglink);
   Config.SymbolsPrefix = InputArgs.getLastArgValue(OBJCOPY_prefix_symbols);
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_redefine_symbol)) {
     if (!StringRef(Arg->getValue()).contains('='))
       error("Bad format for --redefine-sym");
     auto Old2New = StringRef(Arg->getValue()).split('=');
static CopyConfig parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   Config.StripNonAlloc = InputArgs.hasArg(OBJCOPY_strip_non_alloc);
   Config.StripUnneeded = InputArgs.hasArg(OBJCOPY_strip_unneeded);
   Config.ExtractDWO = InputArgs.hasArg(OBJCOPY_extract_dwo);
   Config.LocalizeHidden = InputArgs.hasArg(OBJCOPY_localize_hidden);
   Config.Weaken = InputArgs.hasArg(OBJCOPY_weaken);
   Config.DiscardAll = InputArgs.hasArg(OBJCOPY_discard_all);
   Config.OnlyKeepDebug = InputArgs.hasArg(OBJCOPY_only_keep_debug);
   Config.KeepFileSymbols = InputArgs.hasArg(OBJCOPY_keep_file_symbols);
   for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbol))
     Config.SymbolsToLocalize.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_global_symbol))
     Config.SymbolsToKeepGlobal.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_global_symbols))
     addGlobalSymbolsFromFile(Config.SymbolsToKeepGlobal, Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_globalize_symbol))
     Config.SymbolsToGlobalize.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_weaken_symbol))
     Config.SymbolsToWeaken.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_strip_symbol))
     Config.SymbolsToRemove.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_symbol))
     Config.SymbolsToKeep.push_back(Arg->getValue());
 
   Config.PreserveDates = InputArgs.hasArg(OBJCOPY_preserve_dates);
 
   return Config;
 }
 
 // ParseStripOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseStripOptions will print the help messege and
 // exit.
 static CopyConfig parseStripOptions(ArrayRef<const char *> ArgsArr) {
   StripOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;