[PATCH 0/5] arm: set of big endian related fixes for armeb (v7)

[PATCH 0/5] arm: set of big endian related fixes for armeb (v7)

[Victor Kamensky]

Hi Folks,

Please find five patches following this cover letter that address several
big endian related issues for ARM V7 target. Problems were discovered by
running gdb/testsuite on armeb (big endian) target rootfs/kernel and 
comparing results from the same source against arm (little endian).

Note there are several failures remain that are not addressed by this 
series. I.e valgrind related test cases all failed, because valgrind does 
not support armeb target. There are other failures in a bit obscure
places like gdb.dwarf2/implptrpiece.exp, gdb.python/py-value-cc.exp,
but over all number of passes/failures significantly improved in armeb
case. 

Please note it is my first attempt to contribute into gdb. If I missed
something please guide me how I can correct that.

Thanks,
Victor

Victor Kamensky (5):
  ARM: plt_size functions need to read instructions in right byte order
  ARM: extract_arm_insn function need to read instrs correctly in be8
    case
  ARM: arm_breakpoint should be little endian form in case for arm BE8
  ARM: read_pieced_value do big endian processing only in case of valid
    gdb_regnum
  ARM: asm-source.exp link options in case of armv7b target

 bfd/ChangeLog                        |  9 +++++++
 bfd/elf32-arm.c                      | 48 +++++++++++++++++++++++++++++++++---
 gdb/ChangeLog                        | 16 ++++++++++++
 gdb/arm-linux-tdep.c                 |  2 +-
 gdb/arm-tdep.c                       |  2 +-
 gdb/dwarf2loc.c                      | 30 +++++++++++-----------
 gdb/testsuite/ChangeLog              |  4 +++
 gdb/testsuite/gdb.asm/asm-source.exp |  4 +++
 8 files changed, 94 insertions(+), 21 deletions(-)

-- 
1.8.1.4

[PATCH 1/5] ARM: plt_size functions need to read instructions in right byte order

[Victor Kamensky]

elf32_arm_plt0_size and elf32_arm_plt_size read instructions
to determine what is size of PLT entry. However it does not
read instruction correctly in case of ARM big endian V7 case.
In this case instructions are still kept in little endian
order (BE8).

Because of that in armv7b case gdb.base/dprintf-pending.exp
test is failing - It cannot find 'pendfunc@plt' symbol.
And that symbol is not created because elf32_arm_get_synthetic_symtab
function does not create 'pendfunc@plt' symbol for symbols
from PLT after elf32_arm_plt0_size returns -1.

Fix is to introduce code reading functions read_code32,
read_code16 which would read code content in little endian
mode when it is armv7b executabe (i.e e_flags has EF_ARM_BE8)
set. elf32_arm_plt0_size and elf32_arm_plt_size to use these
functions in place where H_GET_32, H_GET_16 were used before.
---
 bfd/ChangeLog   |  9 +++++++++
 bfd/elf32-arm.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index e4445dc..8b183ac 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,12 @@
+2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
+
+	* elf32-arm.c (read_code32): New function to read 32 bit
+	arm instruction.
+	(read_code16): New function to read 16 bit thumb instrution.
+	(elf32_arm_plt0_size, elf32_arm_plt_size) change code to use
+	read_code32, read_code16 to read instruction to deal with
+	BE8 arm case.
+
 2014-09-29  H.J. Lu  <hongjiu.lu@intel.com>
 
 	PR ld/17440
diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
index 08aa3f9..89e4f35 100644
--- a/bfd/elf32-arm.c
+++ b/bfd/elf32-arm.c
@@ -15953,6 +15953,46 @@ const struct elf_size_info elf32_arm_size_info =
   bfd_elf32_swap_reloca_out
 };
 
+static bfd_vma
+read_code32 (const bfd *abfd, const bfd_byte *addr)
+{
+  bfd_vma retval;
+
+  if ((elf_elfheader(abfd)->e_flags) & EF_ARM_BE8)
+    {
+       /*
+	* V7 BE8 code is always little endian
+	*/
+       retval = bfd_getl32(addr);
+    }
+  else
+    {
+       retval = H_GET_32(abfd, addr);
+    }
+  return retval;
+}
+
+
+static bfd_vma
+read_code16 (const bfd *abfd, const bfd_byte *addr)
+{
+  bfd_vma retval;
+
+  if ((elf_elfheader(abfd)->e_flags) & EF_ARM_BE8)
+    {
+       /*
+	* V7 BE8 code is always little endian
+	*/
+       retval = bfd_getl16(addr);
+    }
+  else
+    {
+       retval = H_GET_16(abfd, addr);
+    }
+  return retval;
+}
+
+
 /* Return size of plt0 entry starting at ADDR
    or (bfd_vma) -1 if size can not be determined.  */
 
@@ -15962,7 +16002,7 @@ elf32_arm_plt0_size (const bfd *abfd, const bfd_byte *addr)
   bfd_vma first_word;
   bfd_vma plt0_size;
 
-  first_word = H_GET_32 (abfd, addr);
+  first_word = read_code32 (abfd, addr);
 
   if (first_word == elf32_arm_plt0_entry[0])
     plt0_size = 4 * ARRAY_SIZE (elf32_arm_plt0_entry);
@@ -15987,17 +16027,17 @@ elf32_arm_plt_size (const bfd *abfd, const bfd_byte *start, bfd_vma offset)
   const bfd_byte *addr = start + offset;
 
   /* PLT entry size if fixed on Thumb-only platforms.  */
-  if (H_GET_32(abfd, start) == elf32_thumb2_plt0_entry[0])
+  if (read_code32(abfd, start) == elf32_thumb2_plt0_entry[0])
       return 4 * ARRAY_SIZE (elf32_thumb2_plt_entry);
 
   /* Respect Thumb stub if necessary.  */
-  if (H_GET_16(abfd, addr) == elf32_arm_plt_thumb_stub[0])
+  if (read_code16(abfd, addr) == elf32_arm_plt_thumb_stub[0])
     {
       plt_size += 2 * ARRAY_SIZE(elf32_arm_plt_thumb_stub);
     }
 
   /* Strip immediate from first add.  */
-  first_insn = H_GET_32(abfd, addr + plt_size) & 0xffffff00;
+  first_insn = read_code32(abfd, addr + plt_size) & 0xffffff00;
 
 #ifdef FOUR_WORD_PLT
   if (first_insn == elf32_arm_plt_entry[0])
-- 
1.8.1.4

[PATCH 5/5] ARM: asm-source.exp link options in case of armv7b target

[Victor Kamensky]

gdb.asm/asm-source.exp fails in armv7b case, because it does
not pass --be8 option to link, as result instructions in asm-source
executable are in big endian order and crash with SIGILL.

Solution is to add --be8 option to link command during test creation.
---
 gdb/testsuite/ChangeLog              | 4 ++++
 gdb/testsuite/gdb.asm/asm-source.exp | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b7cc1c6..1d5fefa 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
+
+	* gdb.asm/asm-source.exp: add armv7b case for target.
+
 2014-09-30  Yao Qi  <yao@codesourcery.com>
 
 	* lib/prelink-support.exp (build_executable_own_libs): Error if
diff --git a/gdb/testsuite/gdb.asm/asm-source.exp b/gdb/testsuite/gdb.asm/asm-source.exp
index fa4585c..153bc50 100644
--- a/gdb/testsuite/gdb.asm/asm-source.exp
+++ b/gdb/testsuite/gdb.asm/asm-source.exp
@@ -37,6 +37,10 @@ switch -glob -- [istarget] {
         set asm-flags "-no-mdebug -I${srcdir}/${subdir} $obj_include"
 	set debug-flags "-gdwarf-2"
     }
+    "armv7b-*-*" {
+	set asm-arch arm
+	append link-flags " -be8"
+    }
     "arm*-*-*" {
         set asm-arch arm
     }
-- 
1.8.1.4

Re: [PATCH 5/5] ARM: asm-source.exp link options in case of armv7b target

[Yao Qi]

Victor Kamensky <victor.kamensky@linaro.org> writes:

> +    "armv7b-*-*" {
> +	set asm-arch arm
> +	append link-flags " -be8"
> +    }

We can't tell whether "-be8" is needed from the target triplet.
Considering multi-lib, "-be8" is needed for one multilib, but not
for the other.

Maybe, you can fix your problem by running tests via LDFLAGS_FOR_TARGET,

$ make check RUNTESTFLAGS='LDFLAGS_FOR_TARGET=-be8'

or you can create your own board file foo.exp, and add

set_board_info ldflags  "-be8"

$ make check RUNTESTFLAGS='--target_board=foo'

-- 
Yao (齐尧)

Re: [PATCH 5/5] ARM: asm-source.exp link options in case of armv7b target

[Victor Kamensky]

Hi Yao,

On 23 October 2014 23:05, Yao Qi <yao@codesourcery.com> wrote:
> Victor Kamensky <victor.kamensky@linaro.org> writes:
>
>> +    "armv7b-*-*" {
>> +     set asm-arch arm
>> +     append link-flags " -be8"
>> +    }
>
> We can't tell whether "-be8" is needed from the target triplet.
> Considering multi-lib, "-be8" is needed for one multilib, but not
> for the other.

Any executable/library that runs on big endian V7 *must* be linked
with -be8 option. Otherwise it simply won't run. In any other multilib
option vfp, neon, etc -be8 must be set. Basically, in big endian case
gcc/gas generates data and instructions in big endian
format but ARM V7 requires that instruction should be little endian
format. It is linker that does instructions byte swap. If -be8 flag
is not passed during link while running on ARM V7 big endian target
executable with crash with SIGILL. If link happens through gcc, then
-be8 always passed for non relocatable code by compiler. In this
particular case link happens directly with linker and -be8 is not
default, so it is needed. One may argue that -be8 for final
executables in ARM V7 BE target should be default even for
linker, but it is not the current case ...

Also note that you have plenty examples in the same test
gdb/testsuite/gdb.asm/asm-source.exp
that do very similar things. For example:

    "powerpc64le-*" {
        set asm-arch powerpc64le
        set asm-flags "-a64 -I${srcdir}/${subdir} $obj_include"
        append link-flags " -m elf64lppc"
    }

Why "-m elf64lppc" is set for powerpc64le target? I suspect
by very similar reasons.

> Maybe, you can fix your problem by running tests via LDFLAGS_FOR_TARGET,
>
> $ make check RUNTESTFLAGS='LDFLAGS_FOR_TARGET=-be8'
>
> or you can create your own board file foo.exp, and add
>
> set_board_info ldflags  "-be8"

I don't feel very strong about it, and definitely I can workaround
this issue or just ignore the failure. It just seemed that it was very
easy to fix.

If you are still not convinced by above argument, yes,
let's just drop it. Please let me know you final thoughts. We
will proceed accordingly. I am fine either way.

Thanks,
Victor

> $ make check RUNTESTFLAGS='--target_board=foo'
>
> --
> Yao (齐尧)

Re: [PATCH 5/5] ARM: asm-source.exp link options in case of armv7b target

[Andrew Pinski]

On Thu, Oct 23, 2014 at 11:35 PM, Victor Kamensky
<victor.kamensky@linaro.org> wrote:
> Hi Yao,
>
> On 23 October 2014 23:05, Yao Qi <yao@codesourcery.com> wrote:
>> Victor Kamensky <victor.kamensky@linaro.org> writes:
>>
>>> +    "armv7b-*-*" {
>>> +     set asm-arch arm
>>> +     append link-flags " -be8"
>>> +    }
>>
>> We can't tell whether "-be8" is needed from the target triplet.
>> Considering multi-lib, "-be8" is needed for one multilib, but not
>> for the other.
>
> Any executable/library that runs on big endian V7 *must* be linked
> with -be8 option. Otherwise it simply won't run. In any other multilib
> option vfp, neon, etc -be8 must be set. Basically, in big endian case
> gcc/gas generates data and instructions in big endian
> format but ARM V7 requires that instruction should be little endian
> format. It is linker that does instructions byte swap. If -be8 flag
> is not passed during link while running on ARM V7 big endian target
> executable with crash with SIGILL. If link happens through gcc, then
> -be8 always passed for non relocatable code by compiler. In this
> particular case link happens directly with linker and -be8 is not
> default, so it is needed. One may argue that -be8 for final
> executables in ARM V7 BE target should be default even for
> linker, but it is not the current case ...
>
> Also note that you have plenty examples in the same test
> gdb/testsuite/gdb.asm/asm-source.exp
> that do very similar things. For example:
>
>     "powerpc64le-*" {
>         set asm-arch powerpc64le
>         set asm-flags "-a64 -I${srcdir}/${subdir} $obj_include"
>         append link-flags " -m elf64lppc"
>     }
>
> Why "-m elf64lppc" is set for powerpc64le target? I suspect
> by very similar reasons.


Yes and no.  For PowerPC64 little-endian is Linux only so it will
never have a multi-libs that support both little-endian and
big-endian.  While for arm*-*-*, you can have a bare metal env and
that could have a multi-lib for both little and big endian.

This is true for MIPS too.

Thanks,
Andrew Pinski


>
>> Maybe, you can fix your problem by running tests via LDFLAGS_FOR_TARGET,
>>
>> $ make check RUNTESTFLAGS='LDFLAGS_FOR_TARGET=-be8'
>>
>> or you can create your own board file foo.exp, and add
>>
>> set_board_info ldflags  "-be8"
>
> I don't feel very strong about it, and definitely I can workaround
> this issue or just ignore the failure. It just seemed that it was very
> easy to fix.
>
> If you are still not convinced by above argument, yes,
> let's just drop it. Please let me know you final thoughts. We
> will proceed accordingly. I am fine either way.
>
> Thanks,
> Victor
>
>> $ make check RUNTESTFLAGS='--target_board=foo'
>>
>> --
>> Yao (齐尧)

Re: [PATCH 5/5] ARM: asm-source.exp link options in case of armv7b target

[Yao Qi]

Andrew Pinski <pinskia@gmail.com> writes:

>> Any executable/library that runs on big endian V7 *must* be linked
>> with -be8 option. Otherwise it simply won't run. In any other multilib
>> option vfp, neon, etc -be8 must be set. Basically, in big endian case
>> gcc/gas generates data and instructions in big endian
>> format but ARM V7 requires that instruction should be little endian
>> format. It is linker that does instructions byte swap. If -be8 flag
>> is not passed during link while running on ARM V7 big endian target
>> executable with crash with SIGILL. If link happens through gcc, then
>> -be8 always passed for non relocatable code by compiler. In this
>> particular case link happens directly with linker and -be8 is not
>> default, so it is needed. One may argue that -be8 for final
>> executables in ARM V7 BE target should be default even for
>> linker, but it is not the current case ...
>>
>> Also note that you have plenty examples in the same test
>> gdb/testsuite/gdb.asm/asm-source.exp
>> that do very similar things. For example:
>>
>>     "powerpc64le-*" {
>>         set asm-arch powerpc64le
>>         set asm-flags "-a64 -I${srcdir}/${subdir} $obj_include"
>>         append link-flags " -m elf64lppc"
>>     }
>>
>> Why "-m elf64lppc" is set for powerpc64le target? I suspect
>> by very similar reasons.
>
>
> Yes and no.  For PowerPC64 little-endian is Linux only so it will
> never have a multi-libs that support both little-endian and
> big-endian.  While for arm*-*-*, you can have a bare metal env and
> that could have a multi-lib for both little and big endian.

Andrew is right.  We can have a arm-linux-gnueabi toolchain which has
multilibs for the combination of {le, be} x {armv7, armv6, armv5}, and
this test still fails on armv7 be multilib.

-- 
Yao (齐尧)

Re: [PATCH 5/5] ARM: asm-source.exp link options in case of armv7b target

[Victor Kamensky]

On 24 October 2014 01:52, Yao Qi <yao@codesourcery.com> wrote:
> Andrew Pinski <pinskia@gmail.com> writes:
>
>>> Any executable/library that runs on big endian V7 *must* be linked
>>> with -be8 option. Otherwise it simply won't run. In any other multilib
>>> option vfp, neon, etc -be8 must be set. Basically, in big endian case
>>> gcc/gas generates data and instructions in big endian
>>> format but ARM V7 requires that instruction should be little endian
>>> format. It is linker that does instructions byte swap. If -be8 flag
>>> is not passed during link while running on ARM V7 big endian target
>>> executable with crash with SIGILL. If link happens through gcc, then
>>> -be8 always passed for non relocatable code by compiler. In this
>>> particular case link happens directly with linker and -be8 is not
>>> default, so it is needed. One may argue that -be8 for final
>>> executables in ARM V7 BE target should be default even for
>>> linker, but it is not the current case ...
>>>
>>> Also note that you have plenty examples in the same test
>>> gdb/testsuite/gdb.asm/asm-source.exp
>>> that do very similar things. For example:
>>>
>>>     "powerpc64le-*" {
>>>         set asm-arch powerpc64le
>>>         set asm-flags "-a64 -I${srcdir}/${subdir} $obj_include"
>>>         append link-flags " -m elf64lppc"
>>>     }
>>>
>>> Why "-m elf64lppc" is set for powerpc64le target? I suspect
>>> by very similar reasons.
>>
>>
>> Yes and no.  For PowerPC64 little-endian is Linux only so it will
>> never have a multi-libs that support both little-endian and
>> big-endian.  While for arm*-*-*, you can have a bare metal env and
>> that could have a multi-lib for both little and big endian.
>
> Andrew is right.  We can have a arm-linux-gnueabi toolchain which has
> multilibs for the combination of {le, be} x {armv7, armv6, armv5}, and
> this test still fails on armv7 be multilib.

I am not sure that I follow your argument, Your point that for
arm-linux-gnueab that has big endian multilib test will fail, and because
of that we want it to keep failing on armv7b-unknown-linux-gnueabihf
target? The fix I suggested will be activate only if target triplet
starts with 'armv7b'. The situation when target name starts
with 'armv7b' and has little endian multilib support seems very
hypothetical to me.

In case of arm-linux-gnueabi with big endian multilib pretty much
all tests will fail, unless caller will not specify correct compile/link
option (i.e compile -mbig-endian -Wl,--be8, link --be8, etc).

OK, it seems that the patch causes too much controversy, I am
going to drop it. I'll re-post series without it with final version of
other 3 patches, the one that I believe approved by you. And I
will wait for few days for additional feedback.

Thanks,
Victor

> --
> Yao (齐尧)

[PATCH 3/5] ARM: arm_breakpoint should be little endian form in case for arm BE8

[Victor Kamensky]

tdep->arm_breakpoint, tdep->thumb_breakpoint, tdep->thumb2_breakpoint
should be set le_ variants in case of arm BE8 code. Those instruciton
sequences are writen to target with simple write_memory, without
regarding gdbarch_byte_order_for_code. But in BE8 case even data
memory is in big endian form, instructions are still in little endian
form.

Because of this issue there are many issues while running gdb test
case in armv7b mode. For example gdb.arch/arm-disp-step.exp test fails
because it gets SIGILL when displaced instrucion sequence reaches
break instruction, which is in wrong byte order.

Solution is to set tdep->xxx_breakpoint sequences in BE8 case (i.e
when gdbarch_byte_order_for_code is BFD_ENDIAN_BIG.
---
 gdb/ChangeLog        | 5 +++++
 gdb/arm-linux-tdep.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2aef5dc..c32fb3f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
 
+	* arm-tdep.c: (extract_arm_insn): use dbarch_byte_order_for_code
+	to read arm instruction.
+
+2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
+
 	* arm-tdep.c (extract_arm_insn): use dbarch_byte_order_for_code
 	to read arm instruction.
 
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index e3587f3..2e79658 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -1361,7 +1361,7 @@ arm_linux_init_abi (struct gdbarch_info info,
   linux_init_abi (info, gdbarch);
 
   tdep->lowest_pc = 0x8000;
-  if (info.byte_order == BFD_ENDIAN_BIG)
+  if (info.byte_order_for_code == BFD_ENDIAN_BIG)
     {
       if (tdep->arm_abi == ARM_ABI_AAPCS)
 	tdep->arm_breakpoint = eabi_linux_arm_be_breakpoint;
-- 
1.8.1.4

Re: [PATCH 3/5] ARM: arm_breakpoint should be little endian form in case for arm BE8

[Yao Qi]

Victor Kamensky <victor.kamensky@linaro.org> writes:

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 2aef5dc..c32fb3f 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,10 @@
>  2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
>  
> +	* arm-tdep.c: (extract_arm_insn): use dbarch_byte_order_for_code
> +	to read arm instruction.

The file and function name in the changelog entry is wrong.  The
description of each entry is a sentence, so the first letter should be
capitalized.

	* arm-linux-tdep.c (arm_linux_init_abi): Use
	 gdbarch_byte_order_for_code to read arm instruction.

Otherwise, it looks good to me.

-- 
Yao (齐尧)

[PATCH 4/5] ARM: read_pieced_value do big endian processing only in case of valid gdb_regnum

[Victor Kamensky]

During armv7b testing gdb.base/store.exp test was failling with
'GDB internal error'. It turns out that compiler generated DWARF
with non-existent register numbers. The compiler issue is present
in both little endian (armv7) and big endian (armv7b) (it is
separate issue). In both case gdbarch_dwarf2_reg_to_regnum returns
-1 which is stored into gdb_regnum. But it cause severe problem
only in big endian case because in read_pieced_value and
write_pieced_value functions BFD_ENDIAN_BIG related processing
happen regardless of gdb_regnum value, and in case of gdb_regnum=-1,
it cause 'GDB internal error' and crash.

Solution is to move BFD_ENDIAN_BIG related processing under
(gdb_regnum != -1) branch of processing.
---
 gdb/ChangeLog   |  6 ++++++
 gdb/dwarf2loc.c | 30 +++++++++++++++---------------
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c32fb3f..6a735b8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,11 @@
 2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
 
+	* dwarf2loc.c (read_pieced_value): do BE processing only if
+	gdb_regnum is not -1.
+	(write_pieced_value): Ditto.
+
+2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
+
 	* arm-tdep.c: (extract_arm_insn): use dbarch_byte_order_for_code
 	to read arm instruction.
 
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index e347e59..fbe99bb 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1686,20 +1686,20 @@ read_pieced_value (struct value *v)
 	    int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch, p->v.regno);
 	    int reg_offset = source_offset;
 
-	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
-		&& this_size < register_size (arch, gdb_regnum))
-	      {
-		/* Big-endian, and we want less than full size.  */
-		reg_offset = register_size (arch, gdb_regnum) - this_size;
-		/* We want the lower-order THIS_SIZE_BITS of the bytes
-		   we extract from the register.  */
-		source_offset_bits += 8 * this_size - this_size_bits;
-	      }
-
 	    if (gdb_regnum != -1)
 	      {
 		int optim, unavail;
 
+		if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
+		    && this_size < register_size (arch, gdb_regnum))
+		  {
+		    /* Big-endian, and we want less than full size.  */
+		    reg_offset = register_size (arch, gdb_regnum) - this_size;
+		    /* We want the lower-order THIS_SIZE_BITS of the bytes
+		       we extract from the register.  */
+		    source_offset_bits += 8 * this_size - this_size_bits;
+		 }
+
 		if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset,
 					       this_size, buffer,
 					       &optim, &unavail))
@@ -1878,13 +1878,13 @@ write_pieced_value (struct value *to, struct value *from)
 	    int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch, p->v.regno);
 	    int reg_offset = dest_offset;
 
-	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
-		&& this_size <= register_size (arch, gdb_regnum))
-	      /* Big-endian, and we want less than full size.  */
-	      reg_offset = register_size (arch, gdb_regnum) - this_size;
-
 	    if (gdb_regnum != -1)
 	      {
+		if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
+		    && this_size <= register_size (arch, gdb_regnum))
+		  /* Big-endian, and we want less than full size.  */
+		  reg_offset = register_size (arch, gdb_regnum) - this_size;
+
 		if (need_bitwise)
 		  {
 		    int optim, unavail;
-- 
1.8.1.4

Re: [PATCH 4/5] ARM: read_pieced_value do big endian processing only in case of valid gdb_regnum

[Yao Qi]

Victor Kamensky <victor.kamensky@linaro.org> writes:

Hi Victor,
Could you please add more details in the commit message? for example....

> During armv7b testing gdb.base/store.exp test was failling with
> 'GDB internal error'. It turns out that compiler generated DWARF

What is the 'GDB internal error'?  Is it like this?

(gdb) PASS: gdb.base/store.exp: continue to wack_double
print l^M
gdb/regcache.c:178: internal-error: register_size: Assertion `regnum >= 0 && regnum < (gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch))' failed.^M
A problem internal to GDB has been detected,

We've seen this internal error on (armv5te big-endian) for a while.

> with non-existent register numbers. The compiler issue is present
> in both little endian (armv7) and big endian (armv7b) (it is
> separate issue). In both case gdbarch_dwarf2_reg_to_regnum returns

Is there any PR opened for the compiler issue?  If there is, please
mention it in the commit message, otherwise, please describe the
mistakes in the compiler generated debug info, the snippet of
'readelf -wi' output, which shows the wrong register number, should be fine.

> -1 which is stored into gdb_regnum. But it cause severe problem
> only in big endian case because in read_pieced_value and
> write_pieced_value functions BFD_ENDIAN_BIG related processing
> happen regardless of gdb_regnum value, and in case of gdb_regnum=-1,
> it cause 'GDB internal error' and crash.
>
> Solution is to move BFD_ENDIAN_BIG related processing under
> (gdb_regnum != -1) branch of processing.

With your patch applied, the internal error is fixed.  How does GDB
behave now?  What is the output for 'print l'?  In my case, it becomes:

print l^M
Unable to access DWARF register number 80^M
(gdb) FAIL: gdb.base/store.exp: upvar float l; print old l, expecting -1

> ---
>  gdb/ChangeLog   |  6 ++++++
>  gdb/dwarf2loc.c | 30 +++++++++++++++---------------
>  2 files changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index c32fb3f..6a735b8 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,11 @@
>  2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
>  
> +	* dwarf2loc.c (read_pieced_value): do BE processing only if
> +	gdb_regnum is not -1.

s/do/Do.  Looks you've fixed it in V2.
s/BE/big endian/ because BE isn't very clear here.

> +	(write_pieced_value): Ditto.
> +
> +2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
> +
>  	* arm-tdep.c: (extract_arm_insn): use dbarch_byte_order_for_code
>  	to read arm instruction.
>  
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index e347e59..fbe99bb 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -1686,20 +1686,20 @@ read_pieced_value (struct value *v)
>  	    int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch, p->v.regno);
>  	    int reg_offset = source_offset;
>  
> -	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
> -		&& this_size < register_size (arch, gdb_regnum))
> -	      {
> -		/* Big-endian, and we want less than full size.  */
> -		reg_offset = register_size (arch, gdb_regnum) - this_size;
> -		/* We want the lower-order THIS_SIZE_BITS of the bytes
> -		   we extract from the register.  */
> -		source_offset_bits += 8 * this_size - this_size_bits;
> -	      }
> -
>  	    if (gdb_regnum != -1)
>  	      {
>  		int optim, unavail;
>  
> +		if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
> +		    && this_size < register_size (arch, gdb_regnum))
> +		  {
> +		    /* Big-endian, and we want less than full size.  */
> +		    reg_offset = register_size (arch, gdb_regnum) - this_size;
> +		    /* We want the lower-order THIS_SIZE_BITS of the bytes
> +		       we extract from the register.  */
> +		    source_offset_bits += 8 * this_size - this_size_bits;
> +		 }
> +

Nit: after the change, local variable 'reg_offset' is only used in the
"if (gdb_regnum != -1) {}" block, so we can move 'reg_offset' into that
block.

-- 
Yao (齐尧)

Re: [PATCH 4/5] ARM: read_pieced_value do big endian processing only in case of valid gdb_regnum

[Victor Kamensky]

[-- Attachment #1: Type: text/plain, Size: 5863 bytes --]

Hi Yao,

I've attached my notes I captured while looking at
this issue. Some snippets from them repeated below
inline:

On 22 October 2014 02:27, Yao Qi <yao@codesourcery.com> wrote:
> Victor Kamensky <victor.kamensky@linaro.org> writes:
>
> Hi Victor,
> Could you please add more details in the commit message? for example....
>
>> During armv7b testing gdb.base/store.exp test was failling with
>> 'GDB internal error'. It turns out that compiler generated DWARF
>
> What is the 'GDB internal error'?  Is it like this?
>
> (gdb) PASS: gdb.base/store.exp: continue to wack_double
> print l^M
> gdb/regcache.c:178: internal-error: register_size: Assertion `regnum >= 0 && regnum < (gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch))' failed.^M
> A problem internal to GDB has been detected,

Yes, it is like this. I'll added into commit message.

> We've seen this internal error on (armv5te big-endian) for a while.
>
>> with non-existent register numbers. The compiler issue is present
>> in both little endian (armv7) and big endian (armv7b) (it is
>> separate issue). In both case gdbarch_dwarf2_reg_to_regnum returns
>
> Is there any PR opened for the compiler issue?  If there is, please
> mention it in the commit message, otherwise, please describe the
> mistakes in the compiler generated debug info, the snippet of
> 'readelf -wi' output, which shows the wrong register number, should be fine.

In both little endian and big endian cases compiler generate DW_OP_reg29-
DW_OP_reg31 something like this.

 <2><792>: Abbrev Number: 10 (DW_TAG_formal_parameter)
    <793>   DW_AT_name        : u
    <795>   DW_AT_decl_file   : 1
    <796>   DW_AT_decl_line   : 115
    <797>   DW_AT_type        : <0x57c>
    <79b>   DW_AT_location    : 6 byte block: 6d 93 4 6c 93 4
(DW_OP_reg29 (r29); DW_OP_piece: 4; DW_OP_reg28 (r28); DW_OP_piece: 4)

I strongly suspect that it is compiler error, but more accurately
it is hard to say, because I never saw a document where for given CPU
mapping from registers to DWARF reg numbers is defined. Have you
seen such document for example for ARM V7? In any case for this
test case Gdb believes that those register numbers are wrong. I.e we
can say for sure that gcc and gdb are disagrees.

>> -1 which is stored into gdb_regnum. But it cause severe problem
>> only in big endian case because in read_pieced_value and
>> write_pieced_value functions BFD_ENDIAN_BIG related processing
>> happen regardless of gdb_regnum value, and in case of gdb_regnum=-1,
>> it cause 'GDB internal error' and crash.
>>
>> Solution is to move BFD_ENDIAN_BIG related processing under
>> (gdb_regnum != -1) branch of processing.
>
> With your patch applied, the internal error is fixed.  How does GDB
> behave now?  What is the output for 'print l'?  In my case, it becomes:
>
> print l^M
> Unable to access DWARF register number 80^M
> (gdb) FAIL: gdb.base/store.exp: upvar float l; print old l, expecting -1

Yes it becomes the same as little endian result for the same test case.

The only issue addressed by patch that it makes big endian case behave
consistently with little endian - i.e don't do register_size call for unknown
register number

>> ---
>>  gdb/ChangeLog   |  6 ++++++
>>  gdb/dwarf2loc.c | 30 +++++++++++++++---------------
>>  2 files changed, 21 insertions(+), 15 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index c32fb3f..6a735b8 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,5 +1,11 @@
>>  2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
>>
>> +     * dwarf2loc.c (read_pieced_value): do BE processing only if
>> +     gdb_regnum is not -1.
>
> s/do/Do.  Looks you've fixed it in V2.
> s/BE/big endian/ because BE isn't very clear here.
>
>> +     (write_pieced_value): Ditto.
>> +
>> +2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
>> +
>>       * arm-tdep.c: (extract_arm_insn): use dbarch_byte_order_for_code
>>       to read arm instruction.
>>
>> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
>> index e347e59..fbe99bb 100644
>> --- a/gdb/dwarf2loc.c
>> +++ b/gdb/dwarf2loc.c
>> @@ -1686,20 +1686,20 @@ read_pieced_value (struct value *v)
>>           int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch, p->v.regno);
>>           int reg_offset = source_offset;
>>
>> -         if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
>> -             && this_size < register_size (arch, gdb_regnum))
>> -           {
>> -             /* Big-endian, and we want less than full size.  */
>> -             reg_offset = register_size (arch, gdb_regnum) - this_size;
>> -             /* We want the lower-order THIS_SIZE_BITS of the bytes
>> -                we extract from the register.  */
>> -             source_offset_bits += 8 * this_size - this_size_bits;
>> -           }
>> -
>>           if (gdb_regnum != -1)
>>             {
>>               int optim, unavail;
>>
>> +             if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
>> +                 && this_size < register_size (arch, gdb_regnum))
>> +               {
>> +                 /* Big-endian, and we want less than full size.  */
>> +                 reg_offset = register_size (arch, gdb_regnum) - this_size;
>> +                 /* We want the lower-order THIS_SIZE_BITS of the bytes
>> +                    we extract from the register.  */
>> +                 source_offset_bits += 8 * this_size - this_size_bits;
>> +              }
>> +
>
> Nit: after the change, local variable 'reg_offset' is only used in the
> "if (gdb_regnum != -1) {}" block, so we can move 'reg_offset' into that
> block.

Ok, I'll make this change too. I'll update comment note with details
and repost the patch.

Thanks,
Victor

> --
> Yao (齐尧)

[-- Attachment #2: gdb_armv7ab_badregnum.txt --]
[-- Type: text/plain, Size: 16615 bytes --]


(gdb) file /wd1/gdb/20140930/build-v7le/gdb/testsuite/gdb.base/store
Reading symbols from /wd1/gdb/20140930/build-v7le/gdb/testsuite/gdb.base/store...done.
(gdb) tbreak wack_double
Temporary breakpoint 1 at 0x1076c: file ../../../binutils-gdb/gdb/testsuite/gdb.base/store.c, line 117.
(gdb) run
Starting program: /wd1/gdb/20140930/build-v7le/gdb/testsuite/gdb.base/store 

Temporary breakpoint 1, wack_double (u=
../../binutils-gdb/gdb/regcache.c:177: internal-error: register_size: Assertion `regnum >= 0 && regnum < (gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch))' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.


BE Dump
=======


 <1><779>: Abbrev Number: 12 (DW_TAG_subprogram)
    <77a>   DW_AT_external    : 1       
    <77a>   DW_AT_name        : (indirect string, offset: 0x3c9): wack_double   
    <77e>   DW_AT_decl_file   : 1       
    <77f>   DW_AT_decl_line   : 115     
    <780>   DW_AT_prototyped  : 1       
    <780>   DW_AT_type        : <0x57c> 
    <784>   DW_AT_low_pc      : 0x10758 
    <788>   DW_AT_high_pc     : 0x40    
    <78c>   DW_AT_frame_base  : 1 byte block: 9c        (DW_OP_call_frame_cfa)
    <78e>   DW_AT_GNU_all_tail_call_sites: 1    
    <78e>   DW_AT_sibling     : <0x7d7> 
 <2><792>: Abbrev Number: 10 (DW_TAG_formal_parameter)
    <793>   DW_AT_name        : u       
    <795>   DW_AT_decl_file   : 1       
    <796>   DW_AT_decl_line   : 115     
    <797>   DW_AT_type        : <0x57c> 
    <79b>   DW_AT_location    : 6 byte block: 6d 93 4 6c 93 4   (DW_OP_reg29 (r29); DW_OP_piece: 4; DW_OP_reg28 (r28); DW_OP_piece: 4)
 <2><7a2>: Abbrev Number: 10 (DW_TAG_formal_parameter)
    <7a3>   DW_AT_name        : v       
    <7a5>   DW_AT_decl_file   : 1       
    <7a6>   DW_AT_decl_line   : 115     
    <7a7>   DW_AT_type        : <0x57c> 
    <7ab>   DW_AT_location    : 6 byte block: 6f 93 4 6e 93 4   (DW_OP_reg31 (r31); DW_OP_piece: 4; DW_OP_reg30 (r30); DW_OP_piece: 4)
 <2><7b2>: Abbrev Number: 13 (DW_TAG_variable)
    <7b3>   DW_AT_name        : l       
    <7b5>   DW_AT_decl_file   : 1       
    <7b6>   DW_AT_decl_line   : 117     
    <7b7>   DW_AT_type        : <0x57c> 
    <7bb>   DW_AT_location    : 8 byte block: 90 21 93 4 90 20 93 4     (DW_OP_regx: 33 (r33); DW_OP_piece: 4; DW_OP_regx: 32 (r32); DW_OP_piece: 4)
 <2><7c4>: Abbrev Number: 13 (DW_TAG_variable)
    <7c5>   DW_AT_name        : r       
    <7c7>   DW_AT_decl_file   : 1       
    <7c8>   DW_AT_decl_line   : 117     
    <7c9>   DW_AT_type        : <0x57c> 
    <7cd>   DW_AT_location    : 8 byte block: 90 23 93 4 90 22 93 4     (DW_OP_regx: 35 (r35); DW_OP_piece: 4; DW_OP_regx: 34 (r34); DW_OP_piece: 4)

LE Dump
=======

 <1><777>: Abbrev Number: 12 (DW_TAG_subprogram)
    <778>   DW_AT_external    : 1       
    <778>   DW_AT_name        : (indirect string, offset: 0x3c9): wack_double   
    <77c>   DW_AT_decl_file   : 1       
    <77d>   DW_AT_decl_line   : 115     
    <77e>   DW_AT_prototyped  : 1       
    <77e>   DW_AT_type        : <0x57a> 
    <782>   DW_AT_low_pc      : 0x10758 
    <786>   DW_AT_high_pc     : 0x40    
    <78a>   DW_AT_frame_base  : 1 byte block: 9c        (DW_OP_call_frame_cfa)
    <78c>   DW_AT_GNU_all_tail_call_sites: 1    
    <78c>   DW_AT_sibling     : <0x7d5> 
 <2><790>: Abbrev Number: 10 (DW_TAG_formal_parameter)
    <791>   DW_AT_name        : u       
    <793>   DW_AT_decl_file   : 1       
    <794>   DW_AT_decl_line   : 115     
    <795>   DW_AT_type        : <0x57a> 
    <799>   DW_AT_location    : 6 byte block: 6c 93 4 6d 93 4   (DW_OP_reg28 (r28); DW_OP_piece: 4; DW_OP_reg29 (r29); DW_OP_piece: 4)
 <2><7a0>: Abbrev Number: 10 (DW_TAG_formal_parameter)
    <7a1>   DW_AT_name        : v       
    <7a3>   DW_AT_decl_file   : 1       
    <7a4>   DW_AT_decl_line   : 115     
    <7a5>   DW_AT_type        : <0x57a> 
    <7a9>   DW_AT_location    : 6 byte block: 6e 93 4 6f 93 4   (DW_OP_reg30 (r30); DW_OP_piece: 4; DW_OP_reg31 (r31); DW_OP_piece: 4)
 <2><7b0>: Abbrev Number: 13 (DW_TAG_variable)
    <7b1>   DW_AT_name        : l       
    <7b3>   DW_AT_decl_file   : 1       
    <7b4>   DW_AT_decl_line   : 117     
    <7b5>   DW_AT_type        : <0x57a> 
    <7b9>   DW_AT_location    : 8 byte block: 90 20 93 4 90 21 93 4     (DW_OP_regx: 32 (r32); DW_OP_piece: 4; DW_OP_regx: 33 (r33); DW_OP_piece: 4)
 <2><7c2>: Abbrev Number: 13 (DW_TAG_variable)
    <7c3>   DW_AT_name        : r       
    <7c5>   DW_AT_decl_file   : 1       
    <7c6>   DW_AT_decl_line   : 117     
    <7c7>   DW_AT_type        : <0x57a> 
    <7cb>   DW_AT_location    : 8 byte block: 90 22 93 4 90 23 93 4     (DW_OP_regx: 34 (r34); DW_OP_piece: 4; DW_OP_regx: 35 (r35); DW_OP_piece: 4)


Backtrace when it failed to get reg number
==========================================

(gdb) n
4207	  if (reg >= 256 && reg <= 287)
(gdb) n
4216	  return -1;
(gdb) bt
#0  arm_dwarf_reg_to_regnum (gdbarch=0x24043a0, reg=29) at ../../binutils-gdb/gdb/arm-tdep.c:4216
#1  0x0020d430 in read_pieced_value (v=0x6c08) at ../../binutils-gdb/gdb/dwarf2loc.c:1686
#2  0x00144d7c in value_fetch_lazy (val=0x2635e08) at ../../binutils-gdb/gdb/value.c:3895
#3  0x001453b0 in value_entirely_covered_by_range_vector (value=0x2635e08, ranges=0x2635e5c)
    at ../../binutils-gdb/gdb/value.c:392
#4  0x00156834 in value_check_printable (val=val@entry=0x2635e08, stream=stream@entry=0x24280c8, options=0xbee8e3ec)
    at ../../binutils-gdb/gdb/valprint.c:810
#5  0x00156bbc in common_val_print (val=0x2635e08, stream=0x24280c8, stream@entry=0x4f505f72, recurse=recurse@entry=2, 
    options=0xbee8e3ec, options@entry=0xbee8e3e4, language=language@entry=0x3b979c <c_language_defn>)
    at ../../binutils-gdb/gdb/valprint.c:849
#6  0x00189cbc in print_frame_arg (arg=0x2044575f) at ../../binutils-gdb/gdb/stack.c:286
#7  0x0018aa04 in print_frame_args (func=<optimized out>, frame=0x0, frame@entry=0x22f74a8, num=36664488, num@entry=-1, 
    stream=0xffffffff) at ../../binutils-gdb/gdb/stack.c:674
#8  0x0018b3f0 in print_frame (frame=frame@entry=0x22f74a8, print_level=print_level@entry=4331400, 
    print_what=print_what@entry=SRC_AND_LOC, print_args=print_args@entry=1, sal=...) at ../../binutils-gdb/gdb/stack.c:1205
#9  0x0018b8bc in print_frame_info (frame=0x22f74a8, frame@entry=0x1, print_level=print_level@entry=4331400, 
    print_what=SRC_AND_LOC, print_what@entry=36664488, print_args=print_args@entry=1, set_current_sal=set_current_sal@entry=1)
    at ../../binutils-gdb/gdb/stack.c:857
#10 0x0018baac in print_stack_frame (frame=0x1, print_level=4331400, print_level@entry=0, print_what=36664488, 
    print_what@entry=SRC_AND_LOC, set_current_sal=0, set_current_sal@entry=1) at ../../binutils-gdb/gdb/stack.c:166
#11 0x0017ffe8 in print_stop_event (ws=ws@entry=0xbee8e6b8) at ../../binutils-gdb/gdb/infrun.c:6280
#12 0x001804c8 in normal_stop () at ../../binutils-gdb/gdb/infrun.c:6426
#13 0x00186304 in fetch_inferior_event (client_data=client_data@entry=0x0) at ../../binutils-gdb/gdb/infrun.c:3149
#14 0x0019bc08 in inferior_event_handler (event_type=INF_REG_EVENT, client_data=0x0) at ../../binutils-gdb/gdb/inf-loop.c:58
#15 0x00199dfc in process_event () at ../../binutils-gdb/gdb/event-loop.c:340
#16 0x0019a204 in gdb_do_one_event () at ../../binutils-gdb/gdb/event-loop.c:392
#17 0x0019a400 in start_event_loop () at ../../binutils-gdb/gdb/event-loop.c:429
#18 0x00193ed4 in captured_command_loop (data=<optimized out>) at ../../binutils-gdb/gdb/main.c:323
#19 0x00190ea4 in catch_errors (func=0x22b01d8, func@entry=0x193ebc <captured_command_loop>, 
    func_args=0x417fe8 <cleanup_chain>, func_args@entry=0x0, errstring=0x0, errstring@entry=0x387fc8 "", mask=37532008, 
    mask@entry=RETURN_MASK_ALL) at ../../binutils-gdb/gdb/exceptions.c:237
#20 0x00195168 in captured_main (data=<optimized out>) at ../../binutils-gdb/gdb/main.c:1151
#21 0x00190ea4 in catch_errors (func=0xb6f02800, func@entry=0x194404 <captured_main>, func_args=0x0, 
    func_args@entry=0xbee8e914, errstring=0xbee8e91c "", errstring@entry=0x387fc8 "", mask=4299140, mask@entry=RETURN_MASK_ALL)
    at ../../binutils-gdb/gdb/exceptions.c:237
#22 0x00195658 in gdb_main (args=args@entry=0xbee8e914) at ../../binutils-gdb/gdb/main.c:1159
#23 0x0005f220 in main (argc=<optimized out>, argv=<optimized out>) at ../../binutils-gdb/gdb/gdb.c:32

Latter it hits assert in register_size

#0  0xb6a9d354 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0xb6aa0eb4 in __GI_abort () at abort.c:89
#2  0x0025b480 in dump_core () at ../../binutils-gdb/gdb/utils.c:578
#3  0x0025d448 in internal_vproblem (problem=0xb6bac0ac <lock>, problem@entry=0x419a78 <internal_error_problem>, 
    file=file@entry=0x37358c "../../binutils-gdb/gdb/regcache.c", line=line@entry=177, fmt=fmt@entry=0x0, ap=...)
    at ../../binutils-gdb/gdb/utils.c:786
#4  0x0025d518 in internal_verror (file=file@entry=0x37358c "../../binutils-gdb/gdb/regcache.c", line=line@entry=177, 
    fmt=fmt@entry=0x0, ap=..., ap@entry=...) at ../../binutils-gdb/gdb/utils.c:812
#5  0x0028f150 in internal_error (file=file@entry=0x37358c "../../binutils-gdb/gdb/regcache.c", line=line@entry=177, 
    fmt=0x331904 "%s: Assertion `%s' failed.") at ../../binutils-gdb/gdb/common/errors.c:55
#6  0x00137558 in register_size (gdbarch=gdbarch@entry=0x24043a0, regnum=-1, regnum@entry=37804872)
    at ../../binutils-gdb/gdb/regcache.c:175
#7  0x0020d590 in read_pieced_value (v=0x6c08) at ../../binutils-gdb/gdb/dwarf2loc.c:1690
#8  0x00144d7c in value_fetch_lazy (val=0x2635e08) at ../../binutils-gdb/gdb/value.c:3895
#9  0x001453b0 in value_entirely_covered_by_range_vector (value=0x2635e08, ranges=0x2635e5c)
    at ../../binutils-gdb/gdb/value.c:392
#10 0x00156834 in value_check_printable (val=val@entry=0x2635e08, stream=stream@entry=0x24280c8, options=0xbee8e3ec)
    at ../../binutils-gdb/gdb/valprint.c:810
#11 0x00156bbc in common_val_print (val=0x2635e08, stream=0x24280c8, stream@entry=0x4f505f72, recurse=recurse@entry=2, 
    options=0xbee8e3ec, options@entry=0xbee8e3e4, language=language@entry=0x3b979c <c_language_defn>)
    at ../../binutils-gdb/gdb/valprint.c:849
#12 0x00189cbc in print_frame_arg (arg=0x2044575f) at ../../binutils-gdb/gdb/stack.c:286
#13 0x0018aa04 in print_frame_args (func=<optimized out>, frame=0x0, frame@entry=0x22f74a8, num=36664488, num@entry=-1, 
    stream=0xffffffff) at ../../binutils-gdb/gdb/stack.c:674
#14 0x0018b3f0 in print_frame (frame=frame@entry=0x22f74a8, print_level=print_level@entry=4331400, 
    print_what=print_what@entry=SRC_AND_LOC, print_args=print_args@entry=1, sal=...) at ../../binutils-gdb/gdb/stack.c:1205
#15 0x0018b8bc in print_frame_info (frame=0x22f74a8, frame@entry=0x1, print_level=print_level@entry=4331400, 
    print_what=SRC_AND_LOC, print_what@entry=36664488, print_args=print_args@entry=1, set_current_sal=set_current_sal@entry=1)
    at ../../binutils-gdb/gdb/stack.c:857
#16 0x0018baac in print_stack_frame (frame=0x1, print_level=4331400, print_level@entry=0, print_what=36664488, 
    print_what@entry=SRC_AND_LOC, set_current_sal=0, set_current_sal@entry=1) at ../../binutils-gdb/gdb/stack.c:166
#17 0x0017ffe8 in print_stop_event (ws=ws@entry=0xbee8e6b8) at ../../binutils-gdb/gdb/infrun.c:6280
#18 0x001804c8 in normal_stop () at ../../binutils-gdb/gdb/infrun.c:6426
#19 0x00186304 in fetch_inferior_event (client_data=client_data@entry=0x0) at ../../binutils-gdb/gdb/infrun.c:3149
#20 0x0019bc08 in inferior_event_handler (event_type=INF_REG_EVENT, client_data=0x0) at ../../binutils-gdb/gdb/inf-loop.c:58
#21 0x00199dfc in process_event () at ../../binutils-gdb/gdb/event-loop.c:340
#22 0x0019a204 in gdb_do_one_event () at ../../binutils-gdb/gdb/event-loop.c:392
#23 0x0019a400 in start_event_loop () at ../../binutils-gdb/gdb/event-loop.c:429
#24 0x00193ed4 in captured_command_loop (data=<optimized out>) at ../../binutils-gdb/gdb/main.c:323
#25 0x00190ea4 in catch_errors (func=0x22b01d8, func@entry=0x193ebc <captured_command_loop>, 
    func_args=0x417fe8 <cleanup_chain>, func_args@entry=0x0, errstring=0x0, errstring@entry=0x387fc8 "", mask=37532008, 
    mask@entry=RETURN_MASK_ALL) at ../../binutils-gdb/gdb/exceptions.c:237
#26 0x00195168 in captured_main (data=<optimized out>) at ../../binutils-gdb/gdb/main.c:1151
#27 0x00190ea4 in catch_errors (func=0xb6f02800, func@entry=0x194404 <captured_main>, func_args=0x0, 
    func_args@entry=0xbee8e914, errstring=0xbee8e91c "", errstring@entry=0x387fc8 "", mask=4299140, mask@entry=RETURN_MASK_ALL)
    at ../../binutils-gdb/gdb/exceptions.c:237

LE code receives gdb_regnum = -1 as well but LE code does not hit 
this because it does not have the following snippet in read_pieced_value


	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
		&& this_size < register_size (arch, gdb_regnum))
	      {
		/* Big-endian, and we want less than full size.  */
		reg_offset = register_size (arch, gdb_regnum) - this_size;
		/* We want the lower-order THIS_SIZE_BITS of the bytes
		   we extract from the register.  */
		source_offset_bits += 8 * this_size - this_size_bits;
	      }

and it proceeds with error:

(gdb) n
1699		    if (gdb_regnum != -1)
(gdb) n
1718			error (_("Unable to access DWARF register number %s"),
(gdb) bt
#0  read_pieced_value (v=0xb6c967c0 <main_arena>) at ../../binutils-gdb/gdb/dwarf2loc.c:1718
#1  0x00144a50 in value_fetch_lazy (val=0x2625020) at ../../binutils-gdb/gdb/value.c:3895
#2  0x00145084 in value_entirely_covered_by_range_vector (value=0x2625020, ranges=0x2625074) at ../../binutils-gdb/gdb/value.c:392
#3  0x00156580 in value_check_printable (val=val@entry=0x2625020, stream=stream@entry=0x2439da8, options=0xbebb85cc) at ../../binutils-gdb/gdb/valprint.c:810
#4  0x00156908 in common_val_print (val=0x2625020, stream=0x2439da8, stream@entry=0x725f504f, recurse=recurse@entry=2, options=0xbebb85cc, options@entry=0xbebb85c4, 
    language=language@entry=0x3b8f60 <c_language_defn>) at ../../binutils-gdb/gdb/valprint.c:849
#5  0x001899c8 in print_frame_arg (arg=0x5f574420) at ../../binutils-gdb/gdb/stack.c:286
#6  0x0018a710 in print_frame_args (func=<optimized out>, frame=0x0, frame@entry=0x2260438, num=36045880, num@entry=-1, stream=0xffffffff) at ../../binutils-gdb/gdb/stack.c:674
#7  0x0018b0fc in print_frame (frame=frame@entry=0x2260438, print_level=print_level@entry=4331144, print_what=print_what@entry=SRC_AND_LOC, print_args=print_args@entry=1, sal=...)
    at ../../binutils-gdb/gdb/stack.c:1205
#8  0x0018b5c8 in print_frame_info (frame=0x2260438, frame@entry=0x1, print_level=print_level@entry=4331144, print_what=SRC_AND_LOC, print_what@entry=36045880, print_args=print_args@entry=1, 
    set_current_sal=set_current_sal@entry=1) at ../../binutils-gdb/gdb/stack.c:857
#9  0x0018b7b8 in print_stack_frame (frame=0x1, print_level=4331144, print_level@entry=0, print_what=36045880, print_what@entry=SRC_AND_LOC, set_current_sal=0, set_current_sal@entry=1)
    at ../../binutils-gdb/gdb/stack.c:166
#10 0x0017fd00 in print_stop_event (ws=ws@entry=0xbebb8898) at ../../binutils-gdb/gdb/infrun.c:6280
#11 0x001801e0 in normal_stop () at ../../binutils-gdb/gdb/infrun.c:6426
#12 0x00186010 in fetch_inferior_event (client_data=client_data@entry=0x0) at ../../binutils-gdb/gdb/infrun.c:3149
#13 0x0019b8fc in inferior_event_handler (event_type=INF_REG_EVENT, client_data=0x0) at ../../binutils-gdb/gdb/inf-loop.c:58
#14 0x00199af0 in process_event () at ../../binutils-gdb/gdb/event-loop.c:340
#15 0x00199ef8 in gdb_do_one_event () at ../../binutils-gdb/gdb/event-loop.c:392
#16 0x0019a0f4 in start_event_loop () at ../../binutils-gdb/gdb/event-loop.c:429
#17 0x00193bcc in captured_command_loop (data=<optimized out>) at ../../binutils-gdb/gdb/main.c:323
#18 0x00190bac in catch_errors (func=0x22191d8, func@entry=0x193bb4 <captured_command_loop>, func_args=0x417ee8 <cleanup_chain>, func_args@entry=0x0, errstring=0x0, 
    errstring@entry=0x3877e0 "", mask=37746520, mask@entry=RETURN_MASK_ALL) at ../../binutils-gdb/gdb/exceptions.c:237
#19 0x00194e60 in captured_main (data=<optimized out>) at ../../binutils-gdb/gdb/main.c:1151
#20 0x00190bac in catch_errors (func=0xb6fec2a0, func@entry=0x1940fc <captured_main>, func_args=0x0, func_args@entry=0xbebb8af4, errstring=0xbebb8afc "\005", errstring@entry=0x3877e0 "", 
    mask=4298884, mask@entry=RETURN_MASK_ALL) at ../../binutils-gdb/gdb/exceptions.c:237
#21 0x00195350 in gdb_main (args=args@entry=0xbebb8af4) at ../../binutils-gdb/gdb/main.c:1159
#22 0x0005f218 in main (argc=<optimized out>, argv=<optimized out>) at ../../binutils-gdb/gdb/gdb.c:32

Re: [PATCH 4/5] ARM: read_pieced_value do big endian processing only in case of valid gdb_regnum

[Yao Qi]

Victor Kamensky <victor.kamensky@linaro.org> writes:

> In both little endian and big endian cases compiler generate DW_OP_reg29-
> DW_OP_reg31 something like this.
>
>  <2><792>: Abbrev Number: 10 (DW_TAG_formal_parameter)
>     <793>   DW_AT_name        : u
>     <795>   DW_AT_decl_file   : 1
>     <796>   DW_AT_decl_line   : 115
>     <797>   DW_AT_type        : <0x57c>
>     <79b>   DW_AT_location    : 6 byte block: 6d 93 4 6c 93 4
> (DW_OP_reg29 (r29); DW_OP_piece: 4; DW_OP_reg28 (r28); DW_OP_piece: 4)
>

This is quite illustrative.

> I strongly suspect that it is compiler error, but more accurately
> it is hard to say, because I never saw a document where for given CPU
> mapping from registers to DWARF reg numbers is defined. Have you
> seen such document for example for ARM V7? In any case for this
> test case Gdb believes that those register numbers are wrong. I.e we
> can say for sure that gcc and gdb are disagrees.

You need doc "DWARF for the ARM Architecture", which has a table about
the mapping between dwarf reg numbers and processor registers. For the
table, we can see that dwarf register 16 to 63 doesn't map to any
processor registers.

>
>
> (gdb) file /wd1/gdb/20140930/build-v7le/gdb/testsuite/gdb.base/store
> Reading symbols from /wd1/gdb/20140930/build-v7le/gdb/testsuite/gdb.base/store...done.
> (gdb) tbreak wack_double
> Temporary breakpoint 1 at 0x1076c: file ../../../binutils-gdb/gdb/testsuite/gdb.base/store.c, line 117.
> (gdb) run
> Starting program: /wd1/gdb/20140930/build-v7le/gdb/testsuite/gdb.base/store 
>
> Temporary breakpoint 1, wack_double (u=
> ../../binutils-gdb/gdb/regcache.c:177: internal-error: register_size: Assertion `regnum >= 0 && regnum < (gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch))' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
>

This is quite useful too.

>
> BE Dump
> =======
>
>
>  <1><779>: Abbrev Number: 12 (DW_TAG_subprogram)
>     <77a>   DW_AT_external    : 1       
>     <77a>   DW_AT_name        : (indirect string, offset: 0x3c9): wack_double   
>     <77e>   DW_AT_decl_file   : 1       
>     <77f>   DW_AT_decl_line   : 115     
>     <780>   DW_AT_prototyped  : 1       
>     <780>   DW_AT_type        : <0x57c> 
>     <784>   DW_AT_low_pc      : 0x10758 
>     <788>   DW_AT_high_pc     : 0x40    
>     <78c>   DW_AT_frame_base  : 1 byte block: 9c        (DW_OP_call_frame_cfa)
>     <78e>   DW_AT_GNU_all_tail_call_sites: 1    
>     <78e>   DW_AT_sibling     : <0x7d7> 
>  <2><792>: Abbrev Number: 10 (DW_TAG_formal_parameter)
>     <793>   DW_AT_name        : u       
>     <795>   DW_AT_decl_file   : 1       
>     <796>   DW_AT_decl_line   : 115     
>     <797>   DW_AT_type        : <0x57c> 
>     <79b>   DW_AT_location    : 6 byte block: 6d 93 4 6c 93 4   (DW_OP_reg29 (r29); DW_OP_piece: 4; DW_OP_reg28 (r28); DW_OP_piece: 4)
>  <2><7a2>: Abbrev Number: 10 (DW_TAG_formal_parameter)
>     <7a3>   DW_AT_name        : v       
>     <7a5>   DW_AT_decl_file   : 1       
>     <7a6>   DW_AT_decl_line   : 115     
>     <7a7>   DW_AT_type        : <0x57c> 
>     <7ab>   DW_AT_location    : 6 byte block: 6f 93 4 6e 93 4   (DW_OP_reg31 (r31); DW_OP_piece: 4; DW_OP_reg30 (r30); DW_OP_piece: 4)
>  <2><7b2>: Abbrev Number: 13 (DW_TAG_variable)
>     <7b3>   DW_AT_name        : l       
>     <7b5>   DW_AT_decl_file   : 1       
>     <7b6>   DW_AT_decl_line   : 117     
>     <7b7>   DW_AT_type        : <0x57c> 
>     <7bb>   DW_AT_location    : 8 byte block: 90 21 93 4 90 20 93 4     (DW_OP_regx: 33 (r33); DW_OP_piece: 4; DW_OP_regx: 32 (r32); DW_OP_piece: 4)
>  <2><7c4>: Abbrev Number: 13 (DW_TAG_variable)
>     <7c5>   DW_AT_name        : r       
>     <7c7>   DW_AT_decl_file   : 1       
>     <7c8>   DW_AT_decl_line   : 117     
>     <7c9>   DW_AT_type        : <0x57c> 
>     <7cd>   DW_AT_location    : 8 byte block: 90 23 93 4 90 22 93 4     (DW_OP_regx: 35 (r35); DW_OP_piece: 4; DW_OP_regx: 34 (r34); DW_OP_piece: 4)
>

However, we don't need to copy the whole DIE here, instead, we can only
copy one DW_TAG_formal_parameter, which is should be illustrative enough
for the problem.

> Backtrace when it failed to get reg number
> ==========================================

We don't need to copy the full stack back trace here.

-- 
Yao (齐尧)

Re: [PATCH 4/5] ARM: read_pieced_value do big endian processing only in case of valid gdb_regnum

[Victor Kamensky]

Hi Yao,

I've posted updated V3 version only for this patch. I've modified
commit message to include more details as you suggested.
And I moved reg_offset var to more specific blocks as you noted.
Please take a look.

Would you like me to repost the whole series again (all 4
patches) or it would be OK just like this?

Also it came up on binutils@ patch discussion with Alan -
I do not have git commit permission. Is it my correct
expectation once folks are OK with the patches, you or
some other gdb maintainer will commit those?

Thanks,
Victor


On 22 October 2014 20:18, Yao Qi <yao@codesourcery.com> wrote:
> Victor Kamensky <victor.kamensky@linaro.org> writes:
>
>> In both little endian and big endian cases compiler generate DW_OP_reg29-
>> DW_OP_reg31 something like this.
>>
>>  <2><792>: Abbrev Number: 10 (DW_TAG_formal_parameter)
>>     <793>   DW_AT_name        : u
>>     <795>   DW_AT_decl_file   : 1
>>     <796>   DW_AT_decl_line   : 115
>>     <797>   DW_AT_type        : <0x57c>
>>     <79b>   DW_AT_location    : 6 byte block: 6d 93 4 6c 93 4
>> (DW_OP_reg29 (r29); DW_OP_piece: 4; DW_OP_reg28 (r28); DW_OP_piece: 4)
>>
>
> This is quite illustrative.
>
>> I strongly suspect that it is compiler error, but more accurately
>> it is hard to say, because I never saw a document where for given CPU
>> mapping from registers to DWARF reg numbers is defined. Have you
>> seen such document for example for ARM V7? In any case for this
>> test case Gdb believes that those register numbers are wrong. I.e we
>> can say for sure that gcc and gdb are disagrees.
>
> You need doc "DWARF for the ARM Architecture", which has a table about
> the mapping between dwarf reg numbers and processor registers. For the
> table, we can see that dwarf register 16 to 63 doesn't map to any
> processor registers.
>
>>
>>
>> (gdb) file /wd1/gdb/20140930/build-v7le/gdb/testsuite/gdb.base/store
>> Reading symbols from /wd1/gdb/20140930/build-v7le/gdb/testsuite/gdb.base/store...done.
>> (gdb) tbreak wack_double
>> Temporary breakpoint 1 at 0x1076c: file ../../../binutils-gdb/gdb/testsuite/gdb.base/store.c, line 117.
>> (gdb) run
>> Starting program: /wd1/gdb/20140930/build-v7le/gdb/testsuite/gdb.base/store
>>
>> Temporary breakpoint 1, wack_double (u=
>> ../../binutils-gdb/gdb/regcache.c:177: internal-error: register_size: Assertion `regnum >= 0 && regnum < (gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch))' failed.
>> A problem internal to GDB has been detected,
>> further debugging may prove unreliable.
>>
>
> This is quite useful too.
>
>>
>> BE Dump
>> =======
>>
>>
>>  <1><779>: Abbrev Number: 12 (DW_TAG_subprogram)
>>     <77a>   DW_AT_external    : 1
>>     <77a>   DW_AT_name        : (indirect string, offset: 0x3c9): wack_double
>>     <77e>   DW_AT_decl_file   : 1
>>     <77f>   DW_AT_decl_line   : 115
>>     <780>   DW_AT_prototyped  : 1
>>     <780>   DW_AT_type        : <0x57c>
>>     <784>   DW_AT_low_pc      : 0x10758
>>     <788>   DW_AT_high_pc     : 0x40
>>     <78c>   DW_AT_frame_base  : 1 byte block: 9c        (DW_OP_call_frame_cfa)
>>     <78e>   DW_AT_GNU_all_tail_call_sites: 1
>>     <78e>   DW_AT_sibling     : <0x7d7>
>>  <2><792>: Abbrev Number: 10 (DW_TAG_formal_parameter)
>>     <793>   DW_AT_name        : u
>>     <795>   DW_AT_decl_file   : 1
>>     <796>   DW_AT_decl_line   : 115
>>     <797>   DW_AT_type        : <0x57c>
>>     <79b>   DW_AT_location    : 6 byte block: 6d 93 4 6c 93 4   (DW_OP_reg29 (r29); DW_OP_piece: 4; DW_OP_reg28 (r28); DW_OP_piece: 4)
>>  <2><7a2>: Abbrev Number: 10 (DW_TAG_formal_parameter)
>>     <7a3>   DW_AT_name        : v
>>     <7a5>   DW_AT_decl_file   : 1
>>     <7a6>   DW_AT_decl_line   : 115
>>     <7a7>   DW_AT_type        : <0x57c>
>>     <7ab>   DW_AT_location    : 6 byte block: 6f 93 4 6e 93 4   (DW_OP_reg31 (r31); DW_OP_piece: 4; DW_OP_reg30 (r30); DW_OP_piece: 4)
>>  <2><7b2>: Abbrev Number: 13 (DW_TAG_variable)
>>     <7b3>   DW_AT_name        : l
>>     <7b5>   DW_AT_decl_file   : 1
>>     <7b6>   DW_AT_decl_line   : 117
>>     <7b7>   DW_AT_type        : <0x57c>
>>     <7bb>   DW_AT_location    : 8 byte block: 90 21 93 4 90 20 93 4     (DW_OP_regx: 33 (r33); DW_OP_piece: 4; DW_OP_regx: 32 (r32); DW_OP_piece: 4)
>>  <2><7c4>: Abbrev Number: 13 (DW_TAG_variable)
>>     <7c5>   DW_AT_name        : r
>>     <7c7>   DW_AT_decl_file   : 1
>>     <7c8>   DW_AT_decl_line   : 117
>>     <7c9>   DW_AT_type        : <0x57c>
>>     <7cd>   DW_AT_location    : 8 byte block: 90 23 93 4 90 22 93 4     (DW_OP_regx: 35 (r35); DW_OP_piece: 4; DW_OP_regx: 34 (r34); DW_OP_piece: 4)
>>
>
> However, we don't need to copy the whole DIE here, instead, we can only
> copy one DW_TAG_formal_parameter, which is should be illustrative enough
> for the problem.
>
>> Backtrace when it failed to get reg number
>> ==========================================
>
> We don't need to copy the full stack back trace here.
>
> --
> Yao (齐尧)

Re: [PATCH 4/5] ARM: read_pieced_value do big endian processing only in case of valid gdb_regnum

[Yao Qi]

Victor Kamensky <victor.kamensky@linaro.org> writes:

> I've posted updated V3 version only for this patch. I've modified
> commit message to include more details as you suggested.
> And I moved reg_offset var to more specific blocks as you noted.
> Please take a look.

Thanks, I'll take a look.

> Would you like me to repost the whole series again (all 4
> patches) or it would be OK just like this?

Well, we have two options usually, supposing you post a patch series,

 - if you only update one or two patches after review, you can
   just reply to reviewer's mail and post the updated patches in the same
   mail thread as your original post.  It'll be convenient to search in
   archive, because all of them are in the same thread.

 - if you update all the patches, for example, change the design, better
   to post the updated series, like what you did for V2.

In this case, you don't have to post them again.
>
> Also it came up on binutils@ patch discussion with Alan -
> I do not have git commit permission. Is it my correct
> expectation once folks are OK with the patches, you or
> some other gdb maintainer will commit those?

It is better for you to create your own account, and commit your patches
after approval in the future.

Please fill in the form here https://sourceware.org/cgi-bin/pdw/ps_form.cgi
I think I can approve your request, so please fill in my mail address in
box "email address of person who approved request".

Once your account is ready, you can commit approved patches first.  I am
still thinking about patch 4/4, it may take some time.

-- 
Yao (齐尧)

[PATCH 2/5] ARM: extract_arm_insn function need to read instrs correctly in be8 case

[Victor Kamensky]

extract_arm_insn function needs to read instructions in
gdbarch_byte_order_for_code byte order, because in case armv7b,
even data is big endian, instructions are still little endian.
Currently function uses gdbarch_byte_order which would be
big endian in armv7b case.

Because of this issue pretty much all gdb.reverse/ tests are
failing with 'Process record does not support instruction' message.

Fix is to change gdbarch_byte_order to gdbarch_byte_order_for_code,
when passed to extract_unsigned_integer that reads instruction.
---
 gdb/ChangeLog  | 5 +++++
 gdb/arm-tdep.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c967a93..2aef5dc 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
+
+	* arm-tdep.c (extract_arm_insn): use dbarch_byte_order_for_code
+	to read arm instruction.
+
 2014-09-30  Don Breazeal  <donb@codesourcery.com>
 
 	* inf-ptrace.c (inf_ptrace_follow_fork): Remove target-independent
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index e2559ec..e7a1ec5 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -13860,7 +13860,7 @@ extract_arm_insn (insn_decode_record *insn_record, uint32_t insn_size)
     return 1;
   insn_record->arm_insn = (uint32_t) extract_unsigned_integer (&buf[0],
                            insn_size, 
-                           gdbarch_byte_order (insn_record->gdbarch));
+			   gdbarch_byte_order_for_code (insn_record->gdbarch));
   return 0;
 }
 
-- 
1.8.1.4

Re: [PATCH 2/5] ARM: extract_arm_insn function need to read instrs correctly in be8 case

[Yao Qi]

Victor Kamensky <victor.kamensky@linaro.org> writes:

> Fix is to change gdbarch_byte_order to gdbarch_byte_order_for_code,
> when passed to extract_unsigned_integer that reads instruction.
> ---
>  gdb/ChangeLog  | 5 +++++
>  gdb/arm-tdep.c | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index c967a93..2aef5dc 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
> +
> +	* arm-tdep.c (extract_arm_insn): use dbarch_byte_order_for_code
> +	to read arm instruction.
> +
>  2014-09-30  Don Breazeal  <donb@codesourcery.com>
>  
>  	* inf-ptrace.c (inf_ptrace_follow_fork): Remove target-independent

Looks good to me.

We don't include the ChangeLog changes in the patch, because that will
cause conflicts when applying your patch locally in the review.
Instead, we include ChangeLog entries in the commit messages, see
https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_formatted_commit_messages

-- 
Yao (齐尧)

Re: [PATCH 2/5] ARM: extract_arm_insn function need to read instrs correctly in be8 case

[Yao Qi]

Yao Qi <yao@codesourcery.com> writes:

>> +2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
>> +
>> +	* arm-tdep.c (extract_arm_insn): use dbarch_byte_order_for_code

s/use dbarch/Use gdbarch/

>> +	to read arm instruction.

-- 
Yao (齐尧)

Re: [PATCH 2/5] ARM: extract_arm_insn function need to read instrs correctly in be8 case

[Victor Kamensky]

On 21 October 2014 00:54, Yao Qi <yao@codesourcery.com> wrote:
> Victor Kamensky <victor.kamensky@linaro.org> writes:
>
>> Fix is to change gdbarch_byte_order to gdbarch_byte_order_for_code,
>> when passed to extract_unsigned_integer that reads instruction.
>> ---
>>  gdb/ChangeLog  | 5 +++++
>>  gdb/arm-tdep.c | 2 +-
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index c967a93..2aef5dc 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
>> +
>> +     * arm-tdep.c (extract_arm_insn): use dbarch_byte_order_for_code
>> +     to read arm instruction.
>> +
>>  2014-09-30  Don Breazeal  <donb@codesourcery.com>
>>
>>       * inf-ptrace.c (inf_ptrace_follow_fork): Remove target-independent
>
> Looks good to me.
>
> We don't include the ChangeLog changes in the patch, because that will
> cause conflicts when applying your patch locally in the review.
> Instead, we include ChangeLog entries in the commit messages, see
> https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_formatted_commit_messages

Thanks! It is good pointer. I have not seen it before.
Maybe gdb/CONTRIBUTE could mention this wiki page.

I will move all proposed commit ChangeLogs as per wiki,
will incorporate review comments and repost updated series.

Thanks,
Victor

> --
> Yao (齐尧)

gdb/CONTRIBUTE

[Pedro Alves]

Hi Victor,

On 10/21/2014 03:44 PM, Victor Kamensky wrote:
>> >
>> > We don't include the ChangeLog changes in the patch, because that will
>> > cause conflicts when applying your patch locally in the review.
>> > Instead, we include ChangeLog entries in the commit messages, see
>> > https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_formatted_commit_messages
> Thanks! It is good pointer. I have not seen it before.
> Maybe gdb/CONTRIBUTE could mention this wiki page.

OOC, how did you find gdb/CONTRIBUTE?  Was that through
https://sourceware.org/gdb/contribute/ or some other means?

IMO we should just move/merge the gross of the contents in the
file to the web/wiki, and either delete the file, or leave just
the preamble.

If it stays, I think it should link to:

  http://www.gnu.org/software/gdb/contribute/

as that's more stable than the wiki, and then that page should
link to relevant wiki pages.

Thanks,
Pedro Alves

Re: gdb/CONTRIBUTE

[Victor Kamensky]

On 24 October 2014 05:20, Pedro Alves <palves@redhat.com> wrote:
> Hi Victor,
>
> On 10/21/2014 03:44 PM, Victor Kamensky wrote:
>>> >
>>> > We don't include the ChangeLog changes in the patch, because that will
>>> > cause conflicts when applying your patch locally in the review.
>>> > Instead, we include ChangeLog entries in the commit messages, see
>>> > https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_formatted_commit_messages
>> Thanks! It is good pointer. I have not seen it before.
>> Maybe gdb/CONTRIBUTE could mention this wiki page.
>
> OOC, how did you find gdb/CONTRIBUTE?  Was that through
> https://sourceware.org/gdb/contribute/ or some other means?

Yes, through above link. I got to it from
https://sourceware.org/gdb/
following 'contributing' (3rd) tab on top under title.

Thanks,
Victor

> IMO we should just move/merge the gross of the contents in the
> file to the web/wiki, and either delete the file, or leave just
> the preamble.
>
> If it stays, I think it should link to:
>
>   http://www.gnu.org/software/gdb/contribute/
>
> as that's more stable than the wiki, and then that page should
> link to relevant wiki pages.
>
> Thanks,
> Pedro Alves
>

Re: [PATCH 0/5] arm: set of big endian related fixes for armeb (v7)

[Andrew Pinski]

On Mon, Oct 20, 2014 at 5:56 PM, Victor Kamensky
<victor.kamensky@linaro.org> wrote:
> Hi Folks,
>
> Please find five patches following this cover letter that address several
> big endian related issues for ARM V7 target. Problems were discovered by
> running gdb/testsuite on armeb (big endian) target rootfs/kernel and
> comparing results from the same source against arm (little endian).
>
> Note there are several failures remain that are not addressed by this
> series. I.e valgrind related test cases all failed, because valgrind does
> not support armeb target. There are other failures in a bit obscure
> places like gdb.dwarf2/implptrpiece.exp, gdb.python/py-value-cc.exp,
> but over all number of passes/failures significantly improved in armeb
> case.
>
> Please note it is my first attempt to contribute into gdb. If I missed
> something please guide me how I can correct that.

Yes bfd patches need to be also sent to binutils@.

Thanks,
Andrew Pinski


>
> Thanks,
> Victor
>
> Victor Kamensky (5):
>   ARM: plt_size functions need to read instructions in right byte order
>   ARM: extract_arm_insn function need to read instrs correctly in be8
>     case
>   ARM: arm_breakpoint should be little endian form in case for arm BE8
>   ARM: read_pieced_value do big endian processing only in case of valid
>     gdb_regnum
>   ARM: asm-source.exp link options in case of armv7b target
>
>  bfd/ChangeLog                        |  9 +++++++
>  bfd/elf32-arm.c                      | 48 +++++++++++++++++++++++++++++++++---
>  gdb/ChangeLog                        | 16 ++++++++++++
>  gdb/arm-linux-tdep.c                 |  2 +-
>  gdb/arm-tdep.c                       |  2 +-
>  gdb/dwarf2loc.c                      | 30 +++++++++++-----------
>  gdb/testsuite/ChangeLog              |  4 +++
>  gdb/testsuite/gdb.asm/asm-source.exp |  4 +++
>  8 files changed, 94 insertions(+), 21 deletions(-)
>
> --
> 1.8.1.4
>

Re: [PATCH 0/5] arm: set of big endian related fixes for armeb (v7)

[Victor Kamensky]

On 20 October 2014 18:12, Andrew Pinski <pinskia@gmail.com> wrote:
> On Mon, Oct 20, 2014 at 5:56 PM, Victor Kamensky
> <victor.kamensky@linaro.org> wrote:
>> Hi Folks,
>>
>> Please find five patches following this cover letter that address several
>> big endian related issues for ARM V7 target. Problems were discovered by
>> running gdb/testsuite on armeb (big endian) target rootfs/kernel and
>> comparing results from the same source against arm (little endian).
>>
>> Note there are several failures remain that are not addressed by this
>> series. I.e valgrind related test cases all failed, because valgrind does
>> not support armeb target. There are other failures in a bit obscure
>> places like gdb.dwarf2/implptrpiece.exp, gdb.python/py-value-cc.exp,
>> but over all number of passes/failures significantly improved in armeb
>> case.
>>
>> Please note it is my first attempt to contribute into gdb. If I missed
>> something please guide me how I can correct that.
>
> Yes bfd patches need to be also sent to binutils@.

Thanks, Andrew. I posted first patch of this series on suggested binutils
mailing list [1]. Other four patches in gdb directory.

- Victor

[1] https://sourceware.org/ml/binutils/2014-10/msg00175.html

> Thanks,
> Andrew Pinski
>
>
>>
>> Thanks,
>> Victor
>>
>> Victor Kamensky (5):
>>   ARM: plt_size functions need to read instructions in right byte order
>>   ARM: extract_arm_insn function need to read instrs correctly in be8
>>     case
>>   ARM: arm_breakpoint should be little endian form in case for arm BE8
>>   ARM: read_pieced_value do big endian processing only in case of valid
>>     gdb_regnum
>>   ARM: asm-source.exp link options in case of armv7b target
>>
>>  bfd/ChangeLog                        |  9 +++++++
>>  bfd/elf32-arm.c                      | 48 +++++++++++++++++++++++++++++++++---
>>  gdb/ChangeLog                        | 16 ++++++++++++
>>  gdb/arm-linux-tdep.c                 |  2 +-
>>  gdb/arm-tdep.c                       |  2 +-
>>  gdb/dwarf2loc.c                      | 30 +++++++++++-----------
>>  gdb/testsuite/ChangeLog              |  4 +++
>>  gdb/testsuite/gdb.asm/asm-source.exp |  4 +++
>>  8 files changed, 94 insertions(+), 21 deletions(-)
>>
>> --
>> 1.8.1.4
>>

Re: [PATCH 0/5] arm: set of big endian related fixes for armeb (v7)

[Yao Qi]

Victor Kamensky <victor.kamensky@linaro.org> writes:

> Note there are several failures remain that are not addressed by this 
> series. I.e valgrind related test cases all failed, because valgrind does 
> not support armeb target. There are other failures in a bit obscure
> places like gdb.dwarf2/implptrpiece.exp, gdb.python/py-value-cc.exp,
> but over all number of passes/failures significantly improved in armeb
> case. 

Out of my curiosity, do you plan to address the rest of fails you
mentioned above?

-- 
Yao (齐尧)

Re: [PATCH 0/5] arm: set of big endian related fixes for armeb (v7)

[Victor Kamensky]

Hi Yao,

Thank you for your help and attention! Please see response inline.

On 21 October 2014 00:35, Yao Qi <yao@codesourcery.com> wrote:
> Victor Kamensky <victor.kamensky@linaro.org> writes:
>
>> Note there are several failures remain that are not addressed by this
>> series. I.e valgrind related test cases all failed, because valgrind does
>> not support armeb target. There are other failures in a bit obscure
>> places like gdb.dwarf2/implptrpiece.exp, gdb.python/py-value-cc.exp,
>> but over all number of passes/failures significantly improved in armeb
>> case.
>
> Out of my curiosity, do you plan to address the rest of fails you
> mentioned above?

Not sure yet. Maybe on the second pass. Currently I am working
on aarch64 BE vs LE gdb testsuite result differences. There are few
issues in BE aarch64 case that affect gdb functionality visible to gdb
user.

I believe that remaining differences in ARM V7 BE test results most
likely are issues in test cases. And I am not sure whether it is more
general BE vs LE issues. I would probably give it a spin on BE ppc
and see how tests in question behave there.

Problems related to valgrind will not be addressed any time soon. It
seems it is huge thing to make valgrind to work in ARM V7 BE case.

Thanks,
Victor

> --
> Yao (齐尧)

Re: [PATCH 0/5] arm: set of big endian related fixes for armeb (v7)

[Yao Qi]

Victor Kamensky <victor.kamensky@linaro.org> writes:

> Problems related to valgrind will not be addressed any time soon. It
> seems it is huge thing to make valgrind to work in ARM V7 BE case.

We can skip these tests requiring valgrind if valgrind isn't supported on
that target.

-- 
Yao (齐尧)