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

Jiong Wang
2013-01-18T15:00:50Z

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