[PATCH] Emit diagnostic on illegal calling convention

[Alexander Zinenko]

Hi John

Sure, this code should belong to SemaDeclAttr. It required several not very
elegant changes to make it work correctly in some places.
I also removed 'if'-ed dyn_cast to FieldDecl used just to get type since it
is inherited from ValueDecl and should be covered in the previous 'if'.


On 29 March 2013 03:20, John McCall <rjmccall at apple.com> wrote:

> On Mar 28, 2013, at 1:27 PM, Alexander Zinenko <ftynse at gmail.com> wrote:
>
> On 28 March 2013 20:07, John McCall <rjmccall at apple.com> wrote:
>
>> On Mar 28, 2013, at 10:57 AM, Alexander Zinenko <ftynse at gmail.com> wrote:
>>
>> On 23 March 2013 01:22, Alexander Zinenko <ftynse at gmail.com> wrote:
>>
>>> Hi John, João!
>>>
>>> This patch adds a diagnostic if the following declarations have illegal
>>> calling conventions:
>>> * pointer to function, reference to function, pointer to member function
>>> variable (or function attribute) declaration;
>>> * function declaration;
>>> * typedef declaration.
>>>
>>> It was proposed in PR13457 discussion (
>>> http://llvm.org/bugs/show_bug.cgi?id=13457#c22).
>>>
>>> We already have such error for variadic functions declared with
>>> CC_X86Fastcall that is emitted from SemaType.  But microsoft ABI has
>>> different allowed CCs depending on whether it is a member function and it
>>> can't be determined given FunctionType only (AFAIK, standard doesn't
>>> differentiate between free function and member function types).  So this
>>> checking has to be done on declarations.
>>>
>>> With this patch, allowed calling conventions are ABI-specific. Therefore
>>> if we teach affected ABIs to disallow fastcall on variadic functions, we
>>> can remove the previous diagnostic.
>>>
>>> Please review!
>>>
>>> --
>>> Alex
>>>
>>
>> <callconv.patch>
>>
>>
>> I would only pass a flag distinguishing non-static member functions vs.
>> everything else.
>>
>
> It makes perfect sense in ABI part, fixed.
> But I left separate flags in the function that emits diagnostic since it
> allows to make it more precise.  Namely to distinguish between __thiscall
> used on non-member function and on static member function.  It is difficult
> to do otherwise while keeping the single diagnostic message because we
> would have to know the reason why check failed rather than just
> passed-or-not flag.
>
>
>>
>> +  if (T->isFunctionProtoType())
>> +    ProtoType = T->getAs<FunctionProtoType>();
>> +  else if (T->isFunctionPointerType())
>> +    ProtoType = T->getPointeeType()->getAs<FunctionProtoType>();
>> +  else if (T->isReferenceType()) {
>> +    QualType RefType = T->getAs<ReferenceType>()->getPointeeType();
>> +    ProtoType = RefType->getAs<FunctionProtoType>();
>> +  } else if (T->isMemberFunctionPointerType()) {
>> +    QualType PointerType =
>> T->getAs<MemberPointerType>()->getPointeeType();
>>
>> Test the result of getAs<>, please.
>>
> Done.
>
>
> Is there any reasonable way we can tie this to applying an explicit CC
> attribute instead of putting explicit checks in a ton of places?  It's not
> so much that I'm concerned about the compile-time impact of checking this
> in a bunch of places (although there is that).  It's that there's really no
> earthly way that the three or four places you've found to insert a check
> for this are really covering all the cases, and it seems unlikely that
> you'll reasonably be able to find them all.  (e.g., what if a
> pointer-to-function appears at an arbitrary position within some other
> type?)
>
> John.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130409/d2d50749/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: callconv.patch
Type: application/octet-stream
Size: 17598 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130409/d2d50749/attachment.obj>