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

[Jordan Rose]

LGTM as well.

On Oct 8, 2012, at 19:02 , Logan Chien <tzuhsiang.chien at gmail.com> wrote:

> 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>
> 
>  
> 
>