[patch] bfin: swapped args in bfin_extract_return_value?

[patch] bfin: swapped args in bfin_extract_return_value?

[Matt Rice]

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

I don't really know this code, so this patch might be wrong,
and don't have a setup to test it.

but from the types, and the variable names, it smells of arguments
that have been swapped.

Thanks.

2011-07-11  Matt Rice  <ratmice@gmail.com>

        * bfin-tdep.c (bfin_extract_return_value): Fix swapped
        arguments to store_unsigned_integer.

[-- Attachment #2: foo.diff --]
[-- Type: application/octet-stream, Size: 473 bytes --]

diff --git a/gdb/bfin-tdep.c b/gdb/bfin-tdep.c
index 8322af5..5522c5b 100644
--- a/gdb/bfin-tdep.c
+++ b/gdb/bfin-tdep.c
@@ -623,7 +623,7 @@ bfin_extract_return_value (struct type *type,
   while (len > 0)
     {
       regcache_cooked_read_unsigned (regs, regno++, &tmp);
-      store_unsigned_integer (valbuf, (len > 4 ? 4 : len), tmp, byte_order);
+      store_unsigned_integer (valbuf, (len > 4 ? 4 : len), byte_order, tmp);
       len -= 4;
       valbuf += 4;
     }

Re: [patch] bfin: swapped args in bfin_extract_return_value?

[Tom Tromey]

>>>>> "Matt" == Matt Rice <ratmice@gmail.com> writes:

Matt> 2011-07-11  Matt Rice  <ratmice@gmail.com>
Matt>         * bfin-tdep.c (bfin_extract_return_value): Fix swapped
Matt>         arguments to store_unsigned_integer.

I think this change is good, but I don't understand why this function
even calls store_unsigned_integer.  I thought
regcache_cooked_read_unsigned did that for you.

Tom

Re: [patch] bfin: swapped args in bfin_extract_return_value?

[Yao Qi]

On 07/20/2011 09:59 PM, Tom Tromey wrote:
> I think this change is good, but I don't understand why this function
> even calls store_unsigned_integer.  I thought
> regcache_cooked_read_unsigned did that for you.

regcache_cooked_read_unsigned and store_unsigned_integer can be replaced
by regcache_cooked_read, IIUC.  However, in the last iteration of loop,
the size of data might not be 4, so we may have to use
regcache_cooked_read_part and consider different data layout related to
endianess.  Looks like current approach doesn't have to worry about this
problem.

-- 
Yao (齐尧)

Re: [patch] bfin: swapped args in bfin_extract_return_value?

[Mike Frysinger]

On Tue, Jul 19, 2011 at 22:12, Matt Rice wrote:
> I don't really know this code, so this patch might be wrong,
> and don't have a setup to test it.
>
> but from the types, and the variable names, it smells of arguments
> that have been swapped.

the Blackfin gdb port is currently being maintained against gdb-6.6
which didnt have an endian argument.  when forward porting to
pre-gdb-7.3, the func now took an endian arg, so one was just thrown
in.  when the func was updated in mainline gdb, rather than add the
new arg to the end, it was inserted, so i guess the update review
didnt go as well as hopped.

in my local version (which contains FDPIC support), it seems like all
calls to store_unsigned_integer() in bfin-tdep.c have the last two
args swapped.  so i imagine it was an upgrade thinko.
-mike

Re: [patch] bfin: swapped args in bfin_extract_return_value?

[Mike Frysinger]

On Wed, Jul 20, 2011 at 09:59, Tom Tromey wrote:
>>>>>> "Matt" == Matt Rice <ratmice@gmail.com> writes:
>
> Matt> 2011-07-11  Matt Rice  <ratmice@gmail.com>
> Matt>         * bfin-tdep.c (bfin_extract_return_value): Fix swapped
> Matt>         arguments to store_unsigned_integer.
>
> I think this change is good, but I don't understand why this function
> even calls store_unsigned_integer.  I thought
> regcache_cooked_read_unsigned did that for you.

most likely because regcache_cooked_read() doesnt exist in gdb-6.6
(which is where the Blackfin port originated).  when moving from
something like 6.6 to 7.3, it's nigh impossible to pick out all the
idioms that have been obsoleted by new helper functions unless you're
very familiar with the gdb code base.  which i am not :).

i can test your proposal and post a follow up patch if it pans out.
-mike

Re: [patch] bfin: swapped args in bfin_extract_return_value?

[Matt Rice]

On Wed, Jul 20, 2011 at 9:03 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tue, Jul 19, 2011 at 22:12, Matt Rice wrote:
>> I don't really know this code, so this patch might be wrong,
>> and don't have a setup to test it.
>>
>> but from the types, and the variable names, it smells of arguments
>> that have been swapped.
>
> the Blackfin gdb port is currently being maintained against gdb-6.6
> which didnt have an endian argument.  when forward porting to
> pre-gdb-7.3, the func now took an endian arg, so one was just thrown
> in.  when the func was updated in mainline gdb, rather than add the
> new arg to the end, it was inserted, so i guess the update review
> didnt go as well as hopped.
>
> in my local version (which contains FDPIC support), it seems like all
> calls to store_unsigned_integer() in bfin-tdep.c have the last two
> args swapped.  so i imagine it was an upgrade thinko.
> -mike
>

Thanks for confirming, I commited my patch as originally posted.
not sure if Tom's was an OK, but it seems to at least fall under the
'its obvious now.' rule.

Re: [patch] bfin: swapped args in bfin_extract_return_value?

[Tom Tromey]

Matt> Thanks for confirming, I commited my patch as originally posted.
Matt> not sure if Tom's was an OK, but it seems to at least fall under the
Matt> 'its obvious now.' rule.

Yeah, I think it was obvious.

I think I was incorrect about the store_unsigned_integer.  It looks like
the return_value gdbarch method expects to see target-ordered bytes.
Sorry about the confusion.

Tom