[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:90
+bool SectionBase::isCompressed() const {
+  return Name.startswith(".zdebug") || (Flags & ELF::SHF_COMPRESSED);
+}
----------------
jakehehrlich wrote:
> I find it dubious that any notion of "is compressed" should have something to do with the name. Use the flag to determine this and ignore the name,
But as far as I can tell, Gnu Style uses both the name and the magic string header. I can check for the "ZLIB" magic here too. 


================
Comment at: tools/llvm-objcopy/Object.cpp:139
   uint8_t *Buf = Out.getBufferStart() + Sec.Offset;
-  std::copy(std::begin(Sec.Contents), std::end(Sec.Contents), Buf);
+  std::copy(std::begin(Sec.OriginalData), std::end(Sec.OriginalData), Buf);
 }
----------------
jakehehrlich wrote:
> This is fine as is. Please don't change this.
Please clarify. When the OriginalData patch lands will this be changed to the right thing?


================
Comment at: tools/llvm-objcopy/Object.h:271-272
   virtual void markSymbols();
+  virtual ArrayRef<uint8_t> getOriginalContents() const;
+  bool isCompressed() const;
 };
----------------
jakehehrlich wrote:
> I don't think there's any reason for these to exist. You can make an isCompressed function externally if you'd like but it shouldn't be a method.
It _was_ external, I made it a method based on Jame's previous review...


================
Comment at: tools/llvm-objcopy/Object.h:324
 public:
-  explicit Section(ArrayRef<uint8_t> Data) : Contents(Data) {}
+  explicit Section(ArrayRef<uint8_t> Data) { OriginalData = Data; }
 
----------------
jakehehrlich wrote:
> OriginalData will land soon in another change. OriginalData shouldn't be set by a constructor.  The constructor was fine as is for the purposes of this change.
ETA?


================
Comment at: tools/llvm-objcopy/Object.h:336
+protected:
+  std::string OwnedName;
   std::vector<uint8_t> Data;
----------------
jakehehrlich wrote:
> So it turns out that we want to make names std::string instead of StringRefs anyway so this is no longer needed. (two other changes also needed this). That will likely be landing soon.
ETA?


================
Comment at: tools/llvm-objcopy/Object.h:366
+public:
+  CompressedSection(StringRef NewName, ArrayRef<uint8_t> Data,
+                    const SectionBase &Sec)
----------------
jakehehrlich wrote:
> I think you'll want to put the compression of the Data here personally. Not a blocker to an LGTM but a preference of mine. That way you can store the information needed to construct the header here rather than importing the compressed data. It will give a nicer interface I think.
I still need to think about how to do this, because while it is a nicer interface I do want the compressed section to replace the original _only if_ it is smaller. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:332
+Expected<std::unique_ptr<SmallVectorImpl<char>>>
+decompress(const SectionBase &Section, const CopyConfig &Config,
+           ElfType OutputElfType) {
----------------
jakehehrlich wrote:
> I think I'd prefer if you handled decompression in another patch.
I could do that, but I was planning on having some testing based on roundtripping the compression/decompression. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:381
+  case ELFT_ELF32LE:
+    return object::compress<object::ELF32LE>(Contents, Section.Align,
+                                             Config.CompressDebugSections);
----------------
jakehehrlich wrote:
> This is a *huge* blocker. Avoid  wherever possible manually matching ElfType with ELFT types. It is just error prone and gross. You can solve this by moving this to the SectionWriter as I outlined in my proposal.
With your proposal, as far as I could tell it would be too late for the section writer to back out of writing the section. Right now, the compression only happens when it is attempted and it is actually smaller than previously. Also, I've seem several places where ElfTypes are matched to ELFT types so I wasn't aware that this would be a huge blocker. 


The problem here is I'm just not sure how you get this behavior without attempting the compression in HandleArgs. 


Repository:
  rL LLVM

https://reviews.llvm.org/D49678