[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:167
+    W.OS << Magic;
+    support::endian::write(W.OS, DecompressedSize, support::big);
+    return Error::success();
----------------
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.


================
Comment at: tools/llvm-objcopy/Object.cpp:194
+  raw_svector_ostream OS(CompressedContents);
+  support::endian::Writer W(OS, ELFT::TargetEndianness);
+  if (Error E =
----------------
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.


================
Comment at: tools/llvm-objcopy/Object.h:333
 
+  // TODO: Contents is present in several classes of the hierarchy.
+  // This needs to be refactored to avoid duplication.
----------------
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.


================
Comment at: tools/llvm-objcopy/Object.h:339-342
+  explicit Section(ArrayRef<uint8_t> Data) : Contents(Data) {
+    // TODO: Unify usage of OriginalData instead of Contents.
+    OriginalData = Contents;
+  }
----------------
Don't change this constructor. OriginalData should already work if you rebase.


================
Comment at: tools/llvm-objcopy/Object.h:354
+protected:
+  std::string OwnedName;
   std::vector<uint8_t> Data;
----------------
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?


================
Comment at: tools/llvm-objcopy/Object.h:360
       : Data(std::begin(Data), std::end(Data)) {
-    Name = SecName;
+    OriginalData = ArrayRef<uint8_t>(Data.data(), Data.size());
+    OwnedName = SecName.str();
----------------
Again, this is not how original data should be set. You don't need to use OwnedDataSection at all.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:355
+  Expected<Decompressor> D = object::Decompressor::create(
+      Section.Name, Contents, (ELFT::TargetEndianness == endianness::little),
+      ELFT::Is64Bits);
----------------
plotfi wrote:
> jakehehrlich wrote:
> > jhenderson wrote:
> > > jakehehrlich wrote:
> > > > Again, you don't need to pass around this sort of data. I will not accept a change that unnecessarily keeps track of endianness and bit width using data when an easy ELFT solution exists. I hate ElfType because it forces you to branch on code you otherwise wouldn't have to and errors can occur where those difference branches have different code. 
> > > My preference would be to refactor or extend the already-existing libObject Decompressor interface to take an ELFT as a template argument. That would allow the complexity to be pushed deeper into the library, and remove some of these issues.
> > 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.
> 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? 
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678