[cfe-commits] Bug 11709 Fix: va_list on ARM is not following AAPCS 7.1.4

[Logan Chien]

LGTM.  Any further comments?

BTW, Weiming, do you have the commit access?

Logan

On Sat, Oct 6, 2012 at 1:10 AM, Weiming Zhao <weimingz at codeaurora.org>wrote:

> Hi Jordan,****
>
> ** **
>
> Thanks for reviewing.****
>
> **1.       **Sema.h is removed.****
>
> **2.       **I glad to find that we don’t need the workaround for the
> exception. Someone fixed something in clang. So we don’t need to relax the
> assertion.****
>
> **3.       **Fixed. To be consistent with “IsThumb”, I name the variable
>  “IsAAPCSS”. But I can’t completely decide it as constructor because
> setABI() will change the ABI. For consistency, the variable name.****
>
> ** **
>
> Please help to review the attached patch.****
>
> ** **
>
> Thanks,****
>
> Weiming****
>
> ** **
>
> ** **
>
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
> by The Linux Foundation****
>
> ** **
>
> *From:* Jordan Rose [mailto:jordan_rose at apple.com]
> *Sent:* Thursday, October 04, 2012 6:19 PM
> *To:* Logan Chien
> *Cc:* Weiming Zhao; cfe-commits at cs.uiuc.edu
>
> *Subject:* Re: [cfe-commits] Bug 11709 Fix: va_list on ARM is not
> following AAPCS 7.1.4****
>
> ** **
>
> A couple comments (sorry for the long long delay):****
>
> ** **
>
> - AST can't depend on Sema. Is there a reason for the include of Sema.h in
> ASTContext.cpp?****
>
> ** **
>
> - It would be a good idea to leave a comment in ExprClassification.cpp
> about why __va_list needs an exception.****
>
> ** **
>
> - For ARMTargetInfo::getBuiltinVaListKind, maybe you can just decide this
> at construction time and stick it in a local variable? 'unsigned usesAAPCS
> : 1'?****
>
> ** **
>
> Thanks for working on this, Weiming, Logan.****
>
> Jordan****
>
> ** **
>
> ** **
>
> On Oct 2, 2012, at 22:34 , Logan Chien <tzuhsiang.chien at gmail.com> wrote:*
> ***
>
>
>
> ****
>
> LGTM.
>
> However, I think we can enhance the test cases a little.
> 1. Keep the void* test in builtins-arm.c for apcs-gnu ABI.
> 2. Add the unit test for va_list name mangling.
>
> Sincerely,
> Logan
>
> ps. The attached patch is the revised patch with the updated
> test cases.****
>
> On Thu, Sep 20, 2012 at 6:13 AM, Weiming Zhao <weimingz at codeaurora.org>
> wrote:****
>
> Ping...
>
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
> by
> The Linux Foundation****
>
>
> <mangle-valist.cpp><builtins-arm.c><0001-Bug-11709-Change-the-definition-of-va_list-to-meet-A.patch>
> ****
>
> ** **
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121009/85a0734b/attachment.html>