[LLVMdev] Request to review patch for bug #14792

[Eli Friedman]

On Thu, Jul 18, 2013 at 10:31 PM, Yun-Wei Lee <lee2041412 at gmail.com> wrote:
> Thank for your suggestion.
>
> I have tried to use GetAlignedArgumentStackSize.
> It is so strange that this function may output uncorrectly.
> For example, the StackSize will be 40 when handling the third function call
> in main.
> If I modified the this function, it will fail one test.
> I wonder this may be another bug in llvm......
> And that's why I didn't use GetAlignedArgumentStackSize previously.
>
> Thank you!

I'm sorry, I'm not that familiar with the code.  A brief look tells me
that it's likely none of the patches are quite right;
http://llvm.org/bugs/attachment.cgi?id=10897 looks closer, but still
not quite right.

-Eli

>
> On Thu, Jul 18, 2013 at 7:56 PM, Eli Friedman <eli.friedman at gmail.com>
> wrote:
>>
>> On Thu, Jul 18, 2013 at 8:36 AM, Yun-Wei Lee <lee2041412 at gmail.com> wrote:
>> > http://llvm.org/bugs/show_bug.cgi?id=14792
>> >
>> > Problem:
>> >   In the i386 ABI Page 3-10, it said that the stack is aligned. However,
>> > the
>> > two example code show that does not handle the alignment correctly when
>> > using variadic function. For example, if the size of the first argument
>> > is
>> > 17, the overflow_arg_area in va_list will be set to "address of first
>> > argument + 16" instead of "address of first argument + 24" after calling
>> > va_start.
>> >   In addition, #6636 showed the same problem because in AMD64, arguments
>> > is
>> > passed by register at first, then pass by memory when run out of
>> > register
>> > (AMD64 ABI 3.5.7 rule 10).
>> >
>> > Why this problem happened?
>> >   When calling va_start to set va_list, overflow_arg_area is not set
>> > correctly. To set the overflow_arg_area correctly, we need to get the
>> > FrameIndex correctly. Now, here comes the problem, llvm doesn't handle
>> > it
>> > correctly. It accounts for StackSize to compute the FrameIndex, and if
>> > the
>> > StackSize is not aligned, it will compute the wrong FrameIndex. As a
>> > result
>> > overflow_arg_area will not be set correctly.
>> >
>> > My Solution:
>> > 1. Record the Align if it is located in Memory.
>> > 2. If it is variadic function and needs to set FrameIndex, adjust the
>> > stacksize.
>>
>> Please read http://llvm.org/docs/DeveloperPolicy.html .  In
>> particular, patches should be sent to llvm-commits, and patches should
>> generally include a regression test.
>>
>> In terms of the code, you might want to consider using
>> llvm::RoundUpToAlignment.
>>
>> -Eli
>
>