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

George Rimar via llvm-commits llvm-commits at lists.llvm.org
Sat May 21 01:16:33 PDT 2016


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.

>> (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





More information about the llvm-commits mailing list