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

Jiong Wang
2013-01-30T04:34:18Z

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
>>
>