Page MenuHomePhabricator

Fix ARM instruction emulation tests on big-endian systems
ClosedPublic

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

Details

Summary

Running the ARM instruction emulation test on a big-endian system
would fail, since the code doesn't respect endianness properly.

In EmulateInstructionARM::TestEmulation, code assumes that an
instruction opcode read in from the test file is in target byte
order, but it was in fact read in in host byte order.

More difficult to fix, the EmulationStateARM structure models
the overlapping sregs and dregs by a union in _sd_regs. This
only works correctly if the host is a little-endian system.
I've removed the union in favor of a simple array containing
the 32 sregs, and changed any code accessing dregs to explicitly
use the correct two sregs overlaying that dreg in the proper
target order.

Also, the EmulationStateARM::ReadPseudoMemory and WritePseudoMemory
track memory as a map of uint32_t values in host byte order, and
implement 64-bit memory accessing by splitting them up into two
uint32_t ones. However, callers expect memory contents to be
provided in the form of a byte array (in target byte order).
This means the uint32_t contents need to be byte-swapped on
BE systems, and when splitting up a 64-bit access into two 32-bit
ones, byte order has to be respected.

Diff Detail

Repository
rL LLVM

Event Timeline

uweigand updated this revision to Diff 53300.Apr 11 2016, 11:47 AM
uweigand retitled this revision from to Fix ARM instruction emulation tests on big-endian systems.
uweigand updated this object.
uweigand added a reviewer: clayborg.
uweigand added a subscriber: lldb-commits.
clayborg accepted this revision.Apr 11 2016, 1:59 PM
clayborg edited edge metadata.

Looks good as long as all tests pass on all other systems.

This revision is now accepted and ready to land.Apr 11 2016, 1:59 PM
omjavaid edited edge metadata.Apr 12 2016, 5:46 AM

Seems legit but One cosmetic comment inline.

Also have you tested this patch by running LLDB testsuite on arm in both little and big endian modes?

source/Plugins/Instruction/ARM/EmulationStateARM.cpp
157 ↗(On Diff #53300)

m_memory is a map with map type uint32? I think we will end up loosing data here if its larger than 4 bytes. if StoreToPseudoAddress isnt used elsewhere better change value from uint64 to uint32 ? Also size seems to be a redundant argument now.

tberghammer edited edge metadata.Apr 12 2016, 9:24 AM

Generally looks good with 2 minor comment inline. I also run the test suite on Android ARM (little endian) and everything looked fine

source/Plugins/Instruction/ARM/EmulationStateARM.cpp
127 ↗(On Diff #53300)

I think you need "idx - 16" here

157 ↗(On Diff #53300)

All caller of the function ensures size<=4 so I think we should change value to a uint32_t and remove size as it is not used anymore.

uweigand updated this revision to Diff 53500.Apr 12 2016, 5:22 PM
uweigand edited edge metadata.

Generally looks good with 2 minor comment inline. I also run the test suite on Android ARM (little endian) and everything looked fine

Thanks for the review and test!

source/Plugins/Instruction/ARM/EmulationStateARM.cpp
127 ↗(On Diff #53500)

Good catch! Now fixed.

157 ↗(On Diff #53500)

Right, that was my intention, just forgot to actually do it ... Now fixed.

tberghammer accepted this revision.Apr 13 2016, 3:01 AM
tberghammer edited edge metadata.

Looks good

omjavaid accepted this revision.Apr 13 2016, 5:25 AM
omjavaid edited edge metadata.

LGTM

This revision was automatically updated to reflect the committed changes.