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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: include/llvm/Object/Compressor.h:44
+                                 uint64_t DecompressedSize, unsigned Align) {
+  using Chdr = Elf_Chdr_Impl<T>;
+  W.write(static_cast<decltype(Chdr::ch_type)>(ELF::ELFCOMPRESS_ZLIB));
----------------
This is much more what I had in mind!

Instead of using using Elf_Chdr_Impl<ELFT> we should use typename ELFT::Chdr because that's the interface we're provided by ELFT where as Elf_Chdr_Impl is the implementation that we happen to be aware of.


================
Comment at: include/llvm/Object/Compressor.h:45
+  using Chdr = Elf_Chdr_Impl<T>;
+  W.write(static_cast<decltype(Chdr::ch_type)>(ELF::ELFCOMPRESS_ZLIB));
+  writeReserved<T>(W);
----------------
Instead of writing this way, what if you constructed a full Chdr, assigned the elements to it directly, and then reinteript_cast a pointer of it to a uint8_t* and then write that? That way you don't even have to know the order that the elements are laid out in.  It also handles the writeReserved issue for you! The ELFT interface is directly intended to be used with reinterpret_cast. That's the magic of it.


================
Comment at: lib/Object/Compressor.cpp:26
+
+StringRef getDebugSectionName(const StringRef Name, bool IsGnuStyle) {
+
----------------
naming convention is to use capitals for non-methods. Same things below.


================
Comment at: lib/Object/Compressor.cpp:34
+
+  static std::map<std::string, std::tuple<std::string, std::string>> NameMap;
+  auto I = NameMap.find(LookupName);
----------------
This is not a bad way to solve the problem of storage. In fact you'll see StringSaver used throughout LLVM in just this way. If this was an expensive computation I'd be all for it too. I'd like to hear other people's thoughts on this just because I'm a bit cautious of it. I've come to find the StringSaver pattern to be an anti-pattern and an indication that StringRef was being over used. Unclear if its the case here or not.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:354
+std::unique_ptr<SmallVectorImpl<char>>
+decompress(StringRef &Name, StringRef Contents, ElfType OutputElfType) {
+  switch (OutputElfType) {
----------------
I think you don't need to do this if you create a 'DecompressedSectionType' that knows how to handle write out the decompressed contents because the writer will be able to call the proper decompress<ELFT> method directly!


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:392
+
+std::unique_ptr<SmallVectorImpl<char>> compress(StringRef &Name,
+                                                StringRef Contents,
----------------
Same thing here for compress that I said about decompress.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678