[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:167
+    W.OS << Magic;
+    support::endian::write(W.OS, DecompressedSize, support::big);
+    return Error::success();
----------------
jakehehrlich wrote:
> 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.
For gnu style, as far as I’ve seen in every other place in llvm it’s always big endian.


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


================
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;
+  }
----------------
jakehehrlich wrote:
> Don't change this constructor. OriginalData should already work if you rebase.
I did rebase, I’ll double check. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:355
+  Expected<Decompressor> D = object::Decompressor::create(
+      Section.Name, Contents, (ELFT::TargetEndianness == endianness::little),
+      ELFT::Is64Bits);
----------------
jakehehrlich wrote:
> 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.
I’ve dropped decompressing in this patch. I’ve given up on that here for now. Forget it.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678