[PATCH] D51841: [llvm-objcopy] Dwarf decompression support.

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked an inline comment as done.
plotfi added a comment.

In https://reviews.llvm.org/D51841#1234544, @jhenderson wrote:

> I may have missed this somewhere, but what happens if you specify --decompress/--compress-debug-sections on a system without zlib?


For decompress right now it probably hits an unreachable, looking at the current code. But for compress it hits an error message.

What you're supposed to do is call zlib::isAvailable() to see if zlib is available and if not just print an error. llvm/lib/Support/Compression.cpp has unreachable versions of all the other zlib library functions.



================
Comment at: tools/llvm-objcopy/Object.cpp:155
+  const char *Magic = "ZLIB";
+  return Section.OriginalData.size() > strlen(Magic) &&
+         !strncmp(reinterpret_cast<const char *>(Section.OriginalData.data()),
----------------
jhenderson wrote:
> No need to use `strlen` on a string literal. In general, use char arrays and sizeof. However, in this case, make `Magic` an ArrayRef, and use the size method and std::equals or similar to compare, rather than strncmp (assuming that ArrayRefs don't have a startswith method). It will read much better, and avoids unnecessary casting from uint8_t to const char.
I don't want to use ArrayRefs like that again. They blow up when you compile with clang-8 and the bots fail. I'd rather just use StringRef. 


================
Comment at: tools/llvm-objcopy/Object.cpp:173
+
+  SmallVector<char, 128> DecompressedContent;
+  if (Error E = zlib::uncompress(CompressedContent, DecompressedContent,
----------------
jhenderson wrote:
> This may not be your fault, but SmallVector is clearly the wrong thing for this here, since this won't be Small for the vast majority of uses.
the choices are:


```
Error uncompress(StringRef InputBuffer, char *UncompressedBuffer,
                 size_t &UncompressedSize);

Error uncompress(StringRef InputBuffer,
                 SmallVectorImpl<char> &UncompressedBuffer,
                 size_t UncompressedSize);

```


I dont want to use the one that passes UncompressedSize by reference. 


================
Comment at: tools/llvm-objcopy/Object.cpp:175
+  if (Error E = zlib::uncompress(CompressedContent, DecompressedContent,
+                                 (size_t)Sec.DecompressedSize))
+    reportError(Sec.Name, std::move(E));
----------------
jhenderson wrote:
> Is this explicit cast necessary? If it is (e.g. we support 32-bit hosts still), use static_cast.
Ah sorry about that. 


Repository:
  rL LLVM

https://reviews.llvm.org/D51841