Re: [Lldb-commits] [PATCH] D18984: Fix ARM instruction emulation tests on big-endian systems

2016-04-14 Thread Ulrich Weigand via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL266314: Fix ARM instruction emulation tests on big-endian 
systems (authored by uweigand).

Changed prior to commit:
  http://reviews.llvm.org/D18984?vs=53500&id=53713#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D18984

Files:
  lldb/trunk/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
  lldb/trunk/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
  lldb/trunk/source/Plugins/Instruction/ARM/EmulationStateARM.h

Index: lldb/trunk/source/Plugins/Instruction/ARM/EmulationStateARM.h
===
--- lldb/trunk/source/Plugins/Instruction/ARM/EmulationStateARM.h
+++ lldb/trunk/source/Plugins/Instruction/ARM/EmulationStateARM.h
@@ -30,10 +30,10 @@
 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 ();
@@ -82,11 +82,7 @@
 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
  
Index: lldb/trunk/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
===
--- lldb/trunk/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
+++ lldb/trunk/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
@@ -61,11 +61,15 @@
 
 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;
@@ -82,16 +86,18 @@
 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;
@@ -110,14 +116,15 @@
 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;
@@ -131,8 +138,8 @@
 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;
@@ -145,23 +152,14 @@
 }
 
 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;
-}
+m_memory[p_address] = value;
 return true;
 }
 
 uint32_t
-EmulationStateARM::ReadFromPseudoAddre

Re: [Lldb-commits] [PATCH] D18984: Fix ARM instruction emulation tests on big-endian systems

2016-04-13 Thread Muhammad Omair Javaid via lldb-commits 
omjavaid accepted this revision.
omjavaid added a comment.

LGTM


http://reviews.llvm.org/D18984



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D18984: Fix ARM instruction emulation tests on big-endian systems

2016-04-13 Thread Tamas Berghammer via lldb-commits 
tberghammer accepted this revision.
tberghammer added a comment.

Looks good


http://reviews.llvm.org/D18984



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D18984: Fix ARM instruction emulation tests on big-endian systems

2016-04-12 Thread Ulrich Weigand via lldb-commits 
uweigand added a comment.

In http://reviews.llvm.org/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!



Comment at: source/Plugins/Instruction/ARM/EmulationStateARM.cpp:127
@@ -119,3 +126,3 @@
 else
-value = m_vfp_regs.d_regs[reg_num - dwarf_d16];
+value = m_vfp_regs.d_regs[idx - 16];
 }

Good catch!  Now fixed.


Comment at: source/Plugins/Instruction/ARM/EmulationStateARM.cpp:157
@@ -149,12 +156,3 @@
 {
-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;
-}
+m_memory[p_address] = value;
 return true;

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


http://reviews.llvm.org/D18984



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D18984: Fix ARM instruction emulation tests on big-endian systems

2016-04-12 Thread Ulrich Weigand via lldb-commits 
uweigand updated this revision to Diff 53500.

http://reviews.llvm.org/D18984

Files:
  source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
  source/Plugins/Instruction/ARM/EmulationStateARM.cpp
  source/Plugins/Instruction/ARM/EmulationStateARM.h

Index: source/Plugins/Instruction/ARM/EmulationStateARM.h
===
--- source/Plugins/Instruction/ARM/EmulationStateARM.h
+++ source/Plugins/Instruction/ARM/EmulationStateARM.h
@@ -30,10 +30,10 @@
 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 ();
@@ -82,11 +82,7 @@
 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
  
Index: source/Plugins/Instruction/ARM/EmulationStateARM.cpp
===
--- source/Plugins/Instruction/ARM/EmulationStateARM.cpp
+++ source/Plugins/Instruction/ARM/EmulationStateARM.cpp
@@ -61,11 +61,15 @@
 
 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;
@@ -82,16 +86,18 @@
 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;
@@ -110,14 +116,15 @@
 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;
@@ -131,8 +138,8 @@
 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;
@@ -145,23 +152,14 @@
 }
 
 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;
-}
+m_memory[p_address] = value;
 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::iterator pos;
 uint32_t ret_val = 0;
@@ -191,25 +189,31 @@
 EmulationStateARM *pseudo_state = (EmulationStateARM *) baton;
 if (length <= 4)
 {
-uint32_t value

Re: [Lldb-commits] [PATCH] D18984: Fix ARM instruction emulation tests on big-endian systems

2016-04-12 Thread Tamas Berghammer via lldb-commits 
tberghammer added a comment.

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



Comment at: source/Plugins/Instruction/ARM/EmulationStateARM.cpp:127
@@ -119,3 +126,3 @@
 else
-value = m_vfp_regs.d_regs[reg_num - dwarf_d16];
+value = m_vfp_regs.d_regs[idx];
 }

I think you need "idx - 16" here


Comment at: source/Plugins/Instruction/ARM/EmulationStateARM.cpp:157
@@ -149,12 +156,3 @@
 {
-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;
-}
+m_memory[p_address] = value;
 return true;

omjavaid wrote:
> 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.
> 
> 
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.


http://reviews.llvm.org/D18984



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D18984: Fix ARM instruction emulation tests on big-endian systems

2016-04-12 Thread Muhammad Omair Javaid via lldb-commits 
omjavaid added a comment.

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?



Comment at: source/Plugins/Instruction/ARM/EmulationStateARM.cpp:157
@@ -149,12 +156,3 @@
 {
-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;
-}
+m_memory[p_address] = value;
 return true;

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.




http://reviews.llvm.org/D18984



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D18984: Fix ARM instruction emulation tests on big-endian systems

2016-04-11 Thread Greg Clayton via lldb-commits 
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

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


http://reviews.llvm.org/D18984



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D18984: Fix ARM instruction emulation tests on big-endian systems

2016-04-11 Thread Ulrich Weigand via lldb-commits 
uweigand created this revision.
uweigand added a reviewer: clayborg.
uweigand added a subscriber: lldb-commits.
Herald added subscribers: rengolin, aemerson.

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.


http://reviews.llvm.org/D18984

Files:
  source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
  source/Plugins/Instruction/ARM/EmulationStateARM.cpp
  source/Plugins/Instruction/ARM/EmulationStateARM.h

Index: source/Plugins/Instruction/ARM/EmulationStateARM.h
===
--- source/Plugins/Instruction/ARM/EmulationStateARM.h
+++ source/Plugins/Instruction/ARM/EmulationStateARM.h
@@ -82,11 +82,7 @@
 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
  
Index: source/Plugins/Instruction/ARM/EmulationStateARM.cpp
===
--- source/Plugins/Instruction/ARM/EmulationStateARM.cpp
+++ source/Plugins/Instruction/ARM/EmulationStateARM.cpp
@@ -61,11 +61,15 @@
 
 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;
@@ -82,16 +86,18 @@
 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;
@@ -110,14 +116,15 @@
 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];
 }
 else
 success = false;
@@ -131,8 +138,8 @@
 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;
@@ -147,16 +154,7 @@
 bool
 EmulationStateARM::StoreToPseudoAddress (lldb::addr_t p_address,