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

[Puyan Lotfi via Phabricator via llvm-commits]

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


================
Comment at: lib/Object/Compressor.cpp:26
+
+StringRef getDebugSectionName(const StringRef Name, bool IsGnuStyle) {
+
----------------
jhenderson wrote:
> jakehehrlich wrote:
> > naming convention is to use capitals for non-methods. Same things below.
> I'm not sure I see anywhere using the incorrect style for LLVM?
Should getDebugSectionName be GetDebugSectionName. I forget the LLVM style. 


================
Comment at: lib/Object/Compressor.cpp:28-29
+
+  if (!Name.startswith(".debug") && !Name.startswith(".zdebug"))
+    return "";
+
----------------
jhenderson wrote:
> This feels like the wrong way to use the function for non-debug sections. Either I'd return an Optional, an Expected (if we want to treat it as an error in the caller) or just assert, or finally possibly just return the Name.
The idea is to just get the .zdebug or .debug name based on whatever name you provided depending on if you asked for it to be in gnu style. What is the problem here?


================
Comment at: tools/llvm-objcopy/Object.cpp:495-496
+
+  if (!doCompressionFinalize)
+    return;
+
----------------
jhenderson wrote:
> plotfi wrote:
> > jhenderson wrote:
> > > This bugs me. It maybe suggests to me that a compressed section should be a new section type.
> > Can you elaborate? Not sure I understand what you mean (may have lost some context, sorry, most recent update I did -U9999).
> This is similar to Jake's point about using OwnedDataSection. I don't want Section to need to be modifiable for all usages, so I think that a sub-class (maybe OwnedDataSection) should be used instead.
Now it is a new section type. I also extended CompressedSection to form a DecompressedSection that works almost the same way (but no compressed flag set). 


Repository:
  rL LLVM

https://reviews.llvm.org/D49678