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

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

Still a few comments to address, but this is mostly okay now.



================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test:10
 # RUN: llvm-readobj -relocations -s %t-compressed.o | FileCheck %s --check-prefix=CHECK-FLAGS
+# RUN: llvm-objdump -s %t-decompressed.o -section=.debug_foo | FileCheck %s
 
----------------
plotfi wrote:
> jhenderson wrote:
> > This still isn't a good enough test. All it does is show that we have a section named .debug_foo after decompression. It shows nothing about the properties of the section header or that the section has been decompressed. You need to check both (e.g. by comparing the contents of the section/section header before and after decompression). If you bring across the comparisons in compress-debug-sections.test, then that at least will cover the contents aspect.
> Added checks for flags and content. Lemme know if these are sufficient. 
Thanks. One more suggestion: change "CHECK-FLAGS-DECOMPRESSED" to "CHECK-HEADER" and check the size field too. Then also add a check for this against the original object file too. That way, you'll be showing that the decompressed version is the same as the original version pre-compression.


================
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
----------------
jhenderson wrote:
> 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.
> 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.

This hasn't been addressed.


================
Comment at: tools/llvm-objcopy/Object.h:405
+        DecompressedAlign(Sec.DecompressedAlign) {
+    Size = DecompressedSize;
+    Flags = (Flags & ~ELF::SHF_COMPRESSED);
----------------
jhenderson wrote:
> 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?
> Sorry, I meant, what is the DecompressedAlign member variable used for. Can we delete it?
This hasn't been addressed yet either. Is the DecompressedSection::DecompressedAlign variable used for anything?

Similarly, why do we need a separate DecompressedSize member variable for this section?

What is wrong with just using the Size and Align variables available in the base class?


================
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) {
----------------
jhenderson wrote:
> 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.
This comment hasn't been addressed yet either.


================
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.");
+
----------------
plotfi wrote:
> jhenderson wrote:
> > "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.
> I could do that in an NFC commit after this one. 
If by that, you mean move the compress/zlib available check to the decompress check's location, that's fine.


Repository:
  rL LLVM

https://reviews.llvm.org/D51841