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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

Sorry, I'm not ready to land this yet.



================
Comment at: include/llvm/Object/Compressor.h:104
+template <typename T>
+Expected<std::unique_ptr<SmallVectorImpl<char>>>
+compress(StringRef Contents, uint64_t Align,
----------------
I think an Expected<std::vector<char>> would be best. move semantics should make it efficient in all the cases we care about.

Side note: having a move only wrapper type that didn't allocate might be a cool addition to ADT


================
Comment at: tools/llvm-objcopy/Object.cpp:90
+bool SectionBase::isCompressed() const {
+  return Name.startswith(".zdebug") || (Flags & ELF::SHF_COMPRESSED);
+}
----------------
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,


================
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);
 }
----------------
This is fine as is. Please don't change this.


================
Comment at: tools/llvm-objcopy/Object.h:242
+protected:
+  ArrayRef<uint8_t> OriginalData;
+
----------------
This should just be public.


================
Comment at: tools/llvm-objcopy/Object.h:271-272
   virtual void markSymbols();
+  virtual ArrayRef<uint8_t> getOriginalContents() const;
+  bool isCompressed() const;
 };
----------------
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.


================
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; }
 
----------------
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.


================
Comment at: tools/llvm-objcopy/Object.h:336
+protected:
+  std::string OwnedName;
   std::vector<uint8_t> Data;
----------------
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.


================
Comment at: tools/llvm-objcopy/Object.h:339
 
+  void setName(std::string NewName) {
+    OwnedName = NewName;
----------------
this method is not needed. Name is public.


================
Comment at: tools/llvm-objcopy/Object.h:366
+public:
+  CompressedSection(StringRef NewName, ArrayRef<uint8_t> Data,
+                    const SectionBase &Sec)
----------------
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.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:332
+Expected<std::unique_ptr<SmallVectorImpl<char>>>
+decompress(const SectionBase &Section, const CopyConfig &Config,
+           ElfType OutputElfType) {
----------------
I think I'd prefer if you handled decompression in another patch.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:381
+  case ELFT_ELF32LE:
+    return object::compress<object::ELF32LE>(Contents, Section.Align,
+                                             Config.CompressDebugSections);
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678