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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Please make sure to rebase this patch when you get a chance, because at the moment, it's a little hard to apply the patch on trunk.



================
Comment at: include/llvm/Object/Compressor.h:65-77
+extern template Expected<std::vector<char>>
+compress<ELF32LE>(StringRef Contents, uint64_t Align,
+                  DebugCompressionType CompressionType);
+extern template Expected<std::vector<char>>
+compress<ELF64LE>(StringRef Contents, uint64_t Align,
+                  DebugCompressionType CompressionType);
+extern template Expected<std::vector<char>>
----------------
plotfi wrote:
> jhenderson wrote:
> > I'm not sure you need all these additional extern declarations.
> I did, couldn't get it to compile otherwise. There might be something I am missing with explicit template instantiation, but this is the best I could figure for now. 
Are you sure that's still the case? In Visual Studio, I get red-squiggly underlines indicating that it doesn't like the code (although compilation works with it still there). I also can compile cleanly on Windows, although I haven't tried on Linux.


================
Comment at: include/llvm/Object/Compressor.h:59
+/// Returns if the section can be compressed based on its name (must have a
+/// debug name, starts with .*debug.
+bool isCompressableSectionName(StringRef Name);
----------------
jhenderson wrote:
> Missing trailing ')'. Also, I think your last "word" should be .debug*, not .*debug...
Okay, I got the second one wrong from misunderstanding what `isCompressableSectionName` was for. However, .*debug is a little too vague, and it should be ".debug or .zdebug" in my opinion (or possibly ".[z]debug"). Please update the appropriate comment.


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-decompress-checkfail.test:1
+# REQUIRES: zlib
+
----------------
Simply say "error" in the name of the test, like in other tests in llvm-objcopy, or even drop "checkfail" entirely, since it doesn't follow our style of test names.

Indeed, a better name for this might be compress-and-decompress-debug-sections[-error].test


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-decompress-checkfail.test:1
+# REQUIRES: zlib
+
----------------
jhenderson wrote:
> Simply say "error" in the name of the test, like in other tests in llvm-objcopy, or even drop "checkfail" entirely, since it doesn't follow our style of test names.
> 
> Indeed, a better name for this might be compress-and-decompress-debug-sections[-error].test
This test doesn't REQUIRE zlib.


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-invalid-format-checkfail.test:1
+# REQUIRES: zlib
+
----------------
Ditto comment on the test name. Just remove "checkfail" since it's implicit.


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-invalid-format-checkfail.test:1
+# REQUIRES: zlib
+
----------------
jhenderson wrote:
> Ditto comment on the test name. Just remove "checkfail" since it's implicit.
This test doesn't REQUIRE zlib.


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test:3
+
+# Reuse compress-debug-sections.test.
+# Here we are testing that zlib-gnu properly sets the right magic header and
----------------
Rather than reusing a test file directly, it might be better to move the yaml into a separate file in the Inputs directory, as it makes it clearer that it might be shared.


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test:8
+# RUN: yaml2obj %S/compress-debug-sections.test -o %t.o
+# RUN: llvm-objcopy --compress-debug-sections=zlib-gnu %t.o %t-zlib-gnu.o
+# RUN: llvm-objcopy --decompress-debug-sections %t-zlib-gnu.o %t-decompressed.o
----------------
This test should probably test the section flags of the compressed section(s) to show whether SHF_COMPRESSED exists or not.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:301
 
+template <typename T>
 static std::unique_ptr<Writer> CreateWriter(const CopyConfig &Config,
----------------
T -> ELFT, here and in other functions.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678