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

[John McCall]

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.
Would you mind trying out a patch which propagates this information
out and decides not to cache in those situations?  You should be
able to remove SkippedLayout completely.

John.