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

[Jiong Wang <jiwang@tilera.com>]

[-- 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);