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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked an inline comment as done.
plotfi added a comment.

In https://reviews.llvm.org/D49678#1176191, @jakehehrlich wrote:

> Also also, why do you need this? I ask because from talking with some people about what this is used for there is almost certainly a better way to accomplish any gain you get out of it. The main reason I can still see to do this is to achieve command line compatibility; if that is the goal we can probably noop or just noop with a section rename.
>
> I don't appose actually implementing this BTW, I'm just wondering if there isn't something both simplair and better you could be doing.


I don't understand. Do you mean the entire patch or a specific part of it? The purpose is to have feature parity with GNU Objcopy in this respect.



================
Comment at: include/llvm/Object/Compressor.h:32
+
+  void writeHeader(support::endian::Writer &W, uint64_t DecompressedSize,
+                   unsigned Alignment, bool Is64Bit, bool IsGnuStyle);
----------------
jakehehrlich wrote:
> If possible I'd prefer to use an interface that lets you write to a pointer instead.
Why? Other places where I see similar zlib headers being written seem to just write to a stream. I think a stream would be a cleaner interface without any need to worry about sizing. 


================
Comment at: include/llvm/Object/Compressor.h:45
+private:
+  Compressor() {}
+  Compressor(StringRef Data) : SectionData(Data) {}
----------------
alexshap wrote:
> is the default ctor used anywhere ? - otherwise, we can  remove it 
Sounds good. 


================
Comment at: include/llvm/Object/Compressor.h:46
+  Compressor() {}
+  Compressor(StringRef Data) : SectionData(Data) {}
+  StringRef SectionData;
----------------
alexshap wrote:
> explicit
yes


================
Comment at: lib/Object/Compressor.cpp:32
+StringRef Compressor::getGnuZDebugSectionName(const StringRef Name) {
+  return StringSwitch<StringRef>(Name)
+      .Case(".debug_str", ".zdebug_str")
----------------
jakehehrlich wrote:
> 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?
Mainly did this due to the use of StringRef. I'll have to think of a smarter way to handle this I agree. 


================
Comment at: tools/llvm-objcopy/Object.h:397
   void finalize() override;
+  virtual bool isCompressable() const override {
+    StringRef GnuZDebugName =
----------------
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.


================
Comment at: tools/llvm-objcopy/Object.h:215
 
+  constexpr bool isBigEndian() const {
+    static_assert((std::is_same<ELFT, object::ELF64LE>::value ||
----------------
jakehehrlich wrote:
> 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.
I can remove the static_asserts. Is that what you meant? Or are you saying don't pass isBigEndian to the compressor? Could you clarify? 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:594
 
+  if (InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections) == "zlib-gnu")
+    Config.CompressDebugSections = DebugCompressionType::GNU;
----------------
jhenderson wrote:
> jakehehrlich wrote:
> > nit: Can you use a StringCase for this?
> I assume this comment is meant to be in the previous block?
> 
> Another nit: this line looks too long. Clang-format?
Sure.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678