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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:171
+                                     DebugCompressionType CompressionType)
+    : Data(std::begin(Sec.OriginalData), std::end(Sec.OriginalData)),
+      CompressionType(CompressionType) {
----------------
plotfi wrote:
> jhenderson wrote:
> > Can't you just set this->OriginalData to Sec.OriginalData?
> > 
> > That would allow you to get rid of the Data member entirely.
> No I dont think so. Unless removal of the section doesn't free up that data. 
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.


================
Comment at: tools/llvm-objcopy/Object.h:377
+    Flags |= Sec.Flags;
+    if (isGnuStyle)
+      return;
----------------
plotfi wrote:
> jhenderson wrote:
> > plotfi wrote:
> > > jakehehrlich wrote:
> > > > 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.
> > > 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. 
> > 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.
> Jake wanted to always set it. I can not set it for the gnu case. However, gnu objcopy worked in gnu more. 
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.


================
Comment at: tools/llvm-objcopy/Object.h:365
+
+  std::vector<uint8_t> Data;
+  SmallVector<char, 128> CompressedData;
----------------
I don't think you need this Data. OriginalData should be what CompressedData is.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678