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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

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.



================
Comment at: tools/llvm-objcopy/Object.cpp:162
+    Chdr->ch_size = Sec.DecompressedSize;
+    Chdr->ch_addralign = Sec.Align;
+    Buf += sizeof(*Chdr);
----------------
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.


================
Comment at: tools/llvm-objcopy/Object.h:365
+
+  std::vector<uint8_t> Data;
+  SmallVector<char, 128> CompressedData;
----------------
plotfi wrote:
> jakehehrlich wrote:
> > I don't think you need this Data. OriginalData should be what CompressedData is.
> But who owns this original data? It comes from another section that will be removed. Will make the change. 
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.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:432
+
+    RemovePred = [RemovePred, S](const SectionBase &Sec) {
+      return &Sec == S || RemovePred(Sec);
----------------
I think we can do this more efficiently now that we don't remove only if space is actually improved.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678