This is an archive of the discontinued LLVM Phabricator instance.

[MC/ELF] - Fixed insufficient compression.s test
ClosedPublic

Authored by grimar on May 20 2016, 3:14 AM.

Details

Summary

Main problem that .debug_info
section was used to check that llvm-dwarfdump is able to decompress
data that was compressed with llvm-mc tool. This section was not compressed
actually, because consumes more space in compressed view.

I changed testcase to use .debug_str section which is one that
is really compressed. So currently test do what is probably was expected to do:
checks that "data"->llvm-mc->llvm-dwarfdump->dumps back initial "data".

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 57920.May 20 2016, 3:14 AM
grimar retitled this revision from to [MC/ELF] - Fixed incorrect compression.s test.
grimar updated this object.
grimar added reviewers: rafael, davide, dblaikie.
grimar added subscribers: llvm-commits, grimar.

This seems to be the reason of why D20331 testcase passed fine, when it looks should have
been broken because of llvm-dwarfdump I think does not support "zlib" style format yet.
So I plan to fix that tool (and looks some others one too), will investigate how that should be done properly and where.
I guess LLVMObject is the place to look.

grimar updated this object.May 20 2016, 3:31 AM
rafael edited edge metadata.May 20 2016, 5:25 AM
rafael added a subscriber: rafael.

This does two things:

  • Change dwarfdump to print a section that is actually compressed, that is good.
  • Change the CHECK patters to check the full content of the sections.

I am not sure why you are doing that.

Cheers,
Rafael

dblaikie added inline comments.May 20 2016, 10:04 AM
compression.s
14–17 ↗(On Diff #57920)

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

grimar added inline comments.May 21 2016, 1:16 AM
compression.s
14–17 ↗(On Diff #57920)

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.

David, so are you ok with commiting that or .. ?

grimar updated this revision to Diff 58229.May 24 2016, 6:33 AM
grimar edited edge metadata.
  • Addressed review comments. Patch only perform change to check comression roundtrip and nothing more (+few comments).
emaste added a subscriber: emaste.May 24 2016, 6:34 AM

LGTM, please commit.

This revision was automatically updated to reflect the committed changes.

LGTM, please commit.

Thanks !
r270560.

grimar retitled this revision from [MC/ELF] - Fixed incorrect compression.s test to [MC/ELF] - Fixed insufficient compression.s test.May 24 2016, 6:52 AM
grimar updated this object.