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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/Object/Compressor.h:34
+  /// @param W Destination buffer stream for compressed data.
+  Error writeCompressedSectionData(support::endian::Writer &W) {
+    SmallVector<char, 128> CompressedBuffer;
----------------
plotfi wrote:
> jhenderson wrote:
> > Don't abbreviate, if you can avoid it: W -> Writer, E ->Err, here and elsewhere in this file.
> > 
> > This function could probably be moved into the .cpp.
> Why? There's just so much LLVM code that uses this convention. Literally every single place I see support::endian::Writer the Writer is called W (or LE or OSE). For Error I see E and Err use pretty interchangeably in the codebase. 
Okay, I misrememberd this line from the coding standard: 

> Avoid abbreviations unless they are well known

If these are widely used, that's fine.




================
Comment at: include/llvm/Object/Compressor.h:62
+
+template <typename T> void writeReserved(support::endian::Writer &W) {
+  if (T::Is64Bits)
----------------
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.


================
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>>
----------------
I'm not sure you need all these additional extern declarations.


================
Comment at: lib/Object/Compressor.cpp:37
+  }
+  std::string Prefix =
+      (CompressionType == DebugCompressionType::GNU) ? ".z" : ".";
----------------
Prefix can be a StringRef.


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-decompress-checkfail.test:4-5
+# Reuse compress-debug-sections.test.
+# This is expected to fail, we can't specify both  --compress-debug-sections and
+# --decompress-debug-sections.
+
----------------
You don't need this comment.


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-decompress-checkfail.test:8
+# RUN: yaml2obj %S/compress-debug-sections.test  > %t-clean.o
+# RUN: not llvm-objcopy --compress-debug-sections=zlib --decompress-debug-sections %t-clean.o
+
----------------
You've missed the FileCheck command here.


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections.test:1
+# REQUIRES: shell
+# REQUIRES: zlib
----------------
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.


================
Comment at: tools/llvm-objcopy/Object.h:271-272
   virtual void markSymbols();
+  virtual ArrayRef<uint8_t> getOriginalContents() const;
+  bool isCompressed() const;
 };
----------------
plotfi wrote:
> jakehehrlich wrote:
> > I don't think there's any reason for these to exist. You can make an isCompressed function externally if you'd like but it shouldn't be a method.
> It _was_ external, I made it a method based on Jame's previous review...
If Jake prefers something, go with his preference.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:397
+        compress(Contents, Section.Align,
+                 (Config.CompressDebugSections == DebugCompressionType::GNU),
+                 OutputElfType);
----------------
plotfi wrote:
> jhenderson wrote:
> > Rather than use Config.CompressDebugSections directly here, pass it in as a function parameter, i.e. the signature of this function should look something like the following:
> > 
> > ```tryCompressSections(const CopyConfig &Config, Object &Obj,
> >     DebugCompressionType CompressionType, SectionPred &RemovePred,
> >     ElfType OutputElfType)
> > ```
> > 
> > Then propagate `CompressionType` into `compress` and do the comparison there.
> I understand your point about passing CompressionType instead of IsGnuStyle but why add an additional parameter when this is already in the CopyConfig???
My bad. I didn't notice CopyConfig being passed in.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:829
+      InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections) != "") {
+    error("Invaid or unsupported --compress-debug-sections format: " +
+          InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections));
----------------
jhenderson wrote:
> Invaid -> Invalid.
> 
> Please add a test for this error.
Where is the test for this error?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:349-350
+decompress(const SectionBase &Section, const CopyConfig &Config) {
+  bool Is64Bit = T::Is64Bits;
+  bool IsLittle = (T::TargetEndianness == endianness::little);
+  StringRef Contents(
----------------
Do these inline rather than stored in local variables, as they're only used once and are clear as to their meaning.


================
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) ||
----------------
I'm not sure I like this change, but it's probably best to let Jake comment on it before you make further changes.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:727
+    return;
+  llvm_unreachable("Binary has Invalid ELFT");
 }
----------------
Invalid -> invalid


Repository:
  rL LLVM

https://reviews.llvm.org/D49678