[PATCH] Emit diagnostic on illegal calling convention

[John McCall]

On Apr 17, 2013, at 6:27 AM, Alexander Zinenko <ftynse at gmail.com> wrote:
> Thanks for the review, I have to provide some explanation (see below).
> Which changes do you think are worth making then?
> 
> On 17 April 2013 03:12, John McCall <rjmccall at apple.com> wrote:
>> 
>> 
>> This is the right general idea, thanks!
>> 
>> +  int ChunkIndex = state.getCurrentChunkIndex();
>> +  while (ChunkIndex != 0 &&
>> +         D.getTypeObject(ChunkIndex).Kind == DeclaratorChunk::Paren)
>> +    ChunkIndex--;
>> 
>> Isn't the current ChunkIndex always a DeclaratorChunk::Function?
> 
> Nope, it can be DeclaratorChunk::Paren for typedefs, for example:
> typedef (__stdcall *ptr)();
> will have pointer, paren and function chunks and the checking is called on paren
> or for variable declarations, for example
> void (*__thiscall foo)();
> and the checking is called on pointer.

Okay, so we don't actually move the CC attribute effectively.  Good to know.

>> I think you want D.getTypeObject(ChunkIndex - 1) here, although I guess
>> I might be off-by-one somehow.
> 
> if (D.getTypeObject(ChunkIndex).Kind == DeclaratorChunk::Paren)
>  --ChunkIndex;
> works for all my tests and frankly I can't figure out a declarator
> with two consecutive paren chunks. However, most places with similar
> analysis in SemaType allow skipping multiple consecutive paren chunks.

You wouldn't normally write it, but this is totally legal:
  typedef ((((__stdcall (*((ptr)))))()));

>> +  bool IsCXXMethod =
>> +      D.getTypeObject(ChunkIndex).Kind == DeclaratorChunk::MemberPointer;
>> +
>> +  // Non-friend functions declared in member context are methods.
>> +  IsCXXMethod = IsCXXMethod || (D.getContext() == Declarator::MemberContext
>> +                                && D.isFunctionDeclarator()
>> +                                && !D.getDeclSpec().isFriendSpecified());
>> 
>> I think the right thing to do here is more like:
>>  bool IsCXXMethod;
>>  if (ChunkIndex == 0) { // CC applies to the function being declared
>>    assert(D.isFunctionDeclarator());
> 
> This is not necessarily a function declarator, consider for example
> variable decl
> void (*__fastcall fastpfunc)();
> chunks are: pointer, paren, function and the checking is called for
> the the first one.

Yeah, this is a consequence of my misunderstanding about moving the
attribute.

Okay, so here's what you should do:
  - If we're on a function declarator chunk right now, walk "inwards".
  - Keep walking inwards past an arbitrary number of parens.
  - If you run out of declarator chunks, this is a function declarator, and you should check the declaring context, whether this is a friend, whether it's static, etc.
  - Otherwise, if it's a member-pointer chunk, this is an instance method context.
  - Otherwise, it's a "normal" function pointer context.

>>    IsCXXMethod = (D.getContext() == Declarator::MemberContext && !D.getDeclSpec().isFriendSpecified() && D.getDeclSpec().getStorageClassSpecifier() != SC_Static);
>>  } else {
>>    IsCXXMethod = D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::MemberPointer;
>>  }
>> 
>> For out-of-line method declarations, we should just be able rely on redeclaration checking.
> 
> It looks like at this point we don't have redeclaration info yet.

Yes, I know.  What I mean is that we can get away with just checking for explicit storage-class specifiers, friend specifiers, etc. because, if this is an out-of-line method definition, we won't be able to accept an illegal CC anyway because it won't match the CC from the original declaration.

> On the other hand, friend declarations will not be handled correctly
> this way since we can no longer differentiate between friend free
> functions and friend methods from other classes. And we have no
> diagnostic if friend declaration uses different CC than original
> declaration.

Really?  That's a bug.

John.