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

Tue Aug 19 05:24:52 PDT 2014

Hi Duncan,
Could you please push the patch upstream when you get a chance? I don't
have commit privileges. Thanks.
Tom

On Mon, Aug 18, 2014 at 7:03 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

> LGTM.
>
> > On 2014-Aug-18, at 16:59, Thomas Jablin <tjablin at gmail.com> wrote:
> >
> > Hi Duncan,
> > Sorry for the long delay. I am not calling ensureMaxAlignment here,
> because the semantics of ensureMaxAlignment are too strong for this
> situation. Empirically, invoking ensureMaxAlignment seems to ensure %rsp is
> always aligned to particular byte width. The tests
> test/CodeGen/X86/red-zone.ll and test/CodeGen/X86/red-zone.ll expose the
> difference in behavior. When ensureMaxAlignment is called unconditionally,
> the "subq    $4, %rsp - addq    $4, %rsp" pairing becomes "pushq   %rax -
> popq    %rax."
> >
> > The goal of the patch is to implement section 3.2.3 of the AMD64 ABI
> correctly. The controlling sentence is, "The size of each argument gets
> rounded up to eightbytes. Therefore the stack will always be eightbyte
> aligned." The equivalent sentence in the i386 ABI page 37 says, "At all
> times, the stack pointer should point to a word-aligned area." For both
> architectures, the stack pointer is not being rounded up to the nearest
> eightbyte or word between the last normal argument and the first variadic
> argument.
> >
> > I have attached an updated patch adding the AlignStack helper function
> as per your suggestion and a simplified test case based on FileCheck.
> Thanks for your help and suggestions.
> >
> > Tom
> >
> >
> > On Mon, Aug 4, 2014 at 7:44 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > 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
> >
> >
> > <variadicAlignment.patch>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140819/05204e1d/attachment.html>