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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 2 inline comments as done.
plotfi added inline comments.


================
Comment at: include/llvm/Object/Compressor.h:62
+
+template <typename T> void writeReserved(support::endian::Writer &W) {
+  if (T::Is64Bits)
----------------
jhenderson wrote:
> jhenderson wrote:
> > 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.
> You haven't changed T to ELFT... I think ELFT is more descriptive as to what type is expected, and is much more common.
Oh my mistake. 


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


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections.test:1
+# REQUIRES: shell
+# REQUIRES: zlib
----------------
jhenderson wrote:
> You still haven't explained why this test needs to be so complicated. Why do you need multiple sections with large amounts of content? We don't need multiple real-world debug sections to test the compress function. A single, fairly small, section named something like .debug_foo would be sufficient.
Working on it. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:722
   std::unique_ptr<Object> Obj = Reader.create();
-
-  HandleArgs(Config, *Obj, Reader, Reader.getElfType());
-
-  std::unique_ptr<Writer> Writer =
-      CreateWriter(Config, *Obj, Out, Reader.getElfType());
-  Writer->finalize();
-  Writer->write();
+  if (HandleArgsAndWrite<ELF32LE>(Config, *Obj, Reader, Binary, Out) ||
+      HandleArgsAndWrite<ELF64LE>(Config, *Obj, Reader, Binary, Out) ||
----------------
jhenderson wrote:
> I'm not sure I like this change, but it's probably best to let Jake comment on it before you make further changes.
As far as I could tell, the only use for the getElfType()/ElfType enum was to pass around an ENUM form of ELFT from ExecuteElfObjcopyOnBinary to CreateWriter. There's no other use as far as I can tell. And it seems Jake really believes any use of ElfType is just "gross" so I decide to do away with the ElfType entirely. Aside from my own proposed changes the only function consuming ElfType is SplitDWOToFile, which passes it to CreateWriter, that then switches over the ElfType to call the templatized ELFWriter with an ELFT. This is getting kind of silly now.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678