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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 26 inline comments as done.
plotfi added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:171
+                                     DebugCompressionType CompressionType)
+    : Data(std::begin(Sec.OriginalData), std::end(Sec.OriginalData)),
+      CompressionType(CompressionType) {
----------------
jhenderson wrote:
> Can't you just set this->OriginalData to Sec.OriginalData?
> 
> That would allow you to get rid of the Data member entirely.
No I dont think so. Unless removal of the section doesn't free up that data. 


================
Comment at: tools/llvm-objcopy/Object.cpp:188
+  Offset = Sec.Offset;
+  Type = ELF::SHT_PROGBITS;
+
----------------
jhenderson wrote:
> This is extremely fragile: if we end up adding to the SectionBase list of members, this list will likely not be updated. Also, some of these fields may well be unset at the point of creation of this section (I'm thinking NameIndex specifically, but there may be others - I haven't bothered to check).
> 
> I think you might be able to get away with calling the base class's assignment method to set these fields, rather than manually copying each one.
Base class has no assignment method. It'll try and add one. Also base class has no constructor because it's abstract I believe. 


================
Comment at: tools/llvm-objcopy/Object.h:376
+public:
+  CompressedSection(const SectionBase &Sec,
+                    DebugCompressionType CompressionType)
----------------
alexshap wrote:
> since it's quite large - move it to .cpp + initialize as many fields as possible before the ctor body
I can't initialize these as far as I can see. The base class has no default constructor. 


================
Comment at: tools/llvm-objcopy/Object.h:17
 #include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/StringTableBuilder.h"
----------------
jhenderson wrote:
> You can get rid of this from the header by forward-declaring the DebugCompressionType enum.
You can not. Usage of DebugCompressionType::GNU etc makes this not possible unless you want to copy the enum class (which I would like not to).


================
Comment at: tools/llvm-objcopy/Object.h:20-22
+#include "llvm/Support/Compression.h"
+#include "llvm/Support/EndianStream.h"
+#include "llvm/Support/Errc.h"
----------------
jhenderson wrote:
> I don't think you need any of these headers.
Still needs Compression.h


================
Comment at: tools/llvm-objcopy/Object.h:377
+    Flags |= Sec.Flags;
+    if (isGnuStyle)
+      return;
----------------
jhenderson wrote:
> plotfi wrote:
> > jakehehrlich wrote:
> > > 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.
> > it might, but I dont remember off the top of my head. I will try out setting it and using gnu objcopy to see what happens. 
> I'm not sure we can make any assumptions for all tools written out there, so I'm reluctant to support having the flag set for GNU-style compressed sections. In particular, I could imagine a case where a tool checks for the flag first, and if it sees it, decompresses it in ZLIB style, or tries to and fails, because it is actually in GNU format. Only if it doesn't see the flag might it then try to decompress as GNU.
Jake wanted to always set it. I can not set it for the gnu case. However, gnu objcopy worked in gnu more. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:414
+
+static void appendCompressableSections(const CopyConfig &Config, Object &Obj,
+                                       SectionPred &RemovePred) {
----------------
jhenderson wrote:
> Is there a reason you changed the name of this function? I feel like it was fine to be called `compressSections`. `appendCompressableSections` suggests that it just adds some sections to the ELF, but in fact, it's also updating the removal predicate to remove the to-be-compressed sections.
I had changed it because in some intermediate patches there was no compression going on at this stage (it was happening in the visitor). 


Repository:
  rL LLVM

https://reviews.llvm.org/D49678