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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've run out of time of review the rest of this change today, but I will get back to it next week. Please could you try to fix the issues I've highlighted in the meantime.



================
Comment at: include/llvm/Object/Compressor.h:13-22
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCTargetOptions.h"
+#include "llvm/Object/ELFObjectFile.h"
+#include "llvm/Object/ELFTypes.h"
+#include "llvm/Object/ObjectFile.h"
----------------
A number of these headers may be unnecessary now, I suspect. Please reduce this list to the minimal set required by the build.


================
Comment at: include/llvm/Object/Compressor.h:34
+  /// @param W Destination buffer stream for compressed data.
+  Error writeCompressedSectionData(support::endian::Writer &W) {
+    SmallVector<char, 128> CompressedBuffer;
----------------
Don't abbreviate, if you can avoid it: W -> Writer, E ->Err, here and elsewhere in this file.

This function could probably be moved into the .cpp.


================
Comment at: include/llvm/Object/Compressor.h:55
+Expected<std::string>
+getNewDebugSectionName(const StringRef Name,
+                       DebugCompressionType CompressionType);
----------------
Drop the const here.


================
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);
----------------
Missing trailing ')'. Also, I think your last "word" should be .debug*, not .*debug...


================
Comment at: include/llvm/Object/Compressor.h:62
+
+template <typename T> void writeReserved(support::endian::Writer &W) {
+  if (T::Is64Bits)
----------------
Use ELFT rather than T, here and elsewhere.

Rather than putting these functions in the public header, move them into the .cpp and use explicit instantiation.


================
Comment at: include/llvm/Object/Compressor.h:79
+  if (CompressionType == DebugCompressionType::GNU) {
+    const StringRef Magic = "ZLIB";
+    W.OS << Magic;
----------------
Don't use "const StringRef".


================
Comment at: lib/Object/Compressor.cpp:10-18
+#include "llvm/Object/Compressor.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/BinaryFormat/ELF.h"
+#include "llvm/Object/ELFObjectFile.h"
+#include "llvm/Support/Compression.h"
+#include "llvm/Support/DataExtractor.h"
+#include "llvm/Support/Endian.h"
----------------
Same applies here as to what I said about the header include list. Remove all but those required by the build (see the LLVM coding standards).


================
Comment at: lib/Object/Compressor.cpp:41
+      (CompressionType == DebugCompressionType::GNU) ? ".z" : ".";
+  return Prefix + Name.substr(Name.startswith(".debug") ? 1 : 2).str();
+}
----------------
Use Twine concatenation for the whole thing. I think this should work:

`return (Prefix + Name.substr(Name.startswith(".debug") ? 1 : 2)).str();`


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-decompress-xfail.test:3
+
+# XFAIL: *
+
----------------
This isn't the purpose of XFAIL. XFAIL should be used to mark a test that SHOULD pass, but there is an issue that prevents it passing.

What you want is to put a not in front of the command that will "fail", and also to check the error output. Something like the following:

```
# RUN: not llvm-objcopy --compress-debug-sections=zlib --decompress-debug-sections %t-clean.o 2>&1 | FileCheck %s

# CHECK: the error message
```

You can see plenty of examples of this in the existing llvm-objcopy tests.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678