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

[Duncan P. N. Exon Smith]

Usually just replying weekly with the text "ping" and the
reattached patch is your best bet.

I had a look, and the patch seems reasonable.  However, you
should use `FileCheck` [1] instead of `grep` and `count`.

[1]: http://llvm.org/docs/CommandGuide/FileCheck.html

Also, I find the name `AllocateStack()` confusing in this
context.  I think it would be better to extract out a helper
called `AlignStack()` that your new code and `AllocateStack()`
both call.

Question: why aren't you calling `ensureMaxAlignment()` here?

> On 2014-Aug-04, at 14:10, Thomas Jablin <tjablin at gmail.com> wrote:
> 
> 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
> 
> 
> 
> 
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits