Clear *VAL in regcache_raw_read_unsigned

Message ID 1455029644-6197-1-git-send-email-yao.qi@linaro.org
State New, archived
Headers

Commit Message

Yao Qi Feb. 9, 2016, 2:54 p.m. UTC
  We have function regcache_raw_read_unsigned defined in both GDB and
GDBserver, so that it is used in common like this,

  ULONGEST value;
  status = regcache_raw_read_unsigned (regcache, regnum, &value);

'value' is correctly set in GDB side, but may not be correctly set
in GDBserver, because &value is passed in regcache_raw_read_unsigned
but collect_register may only set part of the whole variable.  In my
test, I see the top half of 'value' is garbage.  This patch fixes this
problem by clearing *VAL before calling collect_register.

Regression tests are still running.  I'll push it in if there is no
regression in tests.

gdb/gdbserver:

2016-02-09  Yao Qi  <yao.qi@linaro.org>

	* regcache.c (regcache_raw_read_unsigned): Clear *VAL.
---
 gdb/gdbserver/regcache.c | 1 +
 1 file changed, 1 insertion(+)
  

Comments

Yao Qi Feb. 10, 2016, 4:45 p.m. UTC | #1
Yao Qi <qiyaoltc@gmail.com> writes:

> Regression tests are still running.  I'll push it in if there is no
> regression in tests.
>
> gdb/gdbserver:
>
> 2016-02-09  Yao Qi  <yao.qi@linaro.org>
>
> 	* regcache.c (regcache_raw_read_unsigned): Clear *VAL.

Regression test on arm-linux is done.  I push it in to both master and
7.11 branch.  Note that this function is only used for software single
step on arm-linux, so I run tests for arm-linux target.
  
Pedro Alves Feb. 10, 2016, 4:52 p.m. UTC | #2
On 02/10/2016 04:45 PM, Yao Qi wrote:
> Yao Qi <qiyaoltc@gmail.com> writes:
> 
>> Regression tests are still running.  I'll push it in if there is no
>> regression in tests.
>>
>> gdb/gdbserver:
>>
>> 2016-02-09  Yao Qi  <yao.qi@linaro.org>
>>
>> 	* regcache.c (regcache_raw_read_unsigned): Clear *VAL.
> 
> Regression test on arm-linux is done.  I push it in to both master and
> 7.11 branch.  Note that this function is only used for software single
> step on arm-linux, so I run tests for arm-linux target.

Isn't this broken on big endian?  AFAICS, we're reading 32-bits into
the higher 32-bits of a 64-bit variable.

Thanks,
Pedro Alves
  
Yao Qi Feb. 10, 2016, 5:25 p.m. UTC | #3
Pedro Alves <palves@redhat.com> writes:

Hi Pedro,

> Isn't this broken on big endian?  AFAICS, we're reading 32-bits into
> the higher 32-bits of a 64-bit variable.

What do you mean by "this"?  IIUC, "this" means
regcache_raw_read_unsigned, rather than my patch.  My patch just clears
*VAL before passing it to collect_register, so it shouldn't break anything
(I hope) on big endian.
  
Pedro Alves Feb. 10, 2016, 5:36 p.m. UTC | #4
On 02/10/2016 05:25 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
> Hi Pedro,
> 
>> Isn't this broken on big endian?  AFAICS, we're reading 32-bits into
>> the higher 32-bits of a 64-bit variable.
> 
> What do you mean by "this"?  IIUC, "this" means
> regcache_raw_read_unsigned, rather than my patch.  My patch just clears
> *VAL before passing it to collect_register, so it shouldn't break anything
> (I hope) on big endian.

The issue you noticed exposed that regcache_raw_read_unsigned function
is broken for memcpy'ing a 32-bit value into a 64-bit variable, and I think
your patch papered over the problem for little endian, only.

enum register_status
regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
			    ULONGEST *val)
{
  int size;

  gdb_assert (regcache != NULL);
  gdb_assert (regnum >= 0 && regnum < regcache->tdesc->num_registers);

  size = register_size (regcache->tdesc, regnum);

  if (size > (int) sizeof (ULONGEST))
    error (_("That operation is not available on integers of more than"
            "%d bytes."),
          (int) sizeof (ULONGEST));

  collect_register (regcache, regnum, val);

  return REG_VALID;
}

VAL is 64-bit.  collect_register () writes directly into VAL, but it
only writes 32-bits.  On little endian, that will leave the upper halve
of VAL as garbage.  So your patch clears that.  But on big endian,
that collect_register() call writes into the upper halve of VAL, and
the result is that VAL now has the wrong value.

E.g., if the 32-bit register's value is supposed to be 0x11223344,
after the collect register call, *VAL ends up with 0x1122334400000000,
which happens to work for little endian, but not for big endian.

You should be able to trigger this on an ARM system with big endian code.

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index 6a737ea..2af8e24 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -440,6 +440,7 @@  regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
             "%d bytes."),
           (int) sizeof (ULONGEST));
 
+  *val = 0;
   collect_register (regcache, regnum, val);
 
   return REG_VALID;