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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: lib/Object/Compressor.cpp:26
+
+StringRef getDebugSectionName(const StringRef Name, bool IsGnuStyle) {
+
----------------
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?


================
Comment at: lib/Object/Compressor.cpp:28-29
+
+  if (!Name.startswith(".debug") && !Name.startswith(".zdebug"))
+    return "";
+
----------------
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.


================
Comment at: lib/Object/Compressor.cpp:34
+
+  static std::map<std::string, std::tuple<std::string, std::string>> NameMap;
+  auto I = NameMap.find(LookupName);
----------------
jakehehrlich wrote:
> This is not a bad way to solve the problem of storage. In fact you'll see StringSaver used throughout LLVM in just this way. If this was an expensive computation I'd be all for it too. I'd like to hear other people's thoughts on this just because I'm a bit cautious of it. I've come to find the StringSaver pattern to be an anti-pattern and an indication that StringRef was being over used. Unclear if its the case here or not.
Personally, I'm against using static local variables for storage, because it means we can't easily unit-test the function (we'd have to provide some way to reset the state, which is non-trivial for such things). I know that we're not currently unit-testing anything in llvm-objcopy, but I still think we should be.

I don't have a good alternative solution though except to change ownership of section names, which could get expensive if we have a lot of sections.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:335
+                                                  StringRef Contents) {
+
+  auto ModifiedContents = make_unique<SmallVector<char, 128>>();
----------------
Remove this blank line. Please don't put extra lines at the start of functions. This is not the style of LLVM.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:371
+compress(StringRef &Name, StringRef Contents, uint64_t Align, bool IsGnuStyle) {
+
+  auto ModifiedContents = make_unique<SmallVector<char, 128>>();
----------------
Remove this blank line.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:642
+  if ((Config.CompressDebugSections != DebugCompressionType::None) &&
+      !Config.DecompressDebugSections) {
+
----------------
I'm not sure you need the last part of this clause, since you've already emitted an error if attempting to do both. If you're bothered about it, make it an assert.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:643
+      !Config.DecompressDebugSections) {
+
+    bool IsGnuStyle =
----------------
Remove this blank line.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:644-645
+
+    bool IsGnuStyle =
+        (Config.CompressDebugSections == DebugCompressionType::GNU);
+
----------------
When I asked earlier about passing in the style directly, I meant why not pass in `Config.CompressDebugSections` directly? It's more extensible (i.e. we could easily add more compression styles in the future), more descriptive at the use site, and less prone to errors by forgetting to invert a `bool` or whatever.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:727-728
 
-static void ExecuteElfObjcopyOnArchive(const CopyConfig &Config, const Archive &Ar) {
+static void ExecuteElfObjcopyOnArchive(const CopyConfig &Config,
+                                       const Archive &Ar) {
   std::vector<NewArchiveMember> NewArchiveMembers;
----------------
Do this in a separate NFC commit.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:817
+          .Case("zlib", DebugCompressionType::Z)
+          .Default(DebugCompressionType::None);
+
----------------
You should emit an error if using an unknown decompression style.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678