<p dir="ltr">OK, I think David has raised valid issues. Can you change this to just change dwarfdump to print a section that is actually compressed?</p>
<p dir="ltr">Cheers,<br>
Rafael</p>
<div class="gmail_quote">On May 23, 2016 11:34 AM, "David Blaikie" <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, May 21, 2016 at 1:16 AM, George Rimar via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">grimar added inline comments.<br>
<span><br>
================<br>
Comment at: compression.s:14-17<br>
@@ +13,6 @@<br>
</span><span>+// CHECK: Contents of section .zdebug_line:<br>
+// CHECK-NEXT: 0000 5a4c4942 00000000 00000041 789cb365 ZLIB.......Ax..e<br>
+// CHECK-NEXT: 0010 60606062 5000928c 8cbff978 81242323 ```bP......x.$##<br>
+// CHECK-NEXT: 0020 8803422e 459965a9 457a9999 0c60c0c9 ..B.E.e.Ez...`..<br>
+// CHECK-NEXT: 0030 c40005c2 8a60c0c4 08540e00 cbdf05d3 .....`...T......<br>
+// CHECK: Contents of section .debug_abbrev:<br>
</span>----------------<br>
<span>dblaikie wrote:<br>
> Testing the contents like this seems not good. Could you explain more why/how the original test was insufficient?<br>
><br>
> (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)))<br>
>> Testing the contents like this seems not good. Could you explain more why/how the original test was insufficient?<br>
<br>
</span>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.<br>
Since this test is the main our test about compression, I think we want to test all possible logic involved and as much<br>
possible problems as possible to imagine relative to compression.<br>
<br>
Initial test was insufficient generally IMO:<br>
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.<br>
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.<br>
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.<br></blockquote><div><br></div><div>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).<br><br>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.<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
>> (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)))<br>
<br>
</span>With above changes, full compression-decompression circle is tested atm.<br>
<div><div><br>
<br>
<a href="http://reviews.llvm.org/D20466" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20466</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>
</blockquote></div>