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

Wed Aug 15 11:37:51 PDT 2018

plotfi marked 3 inline comments as done.
plotfi added a comment.

In https://reviews.llvm.org/D49678#1200360, @jhenderson wrote:

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

Ah, sorry about that. I was just checking what was done based on your suggestions. Will do.

> 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>>
----------------
jhenderson wrote:
> 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.
Ah! I thought you needed to have the instantiations in the headers too. Alright great. 

================
Comment at: test/tools/llvm-objcopy/Inputs/compress-debug-sections.yaml:1
+--- !ELF
+FileHeader:
----------------
jhenderson wrote:
> 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.
Ah, alright. I'll add back the comment section. I made the content somewhat large so that the compression would actually make it smaller (and not bail out). I can also add the relocation sections back too. I had trouble removing the stack section. I'll try and remove it through llvm-objcopy.

================
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
----------------
jhenderson wrote:
> 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).
Good point!

================
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
+
----------------
jhenderson wrote:
> I'm not sure what this is checking? Is it the contents?
Yes, 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
----------------
jhenderson wrote:
> 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
> ```
I do this using redirects so that the filename doesn't get printed. Otherwise I have to grep -v "file format ELF64-x86-64" before diffing.

Repository:
  rL LLVM

https://reviews.llvm.org/D49678