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

[Puyan Lotfi via Phabricator via llvm-commits]

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


================
Comment at: include/llvm/Object/Compressor.h:43
+
+/// Return the gnu style compressed section name.
+Expected<std::string> getDebugSectionName(const StringRef Name,
----------------
jhenderson wrote:
> This comment implies that the style will always be GNU, but the function takes an "IsGnuStyle" which doesn't make sense. Also, this function should be getDecompressedDebugSectionName, since it's for compressed sections only.
It works for either. If you have a gnu style name and specify IsGnuStyle = false it'll drop the z for debug. This is a little complicated so I will things into two separate functions. I've altered it to take a DebugCompressionType while also making the comments more clear. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:397
+        compress(Contents, Section.Align,
+                 (Config.CompressDebugSections == DebugCompressionType::GNU),
+                 OutputElfType);
----------------
jhenderson wrote:
> Rather than use Config.CompressDebugSections directly here, pass it in as a function parameter, i.e. the signature of this function should look something like the following:
> 
> ```tryCompressSections(const CopyConfig &Config, Object &Obj,
>     DebugCompressionType CompressionType, SectionPred &RemovePred,
>     ElfType OutputElfType)
> ```
> 
> Then propagate `CompressionType` into `compress` and do the comparison there.
I understand your point about passing CompressionType instead of IsGnuStyle but why add an additional parameter when this is already in the CopyConfig???


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:413
+    auto BufPtr = reinterpret_cast<const uint8_t *>(CompressedContents->data());
+    auto BufSize = CompressedContents->size();
+    Obj.addSection<CompressedSection>(
----------------
jhenderson wrote:
> Don't use auto here.
auto BufSize? This was copied from elsewhere in llvm-objcopy.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678