[PATCH] Ensure proper alignment when reading variadic arguments for x86-32 and x86-64

[Thomas Jablin]

Is there anything I can do to accelerate the review of this patch? I
originally submitted it to llvm-commits on June 24.
Tom


On Mon, Jul 28, 2014 at 8:34 AM, Thomas Jablin <tjablin at gmail.com> wrote:

> Adding Elena since she most recently touched this function.
>
>
> On Mon, Jul 28, 2014 at 8:15 AM, Thomas Jablin <tjablin at gmail.com> wrote:
>
>> This patch still requires a reviewer.
>>
>>
>> On Mon, Jul 14, 2014 at 4:07 PM, Thomas Jablin <tjablin at gmail.com> wrote:
>>
>>> I'm still waiting for a review. This is a patch for PR14792.
>>> Tom
>>>
>>>
>>> On Mon, Jun 30, 2014 at 12:34 PM, Thomas Jablin <tjablin at gmail.com>
>>> wrote:
>>>
>>>> I'm still waiting for a review. Maybe Nadav since he owns the x86
>>>> backend?
>>>>
>>>>
>>>> On Tue, Jun 24, 2014 at 12:36 PM, Thomas Jablin <tjablin at gmail.com>
>>>> wrote:
>>>>
>>>>> Hi All,
>>>>> Please find enclosed an updated and simplified patch along with a
>>>>> additional test for properly aligned variadic arguments.
>>>>> Tom
>>>>>
>>>>> On Fri, Jul 19, 2013 at 8:36 PM, Eli Friedman <eli.friedman at gmail.com>
>>>>> wrote:
>>>>>
>>>>>> 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
>>>>>> >
>>>>>> >
>>>>>> _______________________________________________
>>>>>> LLVM Developers mailing list
>>>>>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140804/9b49d563/attachment.html>