Fix ARM instruction emulation tests on big-endian systems

[uweigand]

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.

[clayborg]

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

[omjavaid]

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]

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]

In D18984#398486, @tberghammer wrote:

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]

Looks good

[omjavaid]

LGTM

lldb/trunk/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp

EmulateInstructionARM::TestEmulation (Stream *out_stream, ArchSpec &arch, OptionValueDictionary *test_data)
     }
     test_opcode = value_sp->GetUInt64Value ();
 
 
     if (arch.GetTriple().getArch() == llvm::Triple::thumb || arch.IsAlwaysThumbInstructions ())
     {
         m_opcode_mode = eModeThumb;
         if (test_opcode < 0x10000)
-            m_opcode.SetOpcode16 (test_opcode, GetByteOrder());
+            m_opcode.SetOpcode16 (test_opcode, endian::InlHostByteOrder());
         else
-            m_opcode.SetOpcode32 (test_opcode, GetByteOrder());
+            m_opcode.SetOpcode32 (test_opcode, endian::InlHostByteOrder());
     }
     else if (arch.GetTriple().getArch() == llvm::Triple::arm)
     {
         m_opcode_mode = eModeARM;
-        m_opcode.SetOpcode32 (test_opcode, GetByteOrder());
+        m_opcode.SetOpcode32 (test_opcode, endian::InlHostByteOrder());
     }
     else
     {
         out_stream->Printf ("TestEmulation:  Invalid arch.\n");
         return false;
     }
 
     EmulationStateARM before_state;

lldb/trunk/source/Plugins/Instruction/ARM/EmulationStateARM.h

public:
     
     bool
     StorePseudoRegisterValue (uint32_t reg_num, uint64_t value);
     
     uint64_t
     ReadPseudoRegisterValue (uint32_t reg_num, bool &success);
     
     bool
-    StoreToPseudoAddress (lldb::addr_t p_address, uint64_t value, uint32_t size);
+    StoreToPseudoAddress (lldb::addr_t p_address, uint32_t value);
     
     uint32_t
-    ReadFromPseudoAddress (lldb::addr_t p_address, uint32_t size, bool &success);
+    ReadFromPseudoAddress (lldb::addr_t p_address, bool &success);
     
     void
     ClearPseudoRegisters ();
     
     void
     ClearPseudoMemory ();
     
     bool
WritePseudoRegister (lldb_private::EmulateInstruction *instruction,
                          void *baton,
                          const lldb_private::EmulateInstruction::Context &context,
                          const lldb_private::RegisterInfo *reg_info,
                          const lldb_private::RegisterValue &reg_value);
 private:
     uint32_t m_gpr[17];
     struct _sd_regs
     {
-        union 
-        {
-            uint32_t s_reg[2];
-            uint64_t d_reg;
-        } sd_regs[16];  // sregs 0 - 31 & dregs 0 - 15
+        uint32_t s_regs[32]; // sregs 0 - 31 & dregs 0 - 15
         
         uint64_t d_regs[16]; // dregs 16-31
  
     } m_vfp_regs;
     
     std::map<lldb::addr_t, uint32_t> m_memory; // Eventually will want to change uint32_t to a data buffer heap type.
     
     DISALLOW_COPY_AND_ASSIGN (EmulationStateARM);
 };
 
 #endif  // lldb_EmulationStateARM_h_

lldb/trunk/source/Plugins/Instruction/ARM/EmulationStateARM.cpp

EmulationStateARM::LoadPseudoRegistersFromFrame (StackFrame &frame)
     for (int i = dwarf_d0; i < dwarf_d0 + 32; ++i)
     {
         reg_num = reg_ctx->ConvertRegisterKindToRegisterNumber (eRegisterKindDWARF, i);
         RegisterValue reg_value;
         const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoAtIndex (reg_num);
 
         if (reg_ctx->ReadRegister (reg_info, reg_value))
         {
+            uint64_t value = reg_value.GetAsUInt64();
             uint32_t idx = i - dwarf_d0;
             if (i < 16)
-                m_vfp_regs.sd_regs[idx].d_reg = reg_value.GetAsUInt64();
+            {
+                m_vfp_regs.s_regs[idx * 2] = (uint32_t)value;
+                m_vfp_regs.s_regs[idx * 2 + 1] = (uint32_t)(value >> 32);
+            }
             else
-                m_vfp_regs.d_regs[idx - 16] = reg_value.GetAsUInt64();
+                m_vfp_regs.d_regs[idx - 16] = value;
         }
         else
             success = false;
     }
     
     return success;
 }
     
 bool
 EmulationStateARM::StorePseudoRegisterValue (uint32_t reg_num, uint64_t value)
 {
     if ((dwarf_r0 <= reg_num) && (reg_num <= dwarf_cpsr))
         m_gpr[reg_num  - dwarf_r0] = (uint32_t) value;
     else if ((dwarf_s0 <= reg_num) && (reg_num <= dwarf_s31))
     {
         uint32_t idx = reg_num - dwarf_s0;
-        m_vfp_regs.sd_regs[idx / 2].s_reg[idx % 2] = (uint32_t) value;
+        m_vfp_regs.s_regs[idx] = (uint32_t)value;
     }
     else if ((dwarf_d0 <= reg_num) && (reg_num <= dwarf_d31))
     {
-        if ((reg_num - dwarf_d0) < 16)
+        uint32_t idx = reg_num - dwarf_d0;
+        if (idx < 16)
         {
-            m_vfp_regs.sd_regs[reg_num - dwarf_d0].d_reg = value;
+            m_vfp_regs.s_regs[idx * 2] = (uint32_t)value;
+            m_vfp_regs.s_regs[idx * 2 + 1] = (uint32_t)(value >> 32);
         }
         else
-            m_vfp_regs.d_regs[reg_num - dwarf_d16] = value;
+            m_vfp_regs.d_regs[idx - 16] = value;
     }
     else
         return false;
         
     return true;
 }
     
 uint64_t
 EmulationStateARM::ReadPseudoRegisterValue (uint32_t reg_num, bool &success)
 {
     uint64_t value = 0;
     success = true;
     
     if ((dwarf_r0 <= reg_num) && (reg_num <= dwarf_cpsr))
         value = m_gpr[reg_num  - dwarf_r0];
     else if ((dwarf_s0 <= reg_num) && (reg_num <= dwarf_s31))
     {
         uint32_t idx = reg_num - dwarf_s0;
-        value = m_vfp_regs.sd_regs[idx / 2].s_reg[idx % 2];
+        value = m_vfp_regs.d_regs[idx];
     }
     else if ((dwarf_d0 <= reg_num) && (reg_num <= dwarf_d31))
     {
-        if ((reg_num - dwarf_d0) < 16)
-            value = m_vfp_regs.sd_regs[reg_num - dwarf_d0].d_reg;
+        uint32_t idx = reg_num - dwarf_d0;
+        if (idx < 16)
+            value = (uint64_t)m_vfp_regs.s_regs[idx * 2] | ((uint64_t)m_vfp_regs.s_regs[idx * 2 + 1] >> 32);
         else
-            value = m_vfp_regs.d_regs[reg_num - dwarf_d16];
+            value = m_vfp_regs.d_regs[idx - 16];
     }
     else
         success = false;
         
     return value;
 }
     
 void
 EmulationStateARM::ClearPseudoRegisters ()
 {
     for (int i = 0; i < 17; ++i)
         m_gpr[i] = 0;
     
-    for (int i = 0; i < 16; ++i)
-        m_vfp_regs.sd_regs[i].d_reg = 0;
+    for (int i = 0; i < 32; ++i)
+        m_vfp_regs.s_regs[i] = 0;
     
     for (int i = 0; i < 16; ++i)
         m_vfp_regs.d_regs[i] = 0;
 }
 
 void
 EmulationStateARM::ClearPseudoMemory ()
 {
     m_memory.clear();
 }
     
 bool
-EmulationStateARM::StoreToPseudoAddress (lldb::addr_t p_address, uint64_t value, uint32_t size)
+EmulationStateARM::StoreToPseudoAddress (lldb::addr_t p_address, uint32_t value)
 {
-    if (size > 8)
-        return false;
-    
-    if (size <= 4)
-        m_memory[p_address] = value;
-    else if (size == 8)
-    {
-        m_memory[p_address] = (value << 32) >> 32;
-        m_memory[p_address + 4] = value << 32;
-    }
     return true;
 }
     
 uint32_t
-EmulationStateARM::ReadFromPseudoAddress (lldb::addr_t p_address, uint32_t size, bool &success)
+EmulationStateARM::ReadFromPseudoAddress (lldb::addr_t p_address, bool &success)
 {
     std::map<lldb::addr_t,uint32_t>::iterator pos;
     uint32_t ret_val = 0;
     
     success = true;
     pos = m_memory.find(p_address);
     if (pos != m_memory.end())
         ret_val = pos->second;

 {
     if (!baton)
         return 0;
         
     bool success = true;
     EmulationStateARM *pseudo_state = (EmulationStateARM *) baton;
     if (length <= 4)
     {
-        uint32_t value = pseudo_state->ReadFromPseudoAddress (addr, length, success);
+        uint32_t value = pseudo_state->ReadFromPseudoAddress (addr, success);
         if (!success)
             return 0;
             
+        if (endian::InlHostByteOrder() == lldb::eByteOrderBig)
+            value = llvm::ByteSwap_32 (value);
         *((uint32_t *) dst) = value;
     }
     else if (length == 8)
     {
-        uint32_t value1 = pseudo_state->ReadFromPseudoAddress (addr, 4, success);
+        uint32_t value1 = pseudo_state->ReadFromPseudoAddress (addr, success);
         if (!success)
             return 0;
             
-        uint32_t value2 = pseudo_state->ReadFromPseudoAddress (addr + 4, 4, success);
+        uint32_t value2 = pseudo_state->ReadFromPseudoAddress (addr + 4, success);
         if (!success)
             return 0;
             
-        uint64_t value64 = value2;
-        value64 = (value64 << 32) | value1;
-        *((uint64_t *) dst) = value64;
+        if (endian::InlHostByteOrder() == lldb::eByteOrderBig)
+        {
+            value1 = llvm::ByteSwap_32 (value1);
+            value2 = llvm::ByteSwap_32 (value2);
+        }
+        ((uint32_t *) dst)[0] = value1;
+        ((uint32_t *) dst)[1] = value2;
     }
     else
         success = false;
     
     if (success)
         return length;
     
     return 0;
 }
     
 size_t
 EmulationStateARM::WritePseudoMemory (EmulateInstruction *instruction,
                                       void *baton,
                                       const EmulateInstruction::Context &context,
                                       lldb::addr_t addr,
                                       const void *dst,
                                       size_t length)
 {
     if (!baton)
         return 0;
         
-    bool success;
     EmulationStateARM *pseudo_state = (EmulationStateARM *) baton;
-    uint64_t value = *((const uint64_t *) dst);
-    success = pseudo_state->StoreToPseudoAddress (addr, value, length);
-    if (success)
+
+    if (length <= 4)
+    {
+        uint32_t value = *((const uint32_t *) dst);
+        if (endian::InlHostByteOrder() == lldb::eByteOrderBig)
+            value = llvm::ByteSwap_32 (value);
+
+        pseudo_state->StoreToPseudoAddress (addr, value);
+        return length;
+    }
+    else if (length == 8)
+    {
+        uint32_t value1 = ((const uint32_t *) dst)[0];
+        uint32_t value2 = ((const uint32_t *) dst)[1];
+        if (endian::InlHostByteOrder() == lldb::eByteOrderBig)
+        {
+            value1 = llvm::ByteSwap_32 (value1);
+            value2 = llvm::ByteSwap_32 (value2);
+        }
+
+        pseudo_state->StoreToPseudoAddress (addr, value1);
+        pseudo_state->StoreToPseudoAddress (addr + 4, value2);
         return length;
-        
+    }
     return 0;
 }
     
 bool
 EmulationStateARM::ReadPseudoRegister (EmulateInstruction *instruction,
                                        void *baton,
                                        const lldb_private::RegisterInfo *reg_info,
                                        lldb_private::RegisterValue &reg_value)
EmulationStateARM::CompareState (EmulationStateARM &other_state)
     bool match = true;
    
     for (int i = 0; match && i < 17; ++i)
     {
         if (m_gpr[i] != other_state.m_gpr[i])
             match = false;
     }
     
-    for (int i = 0; match && i < 16; ++i)
-    {
-        if (m_vfp_regs.sd_regs[i].s_reg[0] != other_state.m_vfp_regs.sd_regs[i].s_reg[0])
-            match = false;
-
-        if (m_vfp_regs.sd_regs[i].s_reg[1] != other_state.m_vfp_regs.sd_regs[i].s_reg[1])
-            match = false;
-    }
-    
     for (int i = 0; match && i < 32; ++i)
     {
-        if (i < 16)
-        {
-            if (m_vfp_regs.sd_regs[i].d_reg != other_state.m_vfp_regs.sd_regs[i].d_reg)
-                match = false;
-        }
-        else
-        {
-            if (m_vfp_regs.d_regs[i - 16] != other_state.m_vfp_regs.d_regs[i - 16])
-                match = false;
-        }
-    }
+        if (m_vfp_regs.s_regs[i] != other_state.m_vfp_regs.s_regs[i])
+    
+    for (int i = 0; match && i < 16; ++i)
+        if (m_vfp_regs.d_regs[i] != other_state.m_vfp_regs.d_regs[i])
     
     return match;
 }
 
 bool
 EmulationStateARM::LoadStateFromDictionary (OptionValueDictionary *test_data)
 {
     static ConstString memory_key ("memory");
if (value_sp.get() != NULL)
         uint32_t address = (uint32_t) start_address;
         
         for (uint32_t i = 0; i < num_elts; ++i)
         {
             value_sp = mem_array->GetValueAtIndex (i);
             if (value_sp.get() == NULL)
                 return false;
             uint64_t value = value_sp->GetUInt64Value();
-            StoreToPseudoAddress (address, value, 4);
+            StoreToPseudoAddress (address, value);
             address = address + 4;
         }
     }
     
     value_sp = test_data->GetValueForKey (registers_key);
     if (value_sp.get() == NULL)
         return false;