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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: include/llvm/Object/Compressor.h:32
+
+  void writeHeader(support::endian::Writer &W, uint64_t DecompressedSize,
+                   unsigned Alignment, bool Is64Bit, bool IsGnuStyle);
----------------
If possible I'd prefer to use an interface that lets you write to a pointer instead.


================
Comment at: lib/Object/Compressor.cpp:32
+StringRef Compressor::getGnuZDebugSectionName(const StringRef Name) {
+  return StringSwitch<StringRef>(Name)
+      .Case(".debug_str", ".zdebug_str")
----------------
I'm fine using a curated list but honestly we've just been checking if the section name starts with ".debug_" which from testing is what we've discovered GNU objcopy does. The list of debug section names is finite and in the DWARF and I think this is better but the inconsistency of using ".debug_" prefix one place and a curated list in another bothers me. If you want to keep the curated list will you add a TODO on the code that handles StripDebug to indicate that we should switch to a curated list?


================
Comment at: tools/llvm-objcopy/Object.h:215
 
+  constexpr bool isBigEndian() const {
+    static_assert((std::is_same<ELFT, object::ELF64LE>::value ||
----------------
You should probably not be checking this ever.  I'm going to need a very *very* good reason to allow this function to be used. The whole reason we (and LLD) haven't had any bugs with endianess is because we don't do these sorts of checks. Instead we use the integer types from ELFT in a uniform way.


================
Comment at: tools/llvm-objcopy/Object.h:225
+
+  constexpr bool is64Bit() const {
+    static_assert((std::is_same<ELFT, object::ELF64LE>::value ||
----------------
Same goes here.


================
Comment at: tools/llvm-objcopy/Object.h:242
+  ELFWriter(Object &Obj, Buffer &Buf, bool WSH,
+            DebugCompressionType CompressDebugSections)
+      : Writer(Obj, Buf), WriteSectionHeaders(WSH) {
----------------
Hmm...I think I'd prefer this be handled at the section level so that the writer never needs to know about this. I think for each section to be compressed that an OwnedDataSection containing the compressed data should be added.


================
Comment at: tools/llvm-objcopy/Object.h:289
   virtual void markSymbols();
+  virtual bool isCompressable() const { return false; }
+  virtual void compress(bool IsGnuStyle, bool Is64Bit, bool IsBigEndian) {
----------------
I think this can just be a regular function that checks the name and flags instead of a virtual method.


================
Comment at: tools/llvm-objcopy/Object.h:357
+  }
+  virtual void compress(bool IsGnuStyle, bool Is64Bit,
+                        bool IsBigEndian) override {
----------------
You can compress an arbitrary section (regardless of weather it should be compressible or not) by first writing the contents to a buffer using an ELFSectionWriter on just that section to get its contents. That will for instance handle both Section and OwnedDataSection and means that first adding a debug section and the compressing is 100% ok.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:594
 
+  if (InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections) == "zlib-gnu")
+    Config.CompressDebugSections = DebugCompressionType::GNU;
----------------
nit: Can you use a StringCase for this?


Repository:
  rL LLVM

https://reviews.llvm.org/D49678