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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:156
+    uint64_t *DecompressedSizePtr = reinterpret_cast<uint64_t *>(Buf);
+    *DecompressedSizePtr = support::endian::read64be(&DecompressedSize);
+    Buf += sizeof(DecompressedSize);
----------------
Do we know that this is 64-bits even on 32-bit instances?


================
Comment at: tools/llvm-objcopy/Object.h:369
+public:
+  CompressedSection(const SectionBase &Sec,
+                    DebugCompressionType CompressionType)
----------------
The Size needs to be set in the constructor. Prior to all the fancy changes that people are making now. Most Sizes could be quite trivially determined. Size is used in layout. Because I didn't foresee Size needing to be determined dynamically for different systems, it's just a single variable when it really needs to be a virtual function that takes some magical argument that will allow it to know the proper size for a given output format. The issues here haven't been fully fleshed out but I think a solution that Alex and James proposed to solve other issues and not just this one)could be made to solve this. For the time being, the best option is to use an upper bound. It sucks; I dislike it, but I don't have a better recommendation.

I should finally get the time I need to refactor this in a couple weeks. I should be able to solve a lot of these overarching issues then.

In the meantime, I think this needs to compress the data within the section so that it can get a reasonable upper bound on the size.


================
Comment at: tools/llvm-objcopy/Object.h:377
+    Flags |= Sec.Flags;
+    if (isGnuStyle)
+      return;
----------------
Does it cause problems with other tools if we set SHF_COMPRESSED for GNU style? If not I think I'd like to set it regardless.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:294
 static bool isDebugSection(const SectionBase &Sec) {
-  return Sec.Name.startswith(".debug") || Sec.Name.startswith(".zdebug") ||
+  return !Sec.Name.find(".debug") || !Sec.Name.find(".zdebug") ||
          Sec.Name == ".gdb_index";
----------------
I think find works from anywhere. I'd prefer if the behavior was still as it is with startswith. I think `StringRef(Sec.Name).startswith(...)` would work or in the other patch `std::equal(S2.begin(), S2.end(), S1.begin());` was used with an auxiliary StartsWith function: https://reviews.llvm.org/D50463

either way I'd just prefer we keep the 'startswith' behavior 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:410
+static bool isCompressed(const SectionBase &Section) {
+  return !Section.Name.find(".zdebug") || (Section.Flags & ELF::SHF_COMPRESSED);
+}
----------------
Can we add a check for "ZLIB" at the start of OriginalData if ".zdebug" is set but SHF_COMPRESSED is not?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:416
+  std::vector<SectionBase *> SectionsToRemove;
+  std::for_each(Obj.sections().begin(), Obj.sections().end(),
+                [&SectionsToRemove](SectionBase &Section) {
----------------
Can this be a range based for loop?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:591
         return false;
-      if (Sec.Name.startswith(".gnu.warning"))
+      if (!Sec.Name.find(".gnu.warning"))
         return false;
----------------
ditto on starts with.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678