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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm not seeing anything that checks that the requested compression style is a supported style. I'd expect an error to be generated if a nonsensical version is specified.

On testing this. We now have no tests at all...? That's wrong.



================
Comment at: tools/llvm-objcopy/Object.cpp:495-496
+
+  if (!doCompressionFinalize)
+    return;
+
----------------
plotfi wrote:
> jhenderson wrote:
> > This bugs me. It maybe suggests to me that a compressed section should be a new section type.
> Can you elaborate? Not sure I understand what you mean (may have lost some context, sorry, most recent update I did -U9999).
This is similar to Jake's point about using OwnedDataSection. I don't want Section to need to be modifiable for all usages, so I think that a sub-class (maybe OwnedDataSection) should be used instead.


================
Comment at: tools/llvm-objcopy/Object.cpp:1311
 
+  bool doDecompress = DecompressDebugSections;
+  bool IsGnuStyle = (CompressDebugSections == DebugCompressionType::GNU);
----------------
Incorrect case here and below -> DoDecompress. But why not just use DecompressDebugSections directly?


================
Comment at: tools/llvm-objcopy/Object.cpp:1320
+    for (auto &Section : Obj.sections()) {
+
+      bool isCompressable = object::isCompressable(Section.Name);
----------------
Remove blank line here.


================
Comment at: tools/llvm-objcopy/Object.h:397
   void finalize() override;
+  virtual bool isCompressable() const override {
+    StringRef GnuZDebugName =
----------------
plotfi wrote:
> plotfi wrote:
> > jhenderson wrote:
> > > alexshap wrote:
> > > > here and below - i think the implementations should be moved to Object.cpp
> > > Also, don't mark functions with both `virtual` and `override`. The `override` implies `virtual`, so only use the latter (similar to elsewhere in this file).
> > Yes.
> Thanks @jhenderson, I did not know that. 
Just for added clarity:

`virtual` only has any semantic meaning on the highest version of the method in a class hierarchy. Overriding versions of methods in derived classes automatically inherit `virtual`. However, prior to C++11 it was common to explicitly mark these derived implementations with `virtual` themselves, for clarity. With C++11, `override` came into existence, and ensures that we are genuinely overriding a base-class virtual method, and as such it means we don't need the `virtual` tag for clarity any more.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:598
   Config.KeepFileSymbols = InputArgs.hasArg(OBJCOPY_keep_file_symbols);
+  Config.DecompressDebugSections = InputArgs.hasArg(OBJCOPY_decompress_debug_sections);
   for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbol))
----------------
clang-format


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:612
+      Config.CompressDebugSections != DebugCompressionType::None) {
+    error("Can not specify --compress-debug-sections as well as "
+          "--decompress-debug-sections at the same time.");
----------------
Can not -> Cannot


Repository:
  rL LLVM

https://reviews.llvm.org/D49678