[RFC/TileGX 1/6] fix args alignment bug

[RFC/TileGX 1/6] fix args alignment bug

[Jiong Wang]

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

for tilegx, args pushed on stack should be aligned to 64bit.

gdb/ChangeLog:

        * tilegx-tdep.c (tilegx_push_dummy_call): args pushed on stack
        should be aligned to 64bit.

please review.

---
Regards,
Jiong
Tilera Corporation.



[-- Attachment #2: 0001-tilegx-tdep.c-tilegx_push_dummy_call-stack-alignment.patch --]
[-- Type: text/x-patch, Size: 2034 bytes --]

---
 gdb/tilegx-tdep.c |   20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index 0c560ba..2f1d824 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -292,7 +292,7 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
   int argreg = TILEGX_R0_REGNUM;
   int i, j;
   int typelen, slacklen, alignlen;
-  static const gdb_byte two_zero_words[8] = { 0 };
+  static const gdb_byte four_zero_words[16] = { 0 };
 
   /* If struct_return is 1, then the struct return address will
      consume one argument-passing register.  */
@@ -326,18 +326,6 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
   /* Align SP.  */
   stack_dest = tilegx_frame_align (gdbarch, stack_dest);
 
-  /* Loop backwards through arguments to determine stack alignment.  */
-  alignlen = 0;
-
-  for (j = nargs - 1; j >= i; j--)
-    {
-      typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
-      alignlen += (typelen + 3) & (~3);
-    }
-
-  if (alignlen & 0x4)
-    stack_dest -= 4;
-
   /* Loop backwards through remaining arguments and push them on
      the stack, word aligned.  */
   for (j = nargs - 1; j >= i; j--)
@@ -347,7 +335,7 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
       const gdb_byte *contents = value_contents (args[j]);
 
       typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
-      slacklen = ((typelen + 3) & (~3)) - typelen;
+      slacklen = ((typelen + 7) & (~7)) - typelen;
       val = xmalloc (typelen + slacklen);
       back_to = make_cleanup (xfree, val);
       memcpy (val, contents, typelen);
@@ -360,8 +348,8 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
     }
 
   /* Add 2 words for linkage space to the stack.  */
-  stack_dest = stack_dest - 8;
-  write_memory (stack_dest, two_zero_words, 8);
+  stack_dest = stack_dest - 16;
+  write_memory (stack_dest, four_zero_words, 16);
 
   /* Update stack pointer.  */
   regcache_cooked_write_unsigned (regcache, TILEGX_SP_REGNUM, stack_dest);
-- 
1.7.10.3

Re: [RFC/TileGX 1/6] fix args alignment bug

[Joel Brobecker]

> gdb/ChangeLog:
> 
>        * tilegx-tdep.c (tilegx_push_dummy_call): args pushed on stack
>        should be aligned to 64bit.

Some questions below...

> -  /* Loop backwards through arguments to determine stack alignment.  */
> -  alignlen = 0;
> -
> -  for (j = nargs - 1; j >= i; j--)
> -    {
> -      typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
> -      alignlen += (typelen + 3) & (~3);
> -    }
> -
> -  if (alignlen & 0x4)
> -    stack_dest -= 4;

Do you have any hint as to why this code was written the way it was?
It doesn't seem like the type of code that would be added by accident.
Did Jeff (the apparent author) misunderstand the ABI?

>        typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
> -      slacklen = ((typelen + 3) & (~3)) - typelen;
> +      slacklen = ((typelen + 7) & (~7)) - typelen;

This is a detail, but can you use utils.c:align_up, in this case?
(I find those midly more readable than these incantations, but
it's OK if you prefer the code to stay as is).

>    /* Add 2 words for linkage space to the stack.  */
> -  stack_dest = stack_dest - 8;
> -  write_memory (stack_dest, two_zero_words, 8);
> +  stack_dest = stack_dest - 16;
> +  write_memory (stack_dest, four_zero_words, 16);

It looks like you need to adjust the comment as well.

-- 
Joel

Re: [RFC/TileGX 1/6] fix args alignment bug

[Jiong Wang]

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

Hi Joel,

   thanks for your careful review

>> -  /* Loop backwards through arguments to determine stack alignment.  */
>> -  alignlen = 0;
>> -
>> -  for (j = nargs - 1; j >= i; j--)
>> -    {
>> -      typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
>> -      alignlen += (typelen + 3) & (~3);
>> -    }
>> -
>> -  if (alignlen & 0x4)
>> -    stack_dest -= 4;
> Do you have any hint as to why this code was written the way it was?
> It doesn't seem like the type of code that would be added by accident.
> Did Jeff (the apparent author) misunderstand the ABI?

       actually, these these copied from our tilepro target which is a 
pure 32bit target, and the
compiler for that target is not gcc initially. so there maybe some 
historic reason for those code.

      but for tilegx, the ABI is actually very clear, 10 regs for 
arguments,  then push on stack.

      so I just delete them, gdb works ok on both daily usage and dejagnu
>
>>         typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
>> -      slacklen = ((typelen + 3) & (~3)) - typelen;
>> +      slacklen = ((typelen + 7) & (~7)) - typelen;
> This is a detail, but can you use utils.c:align_up, in this case?
> (I find those midly more readable than these incantations, but
> it's OK if you prefer the code to stay as is).
       thanks, I change to align_up to reuse existing helper function
>
>>     /* Add 2 words for linkage space to the stack.  */
>> -  stack_dest = stack_dest - 8;
>> -  write_memory (stack_dest, two_zero_words, 8);
>> +  stack_dest = stack_dest - 16;
>> +  write_memory (stack_dest, four_zero_words, 16);
> It looks like you need to adjust the comment as well.

    fixed.

   is this new 01-revise.patch ok for this issue?


[-- Attachment #2: 01-revise.patch --]
[-- Type: text/plain, Size: 2001 bytes --]

diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index 0c560ba..8be4046 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -292,7 +292,7 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
   int argreg = TILEGX_R0_REGNUM;
   int i, j;
   int typelen, slacklen, alignlen;
-  static const gdb_byte two_zero_words[8] = { 0 };
+  static const gdb_byte four_zero_words[16] = { 0 };
 
   /* If struct_return is 1, then the struct return address will
      consume one argument-passing register.  */
@@ -326,18 +326,6 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
   /* Align SP.  */
   stack_dest = tilegx_frame_align (gdbarch, stack_dest);
 
-  /* Loop backwards through arguments to determine stack alignment.  */
-  alignlen = 0;
-
-  for (j = nargs - 1; j >= i; j--)
-    {
-      typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
-      alignlen += (typelen + 3) & (~3);
-    }
-
-  if (alignlen & 0x4)
-    stack_dest -= 4;
-
   /* Loop backwards through remaining arguments and push them on
      the stack, word aligned.  */
   for (j = nargs - 1; j >= i; j--)
@@ -347,7 +335,7 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
       const gdb_byte *contents = value_contents (args[j]);
 
       typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
-      slacklen = ((typelen + 3) & (~3)) - typelen;
+      slacklen = align_up (typelen, 8) - typelen;
       val = xmalloc (typelen + slacklen);
       back_to = make_cleanup (xfree, val);
       memcpy (val, contents, typelen);
@@ -359,9 +347,9 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
       do_cleanups (back_to);
     }
 
-  /* Add 2 words for linkage space to the stack.  */
-  stack_dest = stack_dest - 8;
-  write_memory (stack_dest, two_zero_words, 8);
+  /* Add 16 bytes for linkage space to the stack.  */
+  stack_dest = stack_dest - 16;
+  write_memory (stack_dest, four_zero_words, 16);
 
   /* Update stack pointer.  */
   regcache_cooked_write_unsigned (regcache, TILEGX_SP_REGNUM, stack_dest);

Re: [RFC/TileGX 1/6] fix args alignment bug

[Jiong Wang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb18030"; format=flowed, Size: 1907 bytes --]

ÓÚ 2013/1/18 23:00, Jiong Wang дµÀ:
> Hi Joel,
>
>   thanks for your careful review
>
>>> -  /* Loop backwards through arguments to determine stack 
>>> alignment.  */
>>> -  alignlen = 0;
>>> -
>>> -  for (j = nargs - 1; j >= i; j--)
>>> -    {
>>> -      typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
>>> -      alignlen += (typelen + 3) & (~3);
>>> -    }
>>> -
>>> -  if (alignlen & 0x4)
>>> -    stack_dest -= 4;
>> Do you have any hint as to why this code was written the way it was?
>> It doesn't seem like the type of code that would be added by accident.
>> Did Jeff (the apparent author) misunderstand the ABI?
>
>       actually, these these copied from our tilepro target which is a 
> pure 32bit target, and the
> compiler for that target is not gcc initially. so there maybe some 
> historic reason for those code.
>
>      but for tilegx, the ABI is actually very clear, 10 regs for 
> arguments,  then push on stack.
>
>      so I just delete them, gdb works ok on both daily usage and dejagnu
>>
>>>         typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
>>> -      slacklen = ((typelen + 3) & (~3)) - typelen;
>>> +      slacklen = ((typelen + 7) & (~7)) - typelen;
>> This is a detail, but can you use utils.c:align_up, in this case?
>> (I find those midly more readable than these incantations, but
>> it's OK if you prefer the code to stay as is).
>       thanks, I change to align_up to reuse existing helper function
>>
>>>     /* Add 2 words for linkage space to the stack.  */
>>> -  stack_dest = stack_dest - 8;
>>> -  write_memory (stack_dest, two_zero_words, 8);
>>> +  stack_dest = stack_dest - 16;
>>> +  write_memory (stack_dest, four_zero_words, 16);
>> It looks like you need to adjust the comment as well.
>
>    fixed.
>
>   is this new 01-revise.patch ok for this issue?
>
Ping, are this and the other updated tilegx patches OK?

Thanks,
Jiong

Re: [RFC/TileGX 1/6] fix args alignment bug

[Jiong Wang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb18030"; format=flowed, Size: 2315 bytes --]

PING, any feedback?

I had fixed all issues according to feedback, these patches cut tilegx 
Dejagnu unexpected failures from 120 to about 50.

so is it OK to commit them? if it's OK to commit, can any one help 
commit them? we do not have a gdb account yet.

thanks

Regards,
Jiong
Tilera Corporation.


ÓÚ 2013/1/22 18:28, Jiong Wang дµÀ:
> ÓÚ 2013/1/18 23:00, Jiong Wang дµÀ:
>> Hi Joel,
>>
>>   thanks for your careful review
>>
>>>> -  /* Loop backwards through arguments to determine stack 
>>>> alignment.  */
>>>> -  alignlen = 0;
>>>> -
>>>> -  for (j = nargs - 1; j >= i; j--)
>>>> -    {
>>>> -      typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
>>>> -      alignlen += (typelen + 3) & (~3);
>>>> -    }
>>>> -
>>>> -  if (alignlen & 0x4)
>>>> -    stack_dest -= 4;
>>> Do you have any hint as to why this code was written the way it was?
>>> It doesn't seem like the type of code that would be added by accident.
>>> Did Jeff (the apparent author) misunderstand the ABI?
>>
>>       actually, these these copied from our tilepro target which is a 
>> pure 32bit target, and the
>> compiler for that target is not gcc initially. so there maybe some 
>> historic reason for those code.
>>
>>      but for tilegx, the ABI is actually very clear, 10 regs for 
>> arguments,  then push on stack.
>>
>>      so I just delete them, gdb works ok on both daily usage and dejagnu
>>>
>>>>         typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
>>>> -      slacklen = ((typelen + 3) & (~3)) - typelen;
>>>> +      slacklen = ((typelen + 7) & (~7)) - typelen;
>>> This is a detail, but can you use utils.c:align_up, in this case?
>>> (I find those midly more readable than these incantations, but
>>> it's OK if you prefer the code to stay as is).
>>       thanks, I change to align_up to reuse existing helper function
>>>
>>>>     /* Add 2 words for linkage space to the stack.  */
>>>> -  stack_dest = stack_dest - 8;
>>>> -  write_memory (stack_dest, two_zero_words, 8);
>>>> +  stack_dest = stack_dest - 16;
>>>> +  write_memory (stack_dest, four_zero_words, 16);
>>> It looks like you need to adjust the comment as well.
>>
>>    fixed.
>>
>>   is this new 01-revise.patch ok for this issue?
>>
> Ping, are this and the other updated tilegx patches OK?
>
> Thanks,
> Jiong
>

Re: [RFC/TileGX 1/6] fix args alignment bug

[Jiong Wang]

Ok, if no one object, Walter (GNU appointed TileGX gcc/binutils 
maintainer) will checkin all these updated TileGX patches.

thanks.
---
Regards,
Jiong

于 2013/1/29 22:48, Jiong Wang 写道:
> PING, any feedback?
>
> I had fixed all issues according to feedback, these patches cut tilegx 
> Dejagnu unexpected failures from 120 to about 50.
>
> so is it OK to commit them? if it's OK to commit, can any one help 
> commit them? we do not have a gdb account yet.
>
> thanks
>
> Regards,
> Jiong
> Tilera Corporation.
>
>
> 于 2013/1/22 18:28, Jiong Wang 写道:
>> 于 2013/1/18 23:00, Jiong Wang 写道:
>>> Hi Joel,
>>>
>>>   thanks for your careful review
>>>
>>>>> -  /* Loop backwards through arguments to determine stack 
>>>>> alignment.  */
>>>>> -  alignlen = 0;
>>>>> -
>>>>> -  for (j = nargs - 1; j >= i; j--)
>>>>> -    {
>>>>> -      typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
>>>>> -      alignlen += (typelen + 3) & (~3);
>>>>> -    }
>>>>> -
>>>>> -  if (alignlen & 0x4)
>>>>> -    stack_dest -= 4;
>>>> Do you have any hint as to why this code was written the way it was?
>>>> It doesn't seem like the type of code that would be added by accident.
>>>> Did Jeff (the apparent author) misunderstand the ABI?
>>>
>>>       actually, these these copied from our tilepro target which is 
>>> a pure 32bit target, and the
>>> compiler for that target is not gcc initially. so there maybe some 
>>> historic reason for those code.
>>>
>>>      but for tilegx, the ABI is actually very clear, 10 regs for 
>>> arguments,  then push on stack.
>>>
>>>      so I just delete them, gdb works ok on both daily usage and 
>>> dejagnu
>>>>
>>>>>         typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
>>>>> -      slacklen = ((typelen + 3) & (~3)) - typelen;
>>>>> +      slacklen = ((typelen + 7) & (~7)) - typelen;
>>>> This is a detail, but can you use utils.c:align_up, in this case?
>>>> (I find those midly more readable than these incantations, but
>>>> it's OK if you prefer the code to stay as is).
>>>       thanks, I change to align_up to reuse existing helper function
>>>>
>>>>>     /* Add 2 words for linkage space to the stack.  */
>>>>> -  stack_dest = stack_dest - 8;
>>>>> -  write_memory (stack_dest, two_zero_words, 8);
>>>>> +  stack_dest = stack_dest - 16;
>>>>> +  write_memory (stack_dest, four_zero_words, 16);
>>>> It looks like you need to adjust the comment as well.
>>>
>>>    fixed.
>>>
>>>   is this new 01-revise.patch ok for this issue?
>>>
>> Ping, are this and the other updated tilegx patches OK?
>>
>> Thanks,
>> Jiong
>>
>

Re: [RFC/TileGX 1/6] fix args alignment bug

[Joel Brobecker]

> Ok, if no one object, Walter (GNU appointed TileGX gcc/binutils
> maintainer) will checkin all these updated TileGX patches.

Jiong, please be patient. I apologize for the delay, but I cannot
find the time to review these changes at the moment. Unless another
*GDB* Maintainers finds the time to take over, please wait for explicit
approval before checking anything in. This is the procedure that we
all follow.

Also, accepted protocol in this list is to wait for a week or two
between pings.

Thank you,
-- 
Joel

Re: [RFC/TileGX 1/6] fix args alignment bug

[Jiong Wang]

于 2013/1/30 13:46, Joel Brobecker 写道:
>> Ok, if no one object, Walter (GNU appointed TileGX gcc/binutils
>> maintainer) will checkin all these updated TileGX patches.
> Jiong, please be patient. I apologize for the delay, but I cannot
> find the time to review these changes at the moment. Unless another
> *GDB* Maintainers finds the time to take over, please wait for explicit
> approval before checking anything in. This is the procedure that we
> all follow.
>
> Also, accepted protocol in this list is to wait for a week or two
> between pings.
Hi Joel,

I see. it's happy to receive feedback, then I will wait.

---
Regards,
Jiong

>
> Thank you,

Re: [RFC/TileGX 1/6] fix args alignment bug

[Joel Brobecker]

>   is this new 01-revise.patch ok for this issue?

Apologies for the delay. The patch is OK, but you forgot to
provide a ChangeLog entry (I would prefer if you repeated it,
even if it remains unchanged). Patch is approved with the original
ChangeLog.

-- 
Joel

[RFC/TileGX 1/2] fix gdbserver build failure

[Jiong Wang]

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

TileGX gdbserver build failed on native board.

"srv_tgtobj“ in gdbserver/configure.src not updated.


gdbserver/ChangeLog:

* configure.srv (srv_tgtobj): Remove linux-osdata.o, add linux-tile-low.o.

please review.

thanks.

---
Regards,
Jiong
Tilera Corporation.

[-- Attachment #2: gdb-01.patch --]
[-- Type: text/x-patch, Size: 463 bytes --]

diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
index 6eb5a8a..f65dd10 100644
--- a/gdb/gdbserver/configure.srv
+++ b/gdb/gdbserver/configure.srv
@@ -340,7 +340,7 @@ case "${target}" in
 			;;
   tilegx-*-linux*)	srv_regobj=reg-tilegx.o
 			srv_regobj="${srv_regobj} reg-tilegx32.o"
-			srv_tgtobj="$srv_linux_obj linux-osdata.o"
+			srv_tgtobj="$srv_linux_obj linux-tile-low.o"
 			srv_linux_regsets=yes
 			srv_linux_thread_db=yes
 			;;

[RFC/TileGX 2/2] fix gdbserver runtime crash

[Jiong Wang]

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

TileGX is always 64bit, ptrace always return 64bit data,  tilegx32 is 
just an ABI.

So the regset size should be regnum * 8 instead of regnum * 4. The 
latter cause gdbserver crash.

gdbserver/ChangeLog:

   * linux-tile-low.c (tile_regsets): Modify the size field as 64bit for 
each register.

please review.

thanks.

---
Regards,
Jiong
Tilera Corporation.

[-- Attachment #2: gdb-02.patch --]
[-- Type: text/x-patch, Size: 531 bytes --]

diff --git a/gdb/gdbserver/linux-tile-low.c b/gdb/gdbserver/linux-tile-low.c
index 8963b9a..8efb34e 100644
--- a/gdb/gdbserver/linux-tile-low.c
+++ b/gdb/gdbserver/linux-tile-low.c
@@ -124,7 +124,7 @@ tile_store_gregset (struct regcache *regcache, const void *buf)
 
 static struct regset_info tile_regsets[] =
 {
-  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 4,
+  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 8,
     GENERAL_REGS, tile_fill_gregset, tile_store_gregset },
   { 0, 0, 0, -1, -1, NULL, NULL }
 };

Re: [RFC/TileGX 2/2] fix gdbserver runtime crash

[Joel Brobecker]

> TileGX is always 64bit, ptrace always return 64bit data,  tilegx32
> is just an ABI.
> 
> So the regset size should be regnum * 8 instead of regnum * 4. The
> latter cause gdbserver crash.
> 
> gdbserver/ChangeLog:
> 
>   * linux-tile-low.c (tile_regsets): Modify the size field as 64bit
> for each register.

I will trust you on this one, but how did this even work at all
prior to your change???

> diff --git a/gdb/gdbserver/linux-tile-low.c b/gdb/gdbserver/linux-tile-low.c
> index 8963b9a..8efb34e 100644
> --- a/gdb/gdbserver/linux-tile-low.c
> +++ b/gdb/gdbserver/linux-tile-low.c
> @@ -124,7 +124,7 @@ tile_store_gregset (struct regcache *regcache, const void *buf)
>  
>  static struct regset_info tile_regsets[] =
>  {
> -  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 4,
> +  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 8,
>      GENERAL_REGS, tile_fill_gregset, tile_store_gregset },
>    { 0, 0, 0, -1, -1, NULL, NULL }
>  };


-- 
Joel

Re: [RFC/TileGX 2/2] fix gdbserver runtime crash

[Jiong Wang]

于 2013/9/17 20:33, Joel Brobecker 写道:
>> TileGX is always 64bit, ptrace always return 64bit data,  tilegx32
>> is just an ABI.
>>
>> So the regset size should be regnum * 8 instead of regnum * 4. The
>> latter cause gdbserver crash.
>>
>> gdbserver/ChangeLog:
>>
>>    * linux-tile-low.c (tile_regsets): Modify the size field as 64bit
>> for each register.
> I will trust you on this one, but how did this even work at all
> prior to your change???

it's a complicated story and my fault.

we use Perforce to manage code within the company. This bug is fixed by 
Jeff (previous maintainer of TileGX gdb) long time ago in Perforce, but he
didn't commit it to community. And we mainly test binaries generated 
from our Perforce.

I should be more careful when I check dejagnu test result when submit to 
community :)



>
>> diff --git a/gdb/gdbserver/linux-tile-low.c b/gdb/gdbserver/linux-tile-low.c
>> index 8963b9a..8efb34e 100644
>> --- a/gdb/gdbserver/linux-tile-low.c
>> +++ b/gdb/gdbserver/linux-tile-low.c
>> @@ -124,7 +124,7 @@ tile_store_gregset (struct regcache *regcache, const void *buf)
>>   
>>   static struct regset_info tile_regsets[] =
>>   {
>> -  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 4,
>> +  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 8,
>>       GENERAL_REGS, tile_fill_gregset, tile_store_gregset },
>>     { 0, 0, 0, -1, -1, NULL, NULL }
>>   };
>

Re: [RFC/TileGX 2/2] fix gdbserver runtime crash

[Jiong Wang]

于 2013/9/17 20:39, Jiong Wang 写道:
> 于 2013/9/17 20:33, Joel Brobecker 写道:
>>> TileGX is always 64bit, ptrace always return 64bit data,  tilegx32
>>> is just an ABI.
>>>
>>> So the regset size should be regnum * 8 instead of regnum * 4. The
>>> latter cause gdbserver crash.
>>>
>>> gdbserver/ChangeLog:
>>>
>>>    * linux-tile-low.c (tile_regsets): Modify the size field as 64bit
>>> for each register.
>> I will trust you on this one, but how did this even work at all
>> prior to your change???
>
> it's a complicated story and my fault.
>
> we use Perforce to manage code within the company. This bug is fixed 
> by Jeff (previous maintainer of TileGX gdb) long time ago in Perforce, 
> but he
> didn't commit it to community. And we mainly test binaries generated 
> from our Perforce.
>
> I should be more careful when I check dejagnu test result when submit 
> to community :)

is this OK to commit?

>
>
>
>>
>>> diff --git a/gdb/gdbserver/linux-tile-low.c 
>>> b/gdb/gdbserver/linux-tile-low.c
>>> index 8963b9a..8efb34e 100644
>>> --- a/gdb/gdbserver/linux-tile-low.c
>>> +++ b/gdb/gdbserver/linux-tile-low.c
>>> @@ -124,7 +124,7 @@ tile_store_gregset (struct regcache *regcache, 
>>> const void *buf)
>>>     static struct regset_info tile_regsets[] =
>>>   {
>>> -  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 4,
>>> +  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 8,
>>>       GENERAL_REGS, tile_fill_gregset, tile_store_gregset },
>>>     { 0, 0, 0, -1, -1, NULL, NULL }
>>>   };
>>
>

Re: [RFC/TileGX 2/2] fix gdbserver runtime crash

[Joel Brobecker]

> >it's a complicated story and my fault.
> >
> >we use Perforce to manage code within the company. This bug is
> >fixed by Jeff (previous maintainer of TileGX gdb) long time ago in
> >Perforce, but he
> >didn't commit it to community. And we mainly test binaries
> >generated from our Perforce.
> >
> >I should be more careful when I check dejagnu test result when
> >submit to community :)

No worries, here, just trying to understand the full story.

> is this OK to commit?

Yes.

> >>>diff --git a/gdb/gdbserver/linux-tile-low.c
> >>>b/gdb/gdbserver/linux-tile-low.c
> >>>index 8963b9a..8efb34e 100644
> >>>--- a/gdb/gdbserver/linux-tile-low.c
> >>>+++ b/gdb/gdbserver/linux-tile-low.c
> >>>@@ -124,7 +124,7 @@ tile_store_gregset (struct regcache
> >>>*regcache, const void *buf)
> >>>    static struct regset_info tile_regsets[] =
> >>>  {
> >>>-  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 4,
> >>>+  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 8,
> >>>      GENERAL_REGS, tile_fill_gregset, tile_store_gregset },
> >>>    { 0, 0, 0, -1, -1, NULL, NULL }
> >>>  };
> >>
> >

-- 
Joel

[COMMITTED][RFC/TileGX 2/2] fix gdbserver runtime crash

[Jiong Wang]

On 09/17/2013 09:28 PM, Joel Brobecker wrote:
>>> it's a complicated story and my fault.
>>>
>>> we use Perforce to manage code within the company. This bug is
>>> fixed by Jeff (previous maintainer of TileGX gdb) long time ago in
>>> Perforce, but he
>>> didn't commit it to community. And we mainly test binaries
>>> generated from our Perforce.
>>>
>>> I should be more careful when I check dejagnu test result when
>>> submit to community :)
> No worries, here, just trying to understand the full story.
>
>> is this OK to commit?
> Yes.

committed.

2013-09-16  Jiong Wang  <jiwang@tilera.com>

	* linux-tile-low.c (tile_regsets): Modify the size field to 64-bit
	for each register.

===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-tile-low.c,v
retrieving revision 1.4
retrieving revision 1.5
diff -u -r1.4 -r1.5
--- src/gdb/gdbserver/linux-tile-low.c	2013/06/07 10:46:59	1.4
+++ src/gdb/gdbserver/linux-tile-low.c	2013/09/17 14:00:30	1.5
@@ -124,7 +124,7 @@
  
  static struct regset_info tile_regsets[] =
  {
-  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 4,
+  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 8,
      GENERAL_REGS, tile_fill_gregset, tile_store_gregset },
    { 0, 0, 0, -1, -1, NULL, NULL }
  };


>
>>>>> diff --git a/gdb/gdbserver/linux-tile-low.c
>>>>> b/gdb/gdbserver/linux-tile-low.c
>>>>> index 8963b9a..8efb34e 100644
>>>>> --- a/gdb/gdbserver/linux-tile-low.c
>>>>> +++ b/gdb/gdbserver/linux-tile-low.c
>>>>> @@ -124,7 +124,7 @@ tile_store_gregset (struct regcache
>>>>> *regcache, const void *buf)
>>>>>     static struct regset_info tile_regsets[] =
>>>>>   {
>>>>> -  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 4,
>>>>> +  { PTRACE_GETREGS, PTRACE_SETREGS, 0, tile_num_regs * 8,
>>>>>       GENERAL_REGS, tile_fill_gregset, tile_store_gregset },
>>>>>     { 0, 0, 0, -1, -1, NULL, NULL }
>>>>>   };

Re: [RFC/TileGX 1/2] fix gdbserver build failure

[Joel Brobecker]

> TileGX gdbserver build failed on native board.
> 
> "srv_tgtobj“ in gdbserver/configure.src not updated.
> 
> 
> gdbserver/ChangeLog:
> 
> * configure.srv (srv_tgtobj): Remove linux-osdata.o, add linux-tile-low.o.
> 
> please review.

I am trying to understand the circumstances that lead to the failure,
and it's been made harder due to the lack of information. After some
investigation, my guess is that you're getting some duplicate symbols
because linux-osdata.o" is already in srv_tgtobj. Is that correct?
If yes, this part of the patch is OK, obvious even (once explained!).

I don't understand, however, what difference adding linux-tile-low.o
makes in terms of build failure.  Again, I have been trying to figure
out what lead to the failure, and searched our recent past, but could
not find anything. I know the change makes complete sense, but if not
logically tied to the build failure you are mentioning, I think we
should, if nothing else, document that fact for future archival, and
also commit this part of the change separately.

> diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
> index 6eb5a8a..f65dd10 100644
> --- a/gdb/gdbserver/configure.srv
> +++ b/gdb/gdbserver/configure.srv
> @@ -340,7 +340,7 @@ case "${target}" in
>  			;;
>    tilegx-*-linux*)	srv_regobj=reg-tilegx.o
>  			srv_regobj="${srv_regobj} reg-tilegx32.o"
> -			srv_tgtobj="$srv_linux_obj linux-osdata.o"
> +			srv_tgtobj="$srv_linux_obj linux-tile-low.o"
>  			srv_linux_regsets=yes
>  			srv_linux_thread_db=yes
>  			;;


-- 
Joel

Re: [RFC/TileGX 1/2] fix gdbserver build failure

[Jiong Wang]

于 2013/9/17 20:30, Joel Brobecker 写道:
>> TileGX gdbserver build failed on native board.
>>
>> "srv_tgtobj“ in gdbserver/configure.src not updated.
>>
>>
>> gdbserver/ChangeLog:
>>
>> * configure.srv (srv_tgtobj): Remove linux-osdata.o, add linux-tile-low.o.
>>
>> please review.
> I am trying to understand the circumstances that lead to the failure,
> and it's been made harder due to the lack of information. After some
> investigation, my guess is that you're getting some duplicate symbols
> because linux-osdata.o" is already in srv_tgtobj. Is that correct?
> If yes, this part of the patch is OK, obvious even (once explained!).

yes, exactly this error.


>
> I don't understand, however, what difference adding linux-tile-low.o
> makes in terms of build failure.  Again, I have been trying to figure
> out what lead to the failure, and searched our recent past, but could
> not find anything. I know the change makes complete sense, but if not
> logically tied to the build failure you are mentioning, I think we
> should, if nothing else, document that fact for future archival, and
> also commit this part of the change separately.
>
>> diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
>> index 6eb5a8a..f65dd10 100644
>> --- a/gdb/gdbserver/configure.srv
>> +++ b/gdb/gdbserver/configure.srv
>> @@ -340,7 +340,7 @@ case "${target}" in
>>   			;;
>>     tilegx-*-linux*)	srv_regobj=reg-tilegx.o
>>   			srv_regobj="${srv_regobj} reg-tilegx32.o"
>> -			srv_tgtobj="$srv_linux_obj linux-osdata.o"
>> +			srv_tgtobj="$srv_linux_obj linux-tile-low.o"
>>   			srv_linux_regsets=yes
>>   			srv_linux_thread_db=yes
>>   			;;
>

Re: [RFC/TileGX 1/2] fix gdbserver build failure

[Jiong Wang]

于 2013/9/17 20:33, Jiong Wang 写道:
> 于 2013/9/17 20:30, Joel Brobecker 写道:
>>> TileGX gdbserver build failed on native board.
>>>
>>> "srv_tgtobj“ in gdbserver/configure.src not updated.
>>>
>>>
>>> gdbserver/ChangeLog:
>>>
>>> * configure.srv (srv_tgtobj): Remove linux-osdata.o, add 
>>> linux-tile-low.o.
>>>
>>> please review.
>> I am trying to understand the circumstances that lead to the failure,
>> and it's been made harder due to the lack of information. After some
>> investigation, my guess is that you're getting some duplicate symbols
>> because linux-osdata.o" is already in srv_tgtobj. Is that correct?
>> If yes, this part of the patch is OK, obvious even (once explained!).
>
> yes, exactly this error.

is this OK to commit ?

>
>
>>
>> I don't understand, however, what difference adding linux-tile-low.o
>> makes in terms of build failure.  Again, I have been trying to figure
>> out what lead to the failure, and searched our recent past, but could
>> not find anything. I know the change makes complete sense, but if not
>> logically tied to the build failure you are mentioning, I think we
>> should, if nothing else, document that fact for future archival, and
>> also commit this part of the change separately.
>>
>>> diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
>>> index 6eb5a8a..f65dd10 100644
>>> --- a/gdb/gdbserver/configure.srv
>>> +++ b/gdb/gdbserver/configure.srv
>>> @@ -340,7 +340,7 @@ case "${target}" in
>>>               ;;
>>>     tilegx-*-linux*)    srv_regobj=reg-tilegx.o
>>>               srv_regobj="${srv_regobj} reg-tilegx32.o"
>>> -            srv_tgtobj="$srv_linux_obj linux-osdata.o"
>>> +            srv_tgtobj="$srv_linux_obj linux-tile-low.o"
>>>               srv_linux_regsets=yes
>>>               srv_linux_thread_db=yes
>>>               ;;
>>
>

Re: [RFC/TileGX 1/2] fix gdbserver build failure

[Joel Brobecker]

> >>>gdbserver/ChangeLog:
> >>>
> >>>* configure.srv (srv_tgtobj): Remove linux-osdata.o, add
> >>>linux-tile-low.o.
> >>>
> >>>please review.
> >>I am trying to understand the circumstances that lead to the failure,
> >>and it's been made harder due to the lack of information. After some
> >>investigation, my guess is that you're getting some duplicate symbols
> >>because linux-osdata.o" is already in srv_tgtobj. Is that correct?
> >>If yes, this part of the patch is OK, obvious even (once explained!).
> >
> >yes, exactly this error.
> 
> is this OK to commit ?

Only the part that remove linux-osdata.o for now.

I expect that you'll be able to commit the other half, but after
having explained in detail what this fixes. I suspect that this
enables some features, rather than fix a failure.

-- 
Joel

Re: [RFC/TileGX 1/2] fix gdbserver build failure

[Jiong Wang]

于 2013/9/17 21:30, Joel Brobecker 写道:
>>>>> gdbserver/ChangeLog:
>>>>>
>>>>> * configure.srv (srv_tgtobj): Remove linux-osdata.o, add
>>>>> linux-tile-low.o.
>>>>>
>>>>> please review.
>>>> I am trying to understand the circumstances that lead to the failure,
>>>> and it's been made harder due to the lack of information. After some
>>>> investigation, my guess is that you're getting some duplicate symbols
>>>> because linux-osdata.o" is already in srv_tgtobj. Is that correct?
>>>> If yes, this part of the patch is OK, obvious even (once explained!).
>>> yes, exactly this error.
>> is this OK to commit ?
> Only the part that remove linux-osdata.o for now.
>
> I expect that you'll be able to commit the other half, but after
> having explained in detail what this fixes. I suspect that this
> enables some features, rather than fix a failure.

linux-tile-low.o is the target file of TileGX. (it's name is confusing ?)

previous commit by the following wrongly removed the tilegx target file.

commit f7fc28008a4d76dd6e55eb77070eb18e956406a9
Author: Luis Machado <luisgpm@br.ibm.com>
Date: Thu Aug 22 23:46:27 2013 +0000

- srv_tgtobj="linux-low.o linux-tile-low.o linux-osdata.o linux-procfs.o"
- srv_tgtobj="${srv_tgtobj} linux-ptrace.o"
+ srv_tgtobj="$srv_linux_obj linux-osdata.o"

>

Re: [RFC/TileGX 1/2] fix gdbserver build failure

[Joel Brobecker]

> linux-tile-low.o is the target file of TileGX. (it's name is confusing ?)
> 
> previous commit by the following wrongly removed the tilegx target file.
> 
> commit f7fc28008a4d76dd6e55eb77070eb18e956406a9
> Author: Luis Machado <luisgpm@br.ibm.com>
> Date: Thu Aug 22 23:46:27 2013 +0000
> 
> - srv_tgtobj="linux-low.o linux-tile-low.o linux-osdata.o linux-procfs.o"
> - srv_tgtobj="${srv_tgtobj} linux-ptrace.o"
> + srv_tgtobj="$srv_linux_obj linux-osdata.o"

Ok. Thank you for that, I had missed that commit in my research.
That goes to show how important it is to send detailed descriptions
of the problem when sending a patch. Otherwise, we end up wasting
time trying to figure it out again.

I knew what the file was about, and I knew it made sense. But I'd like
every patch to be explained on this list before it gets checked in.
This is for later on, when someone tries to figure out why a patch
was applied, at least it's documented in one public place.

Please go ahead and commit.

-- 
Joel

[COMMITTED][RFC/TileGX 1/2] fix gdbserver build failure

[Jiong Wang]

On 09/17/2013 09:43 PM, Joel Brobecker wrote:
>> linux-tile-low.o is the target file of TileGX. (it's name is confusing ?)
>>
>> previous commit by the following wrongly removed the tilegx target file.
>>
>> commit f7fc28008a4d76dd6e55eb77070eb18e956406a9
>> Author: Luis Machado <luisgpm@br.ibm.com>
>> Date: Thu Aug 22 23:46:27 2013 +0000
>>
>> - srv_tgtobj="linux-low.o linux-tile-low.o linux-osdata.o linux-procfs.o"
>> - srv_tgtobj="${srv_tgtobj} linux-ptrace.o"
>> + srv_tgtobj="$srv_linux_obj linux-osdata.o"
> Ok. Thank you for that, I had missed that commit in my research.
> That goes to show how important it is to send detailed descriptions
> of the problem when sending a patch. Otherwise, we end up wasting
> time trying to figure it out again.
>
> I knew what the file was about, and I knew it made sense. But I'd like
> every patch to be explained on this list before it gets checked in.
> This is for later on, when someone tries to figure out why a patch
> was applied, at least it's documented in one public place.
understand.
thanks for careful review.

commited.

2013-09-16  Jiong Wang  <jiwang@tilera.com>

	* configure.srv <tilegx*-*-linux*>: Remove linux-osdata.o from and add
	linux-tile-low.o to srv_tgtobj.


===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/configure.srv,v
retrieving revision 1.79
retrieving revision 1.80
diff -u -r1.79 -r1.80
--- src/gdb/gdbserver/configure.srv	2013/09/13 14:17:30	1.79
+++ src/gdb/gdbserver/configure.srv	2013/09/17 13:56:54	1.80
@@ -340,7 +340,7 @@
  			;;
    tilegx-*-linux*)	srv_regobj=reg-tilegx.o
  			srv_regobj="${srv_regobj} reg-tilegx32.o"
-			srv_tgtobj="$srv_linux_obj linux-osdata.o"
+			srv_tgtobj="$srv_linux_obj linux-tile-low.o"
  			srv_linux_regsets=yes
  			srv_linux_thread_db=yes
  			;;


>
> Please go ahead and commit.
>

Re: [COMMITTED][RFC/TileGX 1/2] fix gdbserver build failure

[Yao Qi]

On 09/17/2013 10:04 PM, Jiong Wang wrote:
> 2013-09-16  Jiong Wang<jiwang@tilera.com>
> 
> 	* configure.srv <tilegx*-*-linux*>: Remove linux-osdata.o from and add
> 	linux-tile-low.o to srv_tgtobj.

Wang Jiong,
The changelog entry here is correct, but the entry in ChangeLog file is
wrong.  A blank line after date is missing.  Here is the fix.
Committed.

-- 
Yao (齐尧)

Index: gdbserver/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/ChangeLog,v
retrieving revision 1.771
diff -u -r1.771 ChangeLog
--- gdbserver/ChangeLog	17 Sep 2013 14:00:30 -0000	1.771
+++ gdbserver/ChangeLog	18 Sep 2013 01:50:14 -0000
@@ -1,8 +1,10 @@
 2013-09-16  Jiong Wang  <jiwang@tilera.com>
+
 	* linux-tile-low.c (tile_regsets): Modify the size field to 64-bit
 	for each register.
 
 2013-09-16  Jiong Wang  <jiwang@tilera.com>
+
 	* configure.srv <tilegx*-*-linux*>: Remove linux-osdata.o from and add
 	linux-tile-low.o to srv_tgtobj.