[PATCH] D20466: [MC/ELF] - Fixed incorrect compression.s test

[David Blaikie via llvm-commits]

On Sat, May 21, 2016 at 1:16 AM, George Rimar via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> grimar added inline comments.
>
> ================
> Comment at: compression.s:14-17
> @@ +13,6 @@
> +// CHECK:      Contents of section .zdebug_line:
> +// CHECK-NEXT:  0000 5a4c4942 00000000 00000041 789cb365  ZLIB.......Ax..e
> +// CHECK-NEXT:  0010 60606062 5000928c 8cbff978 81242323  ```bP......x.$##
> +// CHECK-NEXT:  0020 8803422e 459965a9 457a9999 0c60c0c9  ..B.E.e.Ez...`..
> +// CHECK-NEXT:  0030 c40005c2 8a60c0c4 08540e00 cbdf05d3  .....`...T......
> +// CHECK:      Contents of section .debug_abbrev:
> ----------------
> dblaikie wrote:
> > Testing the contents like this seems not good. Could you explain more
> why/how the original test was insufficient?
> >
> > (I see the roundtrip example was insufficient, since debug_info wasn't
> compressed - so for sure we should switch that to a section that is
> compressed, like _str, which is good (we should make sure we test that same
> section for compression - so maybe switch the zdebug_line to test
> zdebug_str (or change the roundtrip test to test zdebug_line rather than
> info (rather than changing it to str)))
> >> Testing the contents like this seems not good. Could you explain more
> why/how the original test was insufficient?
>
> Generally I would agree that too much content checking is not a good way,
> but for some tests it is very preferrable I believe. Like for this one.
> Since this test is the main our test about compression, I think we want to
> test all possible logic involved and as much
> possible problems as possible to imagine relative to compression.
>
> Initial test was insufficient generally IMO:
> 1) For zlib-gnu style it should test that 'ZLIB' magic is present and was
> put at the start, that was not done actually, it just checked that 'ZLIB'
> was somewhere there, but in case if 'ZLIB' and for example next 4 bytes
> were swapped, it would not catch that.
> 2) Next thing it should check that big-endian size of uncompressed data
> goes right after that. Probably there is no really good way to check that,
> but my way defenitely do that.
> 3) I also can imaging case if some new code can corrupt the compressed
> data, or new zlib library we using can be buggy or something, I guess we
> would like to notice such little chanced but still possible problems.
>

This is the bit that I'm least in favor of - I don't want us in the
business of testing zlib (or any other dependent library, really). I'd like
to test that we can roundtrip through zlib, and that it does result in
compression (given the limitations of our testing infrastructure, I'm OK
testing for a specific size rather than a more general "is the size smaller
when we enable compression than when we don't" just to sanity check).

I'd prefer to keep something similar to the original - just checking for
the presence or absence of ZLIB in the sections we're interested in. Should
just be two sections - one compressed and one not compressed (plus
debug_frame to demonstrate that even when it's worthwhile, we don't
compress it because of relaxation, etc). Demonstrating that the size is
smaller (which would involve just testing the size specifically since we
have no way to do a relative comparison with our testing tools) would be a
OK improvement, I think.

- Dave


>
> >> (we should make sure we test that same section for compression - so
> maybe switch the zdebug_line to test zdebug_str (or change the roundtrip
> test to test zdebug_line rather than info (rather than changing it to str)))
>
> With above changes, full compression-decompression circle is tested atm.
>
>
> http://reviews.llvm.org/D20466
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160523/5a16b679/attachment.html>