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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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

> Also I think you can build llvm with or without zlib. I think that means you're going to need to #ifdef a lot of this code.


As an additional point, it would be good to minimise the number of distinct areas where we have to add these #ifdefs. I'd recommend trying to hide as much as possible of this behind an interface of some kind, which turns into a no-op or warning/error if --compress-debug-sections is used without zlib support.



================
Comment at: tools/llvm-objcopy/Object.cpp:495-496
+
+  if (!doCompressionFinalize)
+    return;
+
----------------
This bugs me. It maybe suggests to me that a compressed section should be a new section type.


================
Comment at: tools/llvm-objcopy/Object.cpp:1272
 
+  bool IsGnuStyle = (CompressDebugSections == DebugCompressionType::GNU);
+  bool doCompress = (CompressDebugSections != DebugCompressionType::None) &&
----------------
Why not just pass the compression style into the function directly? That way, we can add further methods later on.


================
Comment at: tools/llvm-objcopy/Object.cpp:1273
+  bool IsGnuStyle = (CompressDebugSections == DebugCompressionType::GNU);
+  bool doCompress = (CompressDebugSections != DebugCompressionType::None) &&
+                    !DecompressDebugSections;
----------------
doCompress -> DoCompress.


================
Comment at: tools/llvm-objcopy/Object.cpp:1280
+    for (auto &Section : Obj.sections()) {
+
+      if (doCompress && Section.isCompressable()) {
----------------
Nit: spurious extra blank line.


================
Comment at: tools/llvm-objcopy/Object.h:265
+
+  constexpr bool is64Bit() const {
+    static_assert((std::is_same<ELFT, object::ELF64LE>::value ||
----------------
I'm pretty sure all ELFTs have a member that allows querying 64-bits directly on the type, making this function unnecessary.


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


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


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:594
 
+  if (InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections) == "zlib-gnu")
+    Config.CompressDebugSections = DebugCompressionType::GNU;
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D49678