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

[Puyan Lotfi via Phabricator via llvm-commits]

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


================
Comment at: test/tools/llvm-objcopy/zlib-dwarf.test:1
+# REQUIRES: shell
+
----------------
jhenderson wrote:
> Shouldn't this be `REQUIRES: zlib`? Why do you need to require a shell?
requires shell just because it's using a couple commands like cmp and others but I added  REQUIRES: zlib too. 


================
Comment at: test/tools/llvm-objcopy/zlib-dwarf.test:4
+# RUN: yaml2obj %s  > %t-clean.o
+# RUN: llvm-objdump -s %t-clean.o -section=.debug_str | grep -i clang
+
----------------
probinson wrote:
> `grep` in tests is deprecated.  Use FileCheck.
I will address this in another patch update (specifically for the test). 


================
Comment at: test/tools/llvm-objcopy/zlib-dwarf.test:25
+
+# RUN: llvm-objdump -s %t-clean2.o   | grep " section \." | cut -f1 -d':' | grep " \.debug" | cut -f4 -d' ' | sort | xargs -I% llvm-objdump -s -section=% %t-clean2.o   | grep -v "file format" > %t-clean2.o.txt
+# RUN: llvm-objdump -s %t-zlib.o     | grep " section \." | cut -f1 -d':' | grep " \.debug" | cut -f4 -d' ' | sort | xargs -I% llvm-objdump -s -section=% %t-zlib.o     | grep -v "file format" > %t-zlib.o.txt
----------------
jhenderson wrote:
> Please break this up over multiple lines using '\':
> 
> ```
> # RUN: llvm-objdump -s %t-clean2.o | \
> # RUN:     grep " section \." | \ 
> ...
> ```
> 
> I actually have zero idea what you're doing here. Perhaps we could simplify this significantly by showing that the section gets smaller after compression, and then decompressing it and showing the content is the same.
Sure, here I am greping out the section names that are .debug, sorting them (so that they are in the same order regardless of if I got them from the original or the compressed+decompressed file), and then using the names to pass to llvm-objdump to dump out each section in sorted order to the txt file that will be diffed with the original in the later line. 


================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:21
+defm compress_debug_sections : Eq<"compress-debug-sections">,
+                     MetaVarName<"[ none | zlib | zlib-gnu (deprecated) ]">,
+                     HelpText<"Enable zlib-gnu or zlib Compression of DWARF debug sections.">;
----------------
jhenderson wrote:
> Why is zlib-gnu marked as deprecated here?
I thought the GNU format was an older one, in any case I dropped this. 


================
Comment at: tools/llvm-objcopy/Object.h:269-272
+  virtual const StringRef getContents() const {
+    assert(false && "Section has no Contents.");
+    return "";
+  }
----------------
jhenderson wrote:
> Don't mark return types as const (except for pointers to const and reference returns): the caller can just ignore the const and copy the value, so it has zero use (indeed some compilers actually warn about this). It's also completely redundant for a StringRef, since StringRefs are implicitly const.
> 
> Should this function just be a pure abstract function if it can't be called?
> 
> I also don't know whether "getContents" means get original contents or get contents at the end. Please rename more appropriately.
What do you mean? I guess the tricky thing here is, it depends on the type of object. I don't see this being useful for anything other than a Section or a OwnedDataSection or a Compressed/Decompressed section. 


================
Comment at: tools/llvm-objcopy/Object.h:368
+
+  CompressedSection(std::unique_ptr<SmallVectorImpl<char>> CompressedData)
+      : ModifiedData(std::move(CompressedData)) {}
----------------
jhenderson wrote:
> I'm pretty sure you're supposed to use `SmallVector` directly.
But a SmallVector of what size? As far as I know SmallVector always has some size N. In a lot of places when I see SmallVector passed around (as in the case of raw_ostream) it's passed as a SmallVectorImpl&. Should this not be the case here?


Repository:
  rL LLVM

https://reviews.llvm.org/D49678