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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 3 inline comments as done.
plotfi added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:495-496
+
+  if (!doCompressionFinalize)
+    return;
+
----------------
jhenderson wrote:
> This bugs me. It maybe suggests to me that a compressed section should be a new section type.
Can you elaborate? Not sure I understand what you mean (may have lost some context, sorry, most recent update I did -U9999).


================
Comment at: tools/llvm-objcopy/Object.h:357
+  }
+  virtual void compress(bool IsGnuStyle, bool Is64Bit,
+                        bool IsBigEndian) override {
----------------
jakehehrlich wrote:
> 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.
I don't completely understand what you mean here. Can you elaborate? I thought debug sections were the only ones that can be compressed. At least for the first commit that is all I want to do.


================
Comment at: tools/llvm-objcopy/Object.h:49
 
+struct CopyConfig {
+  StringRef OutputFilename;
----------------
jakehehrlich wrote:
> Why is the CopyConfig being added here?
Because instead of simply passing bool WSH to ELFWriter's constructor I thought it would be way better to just pass a reference to CopyConfig so that it could have access to whatever configs it needs to see. 


================
Comment at: tools/llvm-objcopy/Object.h:387
+  SmallVector<char, 128> ModifiedContents;
+  bool doCompressionFinalize = false;
   SectionBase *LinkSection = nullptr;
----------------
jakehehrlich wrote:
> 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.
Removed it. 


================
Comment at: tools/llvm-objcopy/Object.h:387
+  SmallVector<char, 128> ModifiedContents;
+  bool doCompressionFinalize = false;
   SectionBase *LinkSection = nullptr;
----------------
plotfi wrote:
> jakehehrlich wrote:
> > 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.
> Removed it. 
Still need to study the code some more before I understand what you mean by handling this with OwnedDataSection. Will look at it in my next patch/phab update. 


================
Comment at: tools/llvm-objcopy/Object.h:397
   void finalize() override;
+  virtual bool isCompressable() const override {
+    StringRef GnuZDebugName =
----------------
plotfi wrote:
> jhenderson wrote:
> > alexshap wrote:
> > > here and below - i think the implementations should be moved to Object.cpp
> > Also, don't mark functions with both `virtual` and `override`. The `override` implies `virtual`, so only use the latter (similar to elsewhere in this file).
> Yes.
Thanks @jhenderson, I did not know that. 


Repository:
  rL LLVM

https://reviews.llvm.org/D49678