[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: lib/Object/Compressor.cpp:17
+namespace llvm {
+namespace object {
+
----------------
jhenderson wrote:
> 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!
Yeah, this is in the object library which is the same place Decompressor is. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:355
+  Expected<Decompressor> D = object::Decompressor::create(
+      Section.Name, Contents, (ELFT::TargetEndianness == endianness::little),
+      ELFT::Is64Bits);
----------------
jhenderson wrote:
> 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.
I think in this case, it's a little bit unreasonable to require this because the Decompressor library is not templated. I will go and refactor the Decompressor in another commit but the Decompressor library is used elsewhere in llvm and I do not want to change it in this commit but I also really would like to avoid duplicating what it does too. I want to implement support for decompression because it is handy for verifying that compression worked correctly. Can we have a compromise here, please? 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:369
+
+static bool isCompressed(const SectionBase &Section) {
+  return Section.Name.startswith(".zdebug") ||
----------------
jhenderson wrote:
> 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.
GNU style uses the name, this is not my design choice. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:385
+    if (isCompressed(Section) ||
+        !object::isCompressableSectionName(Section.Name))
+      continue;
----------------
jakehehrlich wrote:
> I think we already have an "is debug section" predicate floating around somewhere that uses the section name and might do another sanity check like making sure it isn't an allocated section. If there is not objectionable reason why this wouldn't work, I'd prefer to use that.
In objcopy or elsewhere in llvm? 


Repository:
  rL LLVM

https://reviews.llvm.org/D49678