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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:188
+  Offset = Sec.Offset;
+  Type = ELF::SHT_PROGBITS;
+
----------------
jakehehrlich wrote:
> plotfi wrote:
> > jhenderson wrote:
> > > 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.
> > Base class has no assignment method. It'll try and add one. Also base class has no constructor because it's abstract I believe. 
> @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.
Alex and I figured out how to get C++ to give us a proper default field copy constructor. 


================
Comment at: tools/llvm-objcopy/Object.h:377
+    Flags |= Sec.Flags;
+    if (isGnuStyle)
+      return;
----------------
jakehehrlich wrote:
> 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.
It's changed to set the flag only in the non-gnu case in the latest diff. 


================
Comment at: tools/llvm-objcopy/Object.h:377
+    Flags |= Sec.Flags;
+    if (isGnuStyle)
+      return;
----------------
plotfi wrote:
> jakehehrlich wrote:
> > 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.
> It's changed to set the flag only in the non-gnu case in the latest diff. 
sounds good


================
Comment at: tools/llvm-objcopy/Object.h:365
+
+  std::vector<uint8_t> Data;
+  SmallVector<char, 128> CompressedData;
----------------
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. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:417
+  SmallVector<SectionBase *, 13> SectionsToRemove;
+  std::for_each(Obj.sections().begin(), Obj.sections().end(),
+                [&SectionsToRemove](SectionBase &Section) {
----------------
jhenderson wrote:
> Use std::copy_if and std::back_inserter to do this. It looks a bit cleaner, in my opinion.
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. 


Repository:
  rL LLVM

https://reviews.llvm.org/D49678