Page MenuHomePhabricator

Fix test cases for big-endian systems
ClosedPublic

Authored by uweigand on Apr 11 2016, 11:48 AM.

Details

Summary

A number of test cases were failing on big-endian systems simply due to
byte order assumptions in the tests themselves, and no underlying bug
in LLDB.

These two test cases:

tools/lldb-server/lldbgdbserverutils.py
python_api/process/TestProcessAPI.py

actually check for big-endian target byte order, but contain Python errors
in the corresponding code paths.

These test cases:

functionalities/data-formatter/data-formatter-python-synth/TestDataFormatterPythonSynth.py
functionalities/data-formatter/data-formatter-smart-array/TestDataFormatterSmartArray.py
functionalities/data-formatter/synthcapping/TestSyntheticCapping.py
lang/cpp/frame-var-anon-unions/TestFrameVariableAnonymousUnions.py
python_api/sbdata/TestSBData.py  (first change)

could be fixed to check for big-endian target byte order and update the
expected result strings accordingly. For the two synthetic tests, I've
also updated the source to make sure the fake_a value is always nonzero
on both big- and little-endian platforms.

These test case:

python_api/sbdata/TestSBData.py  (second change)
functionalities/memory/cache/TestMemoryCache.py

simply accessed memory with the wrong size, which wasn't noticed on LE
but fails on BE.

Event Timeline

uweigand updated this revision to Diff 53301.Apr 11 2016, 11:48 AM
uweigand retitled this revision from to Fix test cases for big-endian systems.
uweigand updated this object.
uweigand added reviewers: granata.enrico, clayborg.
uweigand added a subscriber: lldb-commits.
clayborg requested changes to this revision.Apr 11 2016, 2:08 PM
clayborg edited edge metadata.

So many tests above are going to accept either a little endian or big endian value. This will make most of these tests useless since if a little endian machine fails with a big endian number we won't catch it and vice versa. So we need to expect the correct value for little endian and a different but correct one for big endian tests and only accept the correct one.

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-python-synth/TestDataFormatterPythonSynth.py
83–87 ↗(On Diff #53501)

Zero seems like a bad alternate value as it could cover up a real failure. Can we improve this test so that we are testing for actual values? Or can we change the test by endianness and test for 16777216 for little endian and something else that is not zero for big endian? We don't want zero to be valid for little endian tests.

94 ↗(On Diff #53501)

ditto

98 ↗(On Diff #53501)

ditto

103 ↗(On Diff #53501)

ditto

106 ↗(On Diff #53501)

ditto

121 ↗(On Diff #53501)

We should expect one thing for little endian and one for big, don't accept either.

136 ↗(On Diff #53501)

We should expect one thing for little endian and one for big, don't accept either.

145 ↗(On Diff #53501)

We should expect one thing for little endian and one for big, don't accept either.

160 ↗(On Diff #53501)

We should expect one thing for little endian and one for big, don't accept either.

172 ↗(On Diff #53501)

We should expect one thing for little endian and one for big, don't accept either.

197 ↗(On Diff #53501)

We should expect one thing for little endian and one for big, don't accept either.

packages/Python/lldbsuite/test/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py
66 ↗(On Diff #53501)

We should expect one thing for little endian and one for big, don't accept either.

75 ↗(On Diff #53501)

We should expect one thing for little endian and one for big, don't accept either.

packages/Python/lldbsuite/test/lang/cpp/frame-var-anon-unions/TestFrameVariableAnonymousUnions.py
22–28 ↗(On Diff #53501)

We should expect one thing for little endian and one for big, don't accept either.

This revision now requires changes to proceed.Apr 11 2016, 2:08 PM

So many tests above are going to accept either a little endian or big endian value. This will make most of these tests useless since if a little endian machine fails with a big endian number we won't catch it and vice versa. So we need to expect the correct value for little endian and a different but correct one for big endian tests and only accept the correct one.

I fully agree this would be preferable. Unfortunately I didn't see a straightforward way to detect in those test cases whether the target platform is big- or little-endian. In the Python API tests this is simple, but these tests here just operate on the console interface. Is there a good way to detect target byte order in such tests?

Zero seems like a bad alternate value as it could cover up a real failure. Can we improve this test so that we are testing for actual values? Or can we change the test by endianness and test for 16777216 for little endian and something else that is not zero for big endian? We don't want zero to be valid for little endian tests.

The test case is a bit weird in how it (apparently deliberately) overlays mis-aligned synthetic fields across an integer array. This is always going to behave quite differently depending on byte order. I'll see if I can change the test so that values will be nonzero both on big- and on little-endian platforms.

labath added a subscriber: labath.Apr 12 2016, 3:11 AM

So many tests above are going to accept either a little endian or big endian value. This will make most of these tests useless since if a little endian machine fails with a big endian number we won't catch it and vice versa. So we need to expect the correct value for little endian and a different but correct one for big endian tests and only accept the correct one.

I fully agree this would be preferable. Unfortunately I didn't see a straightforward way to detect in those test cases whether the target platform is big- or little-endian. In the Python API tests this is simple, but these tests here just operate on the console interface. Is there a good way to detect target byte order in such tests?

Just because the tests test the console interface, it doesn't mean you can't use the python interface to do auxiliary tasks, like computing the expectations. It sounds like you should be able to to a self.target.GetByteOrder() to get the info you need. (use self.dbg.GetSelectedTarget() if self.target happens to be empty, i don't remember if it's always set).

Just because the tests test the console interface, it doesn't mean you can't use the python interface to do auxiliary tasks, like computing the expectations. It sounds like you should be able to to a self.target.GetByteOrder() to get the info you need. (use self.dbg.GetSelectedTarget() if self.target happens to be empty, i don't remember if it's always set).

Indeed, that seems to work fine. Thanks for the tip! I'll update the tests accordingly.

uweigand updated this revision to Diff 53501.Apr 12 2016, 5:28 PM
uweigand updated this object.
uweigand edited edge metadata.

So many tests above are going to accept either a little endian or big endian value. This will make most of these tests useless since if a little endian machine fails with a big endian number we won't catch it and vice versa. So we need to expect the correct value for little endian and a different but correct one for big endian tests and only accept the correct one.

The new version should address this. Each test now only accepts either the little-endian or big-endian result, depending on the target byte order.

Zero seems like a bad alternate value as it could cover up a real failure. Can we improve this test so that we are testing for actual values? Or can we change the test by endianness and test for 16777216 for little endian and something else that is not zero for big endian? We don't want zero to be valid for little endian tests.

I've changed the test source value to use values such that none of the data elements tested by the test case is ever zero, neither on little-endian nor on big-endian.

Tested on both System z and Intel to verify it still works with either byte order.

clayborg accepted this revision.Apr 13 2016, 9:59 AM
clayborg edited edge metadata.

Looks good, thanks for updating the changes.

This revision is now accepted and ready to land.Apr 13 2016, 9:59 AM
This revision was automatically updated to reflect the committed changes.