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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

@jakehehrlich, what are your thoughts on @alexshap's comments re. compress only when it makes things smaller?



================
Comment at: lib/Object/Compressor.cpp:17
+namespace llvm {
+namespace object {
+
----------------
jakehehrlich wrote:
> This probably shouldn't be defined in object since that's a library namespace, maybe objcopy instead?
I nearly made the same mistake myself, but this is actually IN the Object library, so is in the correct namespace!


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:355
+  Expected<Decompressor> D = object::Decompressor::create(
+      Section.Name, Contents, (ELFT::TargetEndianness == endianness::little),
+      ELFT::Is64Bits);
----------------
jakehehrlich wrote:
> Again, you don't need to pass around this sort of data. I will not accept a change that unnecessarily keeps track of endianness and bit width using data when an easy ELFT solution exists. I hate ElfType because it forces you to branch on code you otherwise wouldn't have to and errors can occur where those difference branches have different code. 
My preference would be to refactor or extend the already-existing libObject Decompressor interface to take an ELFT as a template argument. That would allow the complexity to be pushed deeper into the library, and remove some of these issues.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:369
+
+static bool isCompressed(const SectionBase &Section) {
+  return Section.Name.startswith(".zdebug") ||
----------------
jakehehrlich wrote:
> This should probably not use the name as indication of compression. Just use the flag.
I don't think we have a choice - it appears that GNU-style compression uses the name only as an indicator of compression. I could be persuaded that we should add the SHF_COMPRESSED flag in either case, since relying on section names is wrong and archaic, when we have other mechanisms to communicate the same thing, but we will at least have to be able to handle the case where a compressed section, compressed via GNU's tools, is fed to us.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:378
+  std::vector<SectionBase *> Sections;
+  std::for_each(
+      Obj.sections().begin(), Obj.sections().end(),
----------------
jakehehrlich wrote:
> Based on James' comment I might be misunderstanding something here. I think you can just loop over Obj.sections() in a range based for loop. You really don't want to do unnecessary loops over sections.
Yes, you are missing that we modify Obj.sections() within the loop below, and we can't iterate over a vector and add to it in the same loop.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:416
+    // Replace this Section with a compressed version.
+    RemovePred = [RemovePred, &Section](const SectionBase &Sec) {
+      return &Sec == &Section || RemovePred(Sec);
----------------
jakehehrlich wrote:
> RemovePred should not be modified in a loop like this. It's going to make RemovePred even more inefficent that it already is.
I suggested this originally, because I didn't see an obvious alternative, unless we completely drop the idea of "compress only if compression makes it smaller".


Repository:
  rL LLVM

https://reviews.llvm.org/D49678