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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

Yeah I really really think this should be handled by constructing new OwnedDataSections. The compression code should also be templated over an ELFT as well to enable using ELFT::Chdr.



================
Comment at: include/llvm/Object/Compressor.h:32
+
+  void writeHeader(support::endian::Writer &W, uint64_t DecompressedSize,
+                   unsigned Alignment, bool Is64Bit, bool IsGnuStyle);
----------------
plotfi wrote:
> jakehehrlich wrote:
> > If possible I'd prefer to use an interface that lets you write to a pointer instead.
> Why? Other places where I see similar zlib headers being written seem to just write to a stream. I think a stream would be a cleaner interface without any need to worry about sizing. 
Mainly It's just how things are done in llvm-objcopy so it's mainly for local consistency. This isn't a big point, more a point of preference. Also I think you can avoid a copy of the data that way. Instead of copying it to a stream and then copying that to the final destination you should just be able to copy once from the compressed buffer to the owned data.


================
Comment at: lib/Object/Compressor.cpp:109
+                                        unsigned Alignment, bool Is64Bit) {
+  if (Is64Bit) {
+    // Write Elf64_Chdr header.
----------------
To further my previous point, if you use an ELFT::Chdr you won't need to check this. That's the general mechanism by which we avoid having to check 64/32 bit and little/big endian. Then you don't need those functions that detect that information. It's far less error prone. 


================
Comment at: lib/Object/Compressor.cpp:111
+    // Write Elf64_Chdr header.
+    W.write(static_cast<ELF::Elf64_Word>(ELF::ELFCOMPRESS_ZLIB));
+    W.write(static_cast<ELF::Elf64_Word>(0)); // ch_reserved field.
----------------
With a pointer interface these clean up a lot and just become regular old assignments to a Chdr struct.


================
Comment at: tools/llvm-objcopy/Object.h:49
 
+struct CopyConfig {
+  StringRef OutputFilename;
----------------
Why is the CopyConfig being added here?


================
Comment at: tools/llvm-objcopy/Object.h:265
+
+  constexpr bool is64Bit() const {
+    static_assert((std::is_same<ELFT, object::ELF64LE>::value ||
----------------
jhenderson wrote:
> I'm pretty sure all ELFTs have a member that allows querying 64-bits directly on the type, making this function unnecessary.
They do but we shouldn't be checking this information either way.


================
Comment at: tools/llvm-objcopy/Object.h:387
+  SmallVector<char, 128> ModifiedContents;
+  bool doCompressionFinalize = false;
   SectionBase *LinkSection = nullptr;
----------------
jhenderson wrote:
> doCompressionFinalize -> DoCompressionFinalize
> 
> But this member doesn't feel like it should exist at all. It probably indicates a new section type is needed for compressed sections.
I thought about this at first but I think these sections can be handled by OwnedDataSection. e.g. they should be constructed and added to the Object rather than a special interface being added to mutate the sections.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678