[cfe-commits] [PATCH] Encoding calling conventions in FunctionTypes

[Charles Davis]

Douglas Gregor wrote:
> Hi Chip,
> 
> On Jan 4, 2010, at 3:13 PM, Charles Davis wrote:
> 
>> Ping...
>>
>> Patch is reattached, rebased against current TOT.
> 
> Thanks for your patience; comments below.
Sorry it took so long. Actually, the reason it took so long is that I
was on vacation :).

Anyway, latest patch reattached.
> 
> With future patches, could you tweak Thunderbird to avoid inline
> attachments? There's rationale + directions here:
> 
>     http://llvm.org/docs/DeveloperPolicy.html#patches
Done.
> 
>> /// isMoreQualifiedThan - Determine whether this type is more
>> /// qualified than the Other type. For example, "const volatile int"
>> /// is more qualified than "const int", "volatile int", and
>> Index: lib/Frontend/PCHWriter.cpp
>> ===================================================================
>> --- lib/Frontend/PCHWriter.cpp    (revision 92538)
>> +++ lib/Frontend/PCHWriter.cpp    (working copy)
>> @@ -139,6 +139,7 @@
>> void PCHTypeWriter::VisitFunctionType(const FunctionType *T) {
>>   Writer.AddTypeRef(T->getResultType(), Record);
>>   Record.push_back(T->getNoReturnAttr());
>> +  Record.push_back(T->getCallConv());
>> }
> 
> could you add a "// FIXME: stable encoding for calling convention" at
> the end of that push_back line? Some day, we'll need to divorce the bit
> encodings of all of our enums from the enum values (but not today!).
Done.
> 
>> Index: lib/AST/ASTContext.cpp
>> ===================================================================
>> --- lib/AST/ASTContext.cpp    (revision 92538)
>> +++ lib/AST/ASTContext.cpp    (working copy)
>> @@ -1681,7 +1696,8 @@
>>
>> /// getFunctionNoProtoType - Return a K&R style C function type like
>> 'int()'.
>> ///
>> -QualType ASTContext::getFunctionNoProtoType(QualType ResultTy, bool
>> NoReturn) {
>> +QualType ASTContext::getFunctionNoProtoType(QualType ResultTy, bool
>> NoReturn,
>> +                                            CallingConv CallConv) {
>>   // Unique functions, to guarantee there is only one function of a
>> particular
>>   // structure.
>>   llvm::FoldingSetNodeID ID;
>> @@ -1694,7 +1710,8 @@
>>
>>   QualType Canonical;
>>   if (!ResultTy.isCanonical()) {
>> -    Canonical = getFunctionNoProtoType(getCanonicalType(ResultTy),
>> NoReturn);
>> +    Canonical = getFunctionNoProtoType(getCanonicalType(ResultTy),
>> NoReturn,
>> +                                       CallConv);
> 
> I'm not convinced that this is the correct formulation of the canonical
> type, because we may have to cope with the fact that the default calling
> convention is the same as the cdecl calling convention (always? for some
> targets? I don't know when). Here's a C++ example that should illustrate
> the issue:
> 
>   void __attribute__((cdecl)) f();
>   void (*fp)() = &f;
> 
> Here, the variable fp has the default calling convention but the its
> initializer, &f, has the cdecl calling convention. G++ accepts this code
> (but rejects similar code where the calling convention of f is stdcall),
> which indicates that it's considering the two types to be equivalent.
> So, we need to make sure that the canonical function type has a
> canonical representation of the calling convention, e.g., with a
> function like
> 
>     CallingConv ASTContext::getCanonicalCallingConv(CallingConv CC) {
>         if (CC == CC_C)
>             return CC_Default;
>         return CC;
>     }
> 
> then, to build the canonical type, use
> 
>     if (!ResultTy.isCanonical() || getCanonicalCallingConv(CallConv) !=
> CallConv)
>         Canonical = getFunctionNoProtoType(getCanonicalType(ResultTy),
> NoReturn, getCanonicalCallingConv(CallConv));
> 
You're absolutely right about that. I can't believe I overlooked that
case. When I added the default calling convention enumerator, I had in
mind that the default calling convention would be the same as C by
default, but could be changed. Apparently, I forgot to canonicalize the
default calling convention.
>> @@ -1746,7 +1764,7 @@
>>     Canonical = getFunctionType(getCanonicalType(ResultTy),
>>                                 CanonicalArgs.data(), NumArgs,
>>                                 isVariadic, TypeQuals, false,
>> -                                false, 0, 0, NoReturn);
>> +                                false, 0, 0, NoReturn, CallConv);
> 
> I have the same comment about using getCanonicalCallingConv here as above.
Done.
> 
>> +static bool isCompatibleCallingConvention(CallingConv lcc,
>> CallingConv rcc) {
>> +  // Here are the rules (as I see them) for compatibility of calling
>> +  // conventions:
>> +  // - If they're the same, they're compatible.
>> +  // - If the first one is not the default and the second one is, then
>> +  //   they're compatible (and the second declaration assumes the
>> calling
>> +  //   convention of the first).
>> +  // - Otherwise, they're not compatible.
>> +  return (lcc == rcc || (lcc != CC_Default && rcc == CC_Default));
>> +}
> 
> The order dependency on lcc/rcc doesn't make sense to me, since the
> definition of compatibility for types is commutative. I think the right
> implementation for this function is to compare the canonical calling
> conventions.
As I said the case I had in mind is this one:

void __attribute__((stdcall)) f(void);
void f(void) {}

or the equivalent MSVC:

void __stdcall f(void);
void f(void) {}

MSVC lets you do that. Oddly enough, I found that GCC doesn't let you do
that. Apparently, clang already handles that, so my concern was
unfounded. (Is it?) So, I changed it to just directly compare the
calling conventions, like you said it should.
> 
> Aside from the canonical-types issue, I think this patch is ready to go
> in. Do you have commit access?
All right! ...But I don't have commit access yet. How do I get it? Or do
you intend to commit this yourself?

Chip

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: callconv-in-types.patch
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100113/2407ab6e/attachment.ksh>