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

Mon May 23 14:49:15 PDT 2016

OK, I think David has raised valid issues. Can you change this to just
change dwarfdump to print a section that is actually compressed?

Cheers,
Rafael
On May 23, 2016 11:34 AM, "David Blaikie" <dblaikie at gmail.com> wrote:

>
>
> 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/8fd0fc6c/attachment.html>