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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

I'm going to outline a proposal for how I think this stuff should work. It turns out I support creating a new section type after all (needed to support a few things like constructing the section name from a Twine and moving the compressed data into the Section's ownership in a clean way). I think this is a faithful version of what James (James please say so if this is not what you had in mind) and I have in mind for how to handle this (well I had to change my stance slightly). If the name ownership problem is solved and we figure out a nice way to plumb the 32-bit vs 64-bit information though that would be even better!

It turns out I was wrong about being able to use ELFSectionWriter. That isn't going to work because the data won't be finalized in all sections at the point. This leaves us with a tricky situation; we need to implement GetData somehow but it isn't 100% clear how. There is currently no way to cast to Section or OwnedDataSection. This creates an unfortunate situation. It turns out this issue has come up in another currently reviewed process. The solution might be to provide a more generic mechanism for getting the original data from a section (as part of SectionBase). The field will be public and called OriginalData. This requires a one line change in the section reading code and would be of general use.

The code in HandleArgs should look like this IMO

  if (Config.CompressDebugSections) {
    for (const SectionBase& Sec : Obj.sections()) {
      if (IsDebugSection(Sec.Name))
        Obj.addSection<CompressedDebugSection>(toCompress.OriginalData);
    }
    RemovePred = [&Config](const SectionBase &Sec) {
      return IsDebugSection(Sec);
    };
  }

Then you'll need to implement a new Section type. It will look something like this

  class CompressedDebugSection : public SectionBase {
    MAKE_SEC_WRITER_FRIEND
  
    uint64_t DecompressedSize;
    uint64_t RequiredAlign;
  
    std::string OwnedName;
    std::vector<uint8_t> CompressedData;
  public:
    CompressedSection(const SectionBase& Sec) {
      CompressedData = CompressData(Sec.OriginalData);
      RequiredAlign = Sec.AddrAlign;
      // String ownership is a little odd here, this idea could use some review. Maybe sections should just own their names
      OwnedName = ZDebugName(Sec.Name);
      Name = OwnedName;
      // The following line smells and is a result of layout needing to know size but not know if this is 32-bit or 64-bit.
      // Here I assume 64-bit because it is larger and will thus work. TBH we can suck a bit on 32-bit though. I would
      // appreciate thoughts on how to solve this issue.
      Size = sizeof(Elf_Chdr64) + CompressedData.size()
    }
  
    void finalize() override;
    void accept(CompressedDebugSection &Visitor) const override;
  };

And then (among some other things) you'll need to implement the ELFSectionWriter code which would look something like this:

  template <class ELFT>
  void ELFSectionWriter<ELFT>::visit(const CompressedDebugSection &Sec) {
    uint8_t *Buf = Out.getBufferStart();
    Buf += Sec.Offset;
    auto *Chdr = reinterpret_cast<typename ELFT::Chdr *>(Buf);
    Chdr.ch_type = ELFCOMPRESS_ZLIB;
    Chdr.ch_size = Sec.DecompressedSize;
    Chdr.ch_addralign = Sec.AddrAlign;
    Buf += sizeof(*Chdr);
    std::copy(std::begin(Sec.CompressedData), std::end(Sec.CompressedData), Buf);
  }

So the list of changes to the code would be

1. Add OriginalData field
2. Initialize OriginalData
3. Write `std::vector<uint8_t> CompressData(ArrayRef<uint8_t>)`
4. Write ZDebugName or just inline it
5. Add the code to handle CompressDebugSections in HandleArgs



================
Comment at: include/llvm/Object/Compressor.h:39
+
+template <>
+void produceCompressedZLibHeader<object::ELF64LE>(support::endian::Writer &W,
----------------
None of these specializations should exist. As far as I can tell they all do the same thing anyway.


================
Comment at: lib/Object/Compressor.cpp:67
+
+template <typename T>
+void produceCompressedZLibHeader(support::endian::Writer &W,
----------------
Please look at how other sections write things. This code is a complete departure from the existing style in the code base. You should not be specializing for ELFT types for instance. You also shouldn't have Elf_Chdr manipulations that look like this or that try and work out the exact order these things happen. The ELFTypes library is very powerful and prevents many many classes of mistakes from occurring.


================
Comment at: tools/llvm-objcopy/Object.cpp:426
 void Section::finalize() { this->Link = LinkSection ? LinkSection->Index : 0; }

+void Section::updateContents(std::unique_ptr<SmallVectorImpl<char>> Update) {
+  ModifiedContents = std::move(Update);
----------------
Again, this is impossible to review. Please resolve this issue with bad diffs.


================
Comment at: tools/llvm-objcopy/Object.cpp:1097
 }

+template <typename T>
+std::unique_ptr<SmallVectorImpl<char>> decompress(StringRef &Name,
----------------
Again, please resolve this bad diff.


================
Comment at: tools/llvm-objcopy/Object.h:49
+  StringRef InputFormat;
+  StringRef BinaryArch;
+
----------------
Please do not move CopyConfig.


================
Comment at: tools/llvm-objcopy/Object.h:196
 private:
+  DebugCompressionType CompressDebugSections = DebugCompressionType::None;
+  bool DecompressDebugSections = false;
----------------
What's up with this diff here? All of these lines change in the diff but don't seem to actually be different. It makes it impossible to review this code. Please fix this.


================
Comment at: tools/llvm-objcopy/Object.h:365
   SYMBOL_HEXAGON_SCOMMON_8 = ELF::SHN_HEXAGON_SCOMMON_8,
   SYMBOL_XINDEX = ELF::SHN_XINDEX,
 };
----------------
You should not be extending Section this way, period.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678