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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

You've forgotten to include context in the latest diff. I can't review some aspects of this change without it.



================
Comment at: test/tools/llvm-objcopy/compress-debug-sections.test:5
+
+# RUN: llvm-objcopy --compress-debug-sections=zlib     %t.o %tz.o
+# RUN: llvm-objcopy --compress-debug-sections=zlib-gnu %t.o %tzg.o
----------------
plotfi wrote:
> jhenderson wrote:
> > As with other tests, this test (or possibly the specific compress and decompress tests) should show that the inputs are not modified when using the specified switch.
> I don't understand what you mean here. Could you please explain?
If you look at a lot of our other feature tests, we make a copy of the input before running llvm-objcopy and then compare %t.o against that copy, to show that llvm-objcopy didn't do an in-place modification of the file.


================
Comment at: tools/llvm-objcopy/Object.cpp:139
 
+static const std::vector<uint8_t> ZlibGnuMagic = {'Z', 'L', 'I', 'B'};
+
----------------
This is okay, but you can actually do the following, which is probably even simpler:
```
static const uint8_t ZlibGnuMagic[4] = {'Z', 'L', 'I', 'B'};
```
You can then use sizeof to get the size, and std::begin/std::end for the start and end of the array.




================
Comment at: tools/llvm-objcopy/Object.cpp:172
+
+  const unsigned DataOffset =
+      isDataGnuCompressed(Sec.OriginalData)
----------------
You're still using unsigned. Use size_t or uint64_t not unsigned. This will fail for very large ELF files.


================
Comment at: tools/llvm-objcopy/Object.h:405
+        DecompressedAlign(Sec.DecompressedAlign) {
+    Size = DecompressedSize;
+    Flags = (Flags & ~ELF::SHF_COMPRESSED);
----------------
plotfi wrote:
> jhenderson wrote:
> > plotfi wrote:
> > > Oops, I think I need to add Align = DecompressedAlign here. What do you think @jakehehrlich 
> > What is DecompressedAlign actually used for? You should certainly be setting Align.
> I'm not actually completely sure. But it's in the ELF Chdr so I believe it's just encoding the alignment of the section from what be was before being compressed. 
Sorry, I meant, what is the DecompressedAlign member variable used for. Can we delete it?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:1019-1020
+      Config.CompressionType != DebugCompressionType::None) {
+    error("Cannot specify --compress-debug-sections as well as "
+          "--decompress-debug-sections at the same time");
+  }
----------------
jhenderson wrote:
> "... as well as --decompress-debug-sections at the same time" -> "... at the same time as --decompress-debug-sectons"
You can remove the last part of the error message after "--decompress-debug-sections"


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:436
+    const CopyConfig &Config, Object &Obj, SectionPred &RemovePred,
+    std::function<bool(const SectionBase &)> shouldReplace,
+    std::function<SectionBase *(const SectionBase *)> addSection) {
----------------
You should probably use function_ref instead of std::function, if I'm not mistaken. Also, I //think// that shouldReplace and addSection should be upper-case (i.e. ShouldReplace etc), since they are technically variables here.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:1023
+  if (Config.DecompressDebugSections && !zlib::isAvailable())
+    error("LLVM was not compiled with LLVM_ENABLE_ZLIB: can not decompress.");
+
----------------
"can not" -> cannot

It feels to me like it would make sense to check both decompress and compress here, rather than in two completely different places, since they are closely related. I'd rather not rely on the library providing error messages, when we can error early and up-front.


Repository:
  rL LLVM

https://reviews.llvm.org/D51841