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

Mon Aug 6 06:51:44 PDT 2018

jhenderson added inline comments.

================
Comment at: lib/Object/Compressor.cpp:26
+
+StringRef getDebugSectionName(const StringRef Name, bool IsGnuStyle) {
+
----------------
plotfi wrote:
> jhenderson wrote:
> > jakehehrlich wrote:
> > > naming convention is to use capitals for non-methods. Same things below.
> > I'm not sure I see anywhere using the incorrect style for LLVM?
> Should getDebugSectionName be GetDebugSectionName. I forget the LLVM style. 
getDebugSectionName is the style, as far as I can see. I've discussed this with @jakehehrlich offline, and I think the llvm-objcopy code uses the incorrect style in some places.

================
Comment at: lib/Object/Compressor.cpp:28-29
+
+  if (!Name.startswith(".debug") && !Name.startswith(".zdebug"))
+    return "";
+
----------------
plotfi wrote:
> jhenderson wrote:
> > This feels like the wrong way to use the function for non-debug sections. Either I'd return an Optional, an Expected (if we want to treat it as an error in the caller) or just assert, or finally possibly just return the Name.
> The idea is to just get the .zdebug or .debug name based on whatever name you provided depending on if you asked for it to be in gnu style. What is the problem here?
This has got slightly out-of-line. I was referring to the first if statement and return "". I don't think "empty string" is a reasonable result of calling this function - we can do better using Optionals or Expecteds as I suggested.

================
Comment at: lib/Object/Compressor.cpp:29
+
+  std::string LookupName = Name.substr(Name.startswith(".debug") ? 1 : 2).str();
+  std::string debugName, zdebugName, Dot = ".";
----------------
Why are you converting this to a std::string here, rather than perform the concatenations and then turning that into std::string?

================
Comment at: lib/Object/Compressor.cpp:32
+  std::tie(debugName, zdebugName) =
+    std::make_tuple(Dot + LookupName, (Dot + "z") + LookupName);
+  return IsGnuStyle ? zdebugName : debugName;
----------------
I don't think wrapping "." in a std::string is beneficial at all. Just do `"." + LookupName` (and `".z" + LookupName`).

================
Comment at: test/tools/llvm-objcopy/zlib-dwarf.test:1
+# REQUIRES: shell
+
----------------
Shouldn't this be `REQUIRES: zlib`? Why do you need to require a shell?

================
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
----------------
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.

================
Comment at: test/tools/llvm-objcopy/zlib-dwarf.test:43
+    AddressAlign:    0x0000000000000010
+    Content:         554889E54883EC1048B80000000000000000897DFC8975F88B75FC8B55F84889C7B000E8000000008B55FC0355F88945F489D04883C4105DC3
+  - Name:            .rela.text
----------------
This test has far too much noise in it. Please reduce it to the smallest possible thing that shows that we compress and decompress a debug section correctly. There is no need for large numbers of sections, relocations etc to achieve this.

================
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.">;
----------------
Why is zlib-gnu marked as deprecated here?

================
Comment at: tools/llvm-objcopy/Object.h:269-272
+  virtual const StringRef getContents() const {
+    assert(false && "Section has no Contents.");
+    return "";
+  }
----------------
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.

================
Comment at: tools/llvm-objcopy/Object.h:332
+  const StringRef getContents() const override {
+    return StringRef((const char *)Contents.data(), Contents.size());
+  }
----------------
Why not just return Contents directly, and make this an ArrayRef<uint8_t> return type?

================
Comment at: tools/llvm-objcopy/Object.h:363
+
+  void setName(std::string Name) {
+    OwnedName = Name;
----------------
It would be cleaner to just call the parameter "NewName" so that you don't have to explicitly specify `this->` below.

================
Comment at: tools/llvm-objcopy/Object.h:368
+
+  CompressedSection(std::unique_ptr<SmallVectorImpl<char>> CompressedData)
+      : ModifiedData(std::move(CompressedData)) {}
----------------
I'm pretty sure you're supposed to use `SmallVector` directly.

================
Comment at: tools/llvm-objcopy/Object.h:389
+
+class DecompressedSection : public CompressedSection {
+public:
----------------
A `DecompressedSection` isn't a `CompressedSection`. One is compressed, the other isn't. Please don't use inheritance to do code re-use.

================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:335
+                                                  StringRef Contents) {
+
+  auto ModifiedContents = make_unique<SmallVector<char, 128>>();
----------------
jhenderson wrote:
> Remove this blank line. Please don't put extra lines at the start of functions. This is not the style of LLVM.
There is still a blank line at the start of `decompress`. Please remove it.

================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:354
+    auto Err = D.takeError();
+    logAllUnhandledErrors(std::move(Err), errs(),
+                          "[llvm-objcopy] zlib decompression error.");
----------------
Please use the existing functions in this file to report errors, not something else.

================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:576

+  if (Config.CompressDebugSections != DebugCompressionType::None) {
+    bool HasCompressedAnySections = false;
----------------
Move the body of this if into a separate function, if possible, please.

================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:589
+
+      bool isSmaller = std::get<1>(Result);
+      bool doBail = !isSmaller || (nullptr == std::get<2>(Result));
----------------
Variables use upper case. Please fix here and elsewhere.

================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:604
+      // Replace this Section with a compressed version.
+      Section.IsMarkedForRemoval = true;
+      HasCompressedAnySections = true;
----------------
Don't do this. Just update RemovePred in this sort of way:

```
RemovePred = [RemovePred, Section](SectionBase &Sec) {
  return &Sec == &Section || RemovePred(Sec);
}
```

Repository:
  rL LLVM

https://reviews.llvm.org/D49678