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

Wed Aug 15 03:36:48 PDT 2018

jhenderson added a comment.

Could I make one request going forwards. It's helpful if you post a quick summary of the changes you made each time you update the diff (a bullet-point list would be fine). Due to the scale of the change, I'd prefer not to re-review everything each time I look, and sometimes it's a little hard to remember whether a bit has changed.

In https://reviews.llvm.org/D49678#1199294, @plotfi wrote:

>

> I did. There seem to have been changes since I submitted. I'd like to be able to push this soon. Is there anything else that needs addressing aside from the test names/content and comments?

This change is certainly improving, but there are still a few things I'd like addressing (see inline comments). I also want @jakehehrlich to comment on the ELFT-related changes.

================
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:
> > 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.
> With:
> 
> 
> ```
> template <typename ELFT>
> Expected<std::vector<char>> compress(StringRef Contents, uint64_t Align,
>                                      DebugCompressionType CompressionType);
> template Expected<std::vector<char>>
> compress<ELF32LE>(StringRef Contents, uint64_t Align,
>                   DebugCompressionType CompressionType);
> template Expected<std::vector<char>>
> compress<ELF64LE>(StringRef Contents, uint64_t Align,
>                   DebugCompressionType CompressionType);
> template Expected<std::vector<char>>
> compress<ELF32BE>(StringRef Contents, uint64_t Align,
>                   DebugCompressionType CompressionType);
> template Expected<std::vector<char>>
> compress<ELF64BE>(StringRef Contents, uint64_t Align,
>                   DebugCompressionType CompressionType);
> ```
> 
> I get tons of compile errors like these (with clang-6.0++):
> 
> 
> ```
> ../include/llvm/Object/Compressor.h:66:1: error: explicit instantiation of undefined function template 'compress'
> compress<ELF32LE>(StringRef Contents, uint64_t Align,
> ^
> ../include/llvm/Object/Compressor.h:63:29: note: explicit instantiation refers here
> Expected<std::vector<char>> compress(StringRef Contents, uint64_t Align,
> ```
> 
> 
Ah, I think you misunderstood what I meant. What you should have is:

Header file:
```
template <typename ELFT>
Expected<std::vector<char>> compress(StringRef Contents, uint64_t Align,
                                     DebugCompressionType CompressionType);
```

Source file:
```
template <typename ELFT>
Expected<std::vector<char>> compress(StringRef Contents, uint64_t Align,
                                     DebugCompressionType CompressionType) {
// function definition.
}

template Expected<std::vector<char>>
compress<ELF32LE>(StringRef Contents, uint64_t Align,
                  DebugCompressionType CompressionType);
// plus the other instantiations...
```

You don't need any declaration in the header, except the template function declaration itself. This indicates to callers that there is a function available with the specified signature, allowing them to create undefined references to it. The source file then contains the definition, and finally the explicit instantiation. The explicit instantiation has to happen after the function definition (and in the same compile unit), because otherwise the compiler doesn't know how to instantiate the template function. Other compile units don't need to know anything about what instantiations of the template function exist, hence there is no need to declare the explicit instantiation in the header file. Does that make sense?

I've built this with Linux as well, and it compiles fine with my stock compiler there too, without the additional declarations.

================
Comment at: lib/Object/Compressor.cpp:34
+    return createStringError(llvm::errc::invalid_argument,
+                             "Invalid Debug Section Name: %s.",
+                             Name.str().c_str());
----------------
You don't need to capitalize "Debug", "Section" and "Name" -> "Invalid debug section name"

================
Comment at: lib/Object/Compressor.cpp:54
+        llvm::errc::invalid_argument,
+        "Invalid DebugCompressionType, only GNU and ZLIB are supported.");
+  }
----------------
Nit: change the ',' to a ':'.

================
Comment at: test/tools/llvm-objcopy/Inputs/compress-debug-sections.yaml:1
+--- !ELF
+FileHeader:
----------------
Are you planning any more updates to this test? Because I still don't see why you need such a large section.

You also definitely don't need the GNU stack section, or the symbols.

I think that one thing that is important is having relocations patching the debug section, because these need to point at the decompressed offset, not something else. You should add one that patches somewhere in the middle of your (small) section, and another that uses a section-relative reference and addend, referencing the compressed section.

Finally, you should have testing that shows that non-debug sections are not compressed.

================
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
----------------
plotfi wrote:
> jhenderson wrote:
> > This test should probably test the section flags of the compressed section(s) to show whether SHF_COMPRESSED exists or not.
> Not this test, but I will add that check to compress-debug-sections-zlib.test. GNU Style doesn't use or set that flag. 
Actually, since GNU style doesn't set that flag, we should check that it doesn't in this test (and does in the other test like you do).

================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib.test:18-19
+# CHECK-ZLIB: .debug_str
+# CHECK-ZLIB: 01000000 00000000 d0010000 00000000
+# CHECK-ZLIB: 01000000 00000000 789c4550
+
----------------
I'm not sure what this is checking? Is it the contents?

================
Comment at: test/tools/llvm-objcopy/compress-debug-sections.test:11
+
+# RUN: llvm-objdump -s -section=.debug_str - < %t.o  > %t.txt
+# RUN: llvm-objdump -s -section=.debug_str - < %t2.o > %t2.txt
----------------
You don't need to use "-" and "<". In this case, it probably makes more sense to simply pass in the object file directly i.e:

```
# RUN: llvm-objdump -s -section=.debug_str %t.o > %t.txt
```

But I think you could also do something like:
```
# RUN: llvm-objcopy --decompress-debug-sections %tz.o - | \
# RUN:    llvm-objdump -s -section=.debug_str - > %t2.txt
```

================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:704
+template <typename ELFT>
+static bool HandleArgsAndWrite(const CopyConfig &Config, Object &Obj,
+                               const Reader &Reader, Binary &Binary,
----------------
Nit: use lower case function name.

Repository:
  rL LLVM

https://reviews.llvm.org/D49678