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

[Jiong Wang <jiwang@tilera.com>]

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