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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, if you make the suggested test changes.



================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test:4
+# RUN: yaml2obj %p/Inputs/compress-debug-sections.yaml -o %t
+# RUN: cp %t %t.o
 # RUN: llvm-objcopy --compress-debug-sections=zlib-gnu %t.o %t-compressed.o
----------------
Sorry, I guess I've confused you. You don't need to copy the input in this particular test, as long as one test (i.e. compress-debug-sections.test - see my inline comments) shows that llvm-objcopy does not change the input.

When you do `llvm-readobj -relocations -s %t` below, just do it on %t.o.


================
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:
> > plotfi wrote:
> > > jhenderson wrote:
> > > > 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.
> > > I dont understand what you mean? Do you mean the decompressed section is the same as original? 
> > No. I mean you should look at the examples in the testsuite, such as discard-all.test and do something similar to show that the input file (%t.o) is not modified, by copying it first and comparing it against the copy after you have run llvm-objcopy.
> I hope this is what you meant.
Nearly, I'll be more specific below.


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections.test:4
+# RUN: yaml2obj %p/Inputs/compress-debug-sections.yaml -o %t
+# RUN: cp %t %t.o
+
----------------
Rename %t to %t.o, and copy it to %t.copy.o. This is more self-documenting as to the purpose, I think.


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections.test:8
+# RUN: llvm-objcopy --compress-debug-sections=zlib-gnu %t.o %tzg.o
+
+# RUN: llvm-objcopy --decompress-debug-sections %tz.o  %t2.o
----------------
Add the following here:

```
# RUN: cp %tz.o %tz.copy.o
# RUN: cp %tzg.o %tzg.copy.o
```


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections.test:19
+# RUN: diff %t.txt %t3.txt
+
----------------
Add the following here:

```
# RUN: cmp %t.o %t.copy.o
# RUN: cmp %tz.o %tz.copy.o
# RUN: cmp %tzg.o %tzg.copy.o
```


Repository:
  rL LLVM

https://reviews.llvm.org/D51841