<div dir="ltr"><div><div><div>Hi Duncan,<br></div>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." <br>
<br>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.<br>
<br></div>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.<br><br></div>Tom<br>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Aug 4, 2014 at 7:44 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Usually just replying weekly with the text "ping" and the<br>
reattached patch is your best bet.<br>
<br>
I had a look, and the patch seems reasonable. However, you<br>
should use `FileCheck` [1] instead of `grep` and `count`.<br>
<br>
[1]: <a href="http://llvm.org/docs/CommandGuide/FileCheck.html" target="_blank">http://llvm.org/docs/CommandGuide/FileCheck.html</a><br>
<br>
Also, I find the name `AllocateStack()` confusing in this<br>
context. I think it would be better to extract out a helper<br>
called `AlignStack()` that your new code and `AllocateStack()`<br>
both call.<br>
<br>
Question: why aren't you calling `ensureMaxAlignment()` here?<br>
<div><div class="h5"><br>
> On 2014-Aug-04, at 14:10, Thomas Jablin <<a href="mailto:tjablin@gmail.com">tjablin@gmail.com</a>> wrote:<br>
><br>
> Is there anything I can do to accelerate the review of this patch? I originally submitted it to llvm-commits on June 24.<br>
> Tom<br>
><br>
><br>
> On Mon, Jul 28, 2014 at 8:34 AM, Thomas Jablin <<a href="mailto:tjablin@gmail.com">tjablin@gmail.com</a>> wrote:<br>
> Adding Elena since she most recently touched this function.<br>
><br>
><br>
> On Mon, Jul 28, 2014 at 8:15 AM, Thomas Jablin <<a href="mailto:tjablin@gmail.com">tjablin@gmail.com</a>> wrote:<br>
> This patch still requires a reviewer.<br>
><br>
><br>
> On Mon, Jul 14, 2014 at 4:07 PM, Thomas Jablin <<a href="mailto:tjablin@gmail.com">tjablin@gmail.com</a>> wrote:<br>
> I'm still waiting for a review. This is a patch for PR14792.<br>
> Tom<br>
><br>
><br>
> On Mon, Jun 30, 2014 at 12:34 PM, Thomas Jablin <<a href="mailto:tjablin@gmail.com">tjablin@gmail.com</a>> wrote:<br>
> I'm still waiting for a review. Maybe Nadav since he owns the x86 backend?<br>
><br>
><br>
> On Tue, Jun 24, 2014 at 12:36 PM, Thomas Jablin <<a href="mailto:tjablin@gmail.com">tjablin@gmail.com</a>> wrote:<br>
> Hi All,<br>
> Please find enclosed an updated and simplified patch along with a additional test for properly aligned variadic arguments.<br>
> Tom<br>
><br>
> On Fri, Jul 19, 2013 at 8:36 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>> wrote:<br>
> On Thu, Jul 18, 2013 at 10:31 PM, Yun-Wei Lee <<a href="mailto:lee2041412@gmail.com">lee2041412@gmail.com</a>> wrote:<br>
> > Thank for your suggestion.<br>
> ><br>
> > I have tried to use GetAlignedArgumentStackSize.<br>
> > It is so strange that this function may output uncorrectly.<br>
> > For example, the StackSize will be 40 when handling the third function call<br>
> > in main.<br>
> > If I modified the this function, it will fail one test.<br>
> > I wonder this may be another bug in llvm......<br>
> > And that's why I didn't use GetAlignedArgumentStackSize previously.<br>
> ><br>
> > Thank you!<br>
><br>
> I'm sorry, I'm not that familiar with the code. A brief look tells me<br>
> that it's likely none of the patches are quite right;<br>
> <a href="http://llvm.org/bugs/attachment.cgi?id=10897" target="_blank">http://llvm.org/bugs/attachment.cgi?id=10897</a> looks closer, but still<br>
> not quite right.<br>
><br>
> -Eli<br>
><br>
> ><br>
> > On Thu, Jul 18, 2013 at 7:56 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>><br>
> > wrote:<br>
> >><br>
> >> On Thu, Jul 18, 2013 at 8:36 AM, Yun-Wei Lee <<a href="mailto:lee2041412@gmail.com">lee2041412@gmail.com</a>> wrote:<br>
> >> > <a href="http://llvm.org/bugs/show_bug.cgi?id=14792" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=14792</a><br>
> >> ><br>
> >> > Problem:<br>
> >> > In the i386 ABI Page 3-10, it said that the stack is aligned. However,<br>
> >> > the<br>
> >> > two example code show that does not handle the alignment correctly when<br>
> >> > using variadic function. For example, if the size of the first argument<br>
> >> > is<br>
> >> > 17, the overflow_arg_area in va_list will be set to "address of first<br>
> >> > argument + 16" instead of "address of first argument + 24" after calling<br>
> >> > va_start.<br>
> >> > In addition, #6636 showed the same problem because in AMD64, arguments<br>
> >> > is<br>
> >> > passed by register at first, then pass by memory when run out of<br>
> >> > register<br>
> >> > (AMD64 ABI 3.5.7 rule 10).<br>
> >> ><br>
> >> > Why this problem happened?<br>
> >> > When calling va_start to set va_list, overflow_arg_area is not set<br>
> >> > correctly. To set the overflow_arg_area correctly, we need to get the<br>
> >> > FrameIndex correctly. Now, here comes the problem, llvm doesn't handle<br>
> >> > it<br>
> >> > correctly. It accounts for StackSize to compute the FrameIndex, and if<br>
> >> > the<br>
> >> > StackSize is not aligned, it will compute the wrong FrameIndex. As a<br>
> >> > result<br>
> >> > overflow_arg_area will not be set correctly.<br>
> >> ><br>
> >> > My Solution:<br>
> >> > 1. Record the Align if it is located in Memory.<br>
> >> > 2. If it is variadic function and needs to set FrameIndex, adjust the<br>
> >> > stacksize.<br>
> >><br>
> >> Please read <a href="http://llvm.org/docs/DeveloperPolicy.html" target="_blank">http://llvm.org/docs/DeveloperPolicy.html</a> . In<br>
> >> particular, patches should be sent to llvm-commits, and patches should<br>
> >> generally include a regression test.<br>
> >><br>
> >> In terms of the code, you might want to consider using<br>
> >> llvm::RoundUpToAlignment.<br>
> >><br>
> >> -Eli<br>
> ><br>
> ><br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
</div></div>> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote></div><br></div>