PR13156: IRGen crash: function declared to take a pointer to a function involving incomplete types

Hi John,

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]

Thoughts?

Thanks,
- David

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. :slight_smile:

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.

(3) non-convertible array types.

-Eli