[cfe-dev] PR13156: IRGen crash: function declared to take a pointer to a function involving incomplete types

[Eli Friedman]

On Mon, Jul 16, 2012 at 4:52 PM, John McCall <rjmccall at apple.com> wrote:
> On Jul 16, 2012, at 2:42 PM, David Blaikie wrote:
>> Richard's helped me working through/debugging PR13156 & we've managed
>> to isolate what appears to be the root cause, but I'm unsure as to the
>> right fix & wanted to get your (& anyone else's) thoughts on the
>> matter.
>>
>> The following C (reproducible in C++ too) code:
>>
>>  struct A;
>>  void fA(struct A (*)(void));
>>  void cA() {
>>    fA(0);
>>  }
>>  struct A {};
>>  void fA(struct A (*t)(void)) {
>>    t();
>>  }
>>
>> crashes in Clang IRGen. The story goes something like this:
>>
>> * the LLVM IR declaration of fA is emitted with a {}* parameter since
>> we don't have enough information to emit the correct void()* type
>> * the type of the parameter is cached in the TypeCache
>> * the TypeCache is not invalidated when struct A becomes defined
>> because struct A is not in the RecordDeclTypes map
>> * since the TypeCache is not invalidated before the definition of fA,
>> IRGen'ing the t() call attempts to make a call to a {}* IR type and
>> asserts (CGCall.cpp:1926 casts to a FunctionType and fails)
>>
>> For normal (non-function) pointer types we correctly walk down through
>> the type and add the types to the RecordDeclTypes map (this can be
>> seen if you add a struct A* parameter to fA and the crash goes away).
>>
>> We could do the same thing for function pointers - finding all the
>> types in parameters and return types and adding those to the
>> RecordDeclTypes map. Is that reasonable? Should we do something more
>> coarse-grained?
>>
>> [This approach is at least viable - with the following simple patch:
>>
>> diff --git lib/CodeGen/CodeGenTypes.cpp lib/CodeGen/CodeGenTypes.cpp
>> index 9a78dae..602c40d 100644
>> --- lib/CodeGen/CodeGenTypes.cpp
>> +++ lib/CodeGen/CodeGenTypes.cpp
>> @@ -452,6 +452,8 @@ llvm::Type *CodeGenTypes::ConvertType(QualType T) {
>>     // function type depends on an incomplete type (e.g. a struct or enum), we
>>     // cannot lower the function type.
>>     if (!isFuncTypeConvertible(FT)) {
>> +      if (const RecordType *RT =
>> dyn_cast<RecordType>(FT->getResultType().getTypePtr()))
>> +        RecordDeclTypes[RT];
>>       // This function's type depends on an incomplete tag type.
>>       // Return a placeholder type.
>>       ResultType = llvm::StructType::get(getLLVMContext());
>>
>> This fixes the return type case, but still crashes if the struct is a
>> parameter to the function pointer instead of its return type - so we'd
>> need to do a bit more work. This work could be done in
>> "isFuncTypeConvertible" (perhaps it would be renamed if it were to
>> take on this purpose), potentially]
>
> This is definitely not the right fix.  The more I look at SkippedLayout,
> however, the more concerned I get.  Notably, SkippedLayout never
> actually gets set to false, so as soon as one thing triggers it, any
> subsequent completion of a record type causes the entire type
> cache to be flushed.
>
> This is all Chris's fault. :)
>
> Okay.  The high-level requirement here is that we should not try
> to cache any type whose computation involved a type that could
> be not be "stably converted".  That should just mean
>   (1) non-convertible function types and
>   (2) incomplete enum types.

(3) non-convertible array types.

-Eli