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

[Douglas Gregor]

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.

With future patches, could you tweak Thunderbird to avoid inline  
attachments? There's rationale + directions here:

	http://llvm.org/docs/DeveloperPolicy.html#patches

> /// 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!).

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

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

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

Aside from the canonical-types issue, I think this patch is ready to  
go in. Do you have commit access?

	- Doug