[CGTypes] Remove SkippedLayout flag

[nikic]

Summary

This is a followup to D152999. I don't believe this flag is needed anymore now that the recursion detection is gone. We have separate mechanisms (UpdateCompletedType, RefreshTypeCacheForClass) to deal with incomplete types and similar.

[efriedma]

The following miscompiles with this patch:

struct S;
extern S a[10];
S(*b)[10] = &a;
struct S{int x;};
int f() { return a[3].x; }

The following also behaves a bit strangely:

struct S;
void f(S);
void (*ff)(S) = &f;
struct S {int x;};
void f2(S);
void (*ff2)(S) = &f2;

With your patch, the signature of f2() changes. Granted, not sure how important that is; with opaque pointers, we don't actually use the signatures of function declarations for anything.

clang/lib/CodeGen/CodeGenTypes.cpp

272

FunctionsBeingProcessed can probably also be killed off? With opaque pointers, a function signature can never refer to another function type. (Only calls and functions declarations involve function types.)

[nikic]

I've added the first test case in https://github.com/llvm/llvm-project/commit/e98cbb95b8b96d3908a808bbcd639680a5197428, to make sure this doesn't regress.

Going to abandon this patch, as we clearly can't drop SkippedLayout, or at least not entirely.

[efriedma]

We can probably make this a bit more readable/efficient by adding a mechanism for function/array types similar to RecordsWithOpaqueMemberPointers.

clang/lib/CodeGen/CodeGenTypes.h

class CodeGenTypes {
   /// Contains the LLVM IR type for any converted RecordDecl.
   llvm::DenseMap<const Type*, llvm::StructType *> RecordDeclTypes;
 
   /// Hold memoized CGFunctionInfo results.
   llvm::FoldingSet<CGFunctionInfo> FunctionInfos{FunctionInfosLog2InitSize};
 
   llvm::SmallPtrSet<const CGFunctionInfo*, 4> FunctionsBeingProcessed;
 
-  /// True if we didn't layout a function due to a being inside
-  /// a recursive struct conversion, set this to true.
-  bool SkippedLayout;
-
   /// This map keeps cache of llvm::Types and maps clang::Type to
   /// corresponding llvm::Type.
   llvm::DenseMap<const Type *, llvm::Type *> TypeCache;
 
   llvm::DenseMap<const Type *, llvm::Type *> RecordsWithOpaqueMemberPointers;
 
   static constexpr unsigned FunctionInfosLog2InitSize = 9;
   /// Helper for ConvertType.

clang/lib/CodeGen/CodeGenTypes.cpp

 
 using namespace clang;
 using namespace CodeGen;
 
 CodeGenTypes::CodeGenTypes(CodeGenModule &cgm)
   : CGM(cgm), Context(cgm.getContext()), TheModule(cgm.getModule()),
     Target(cgm.getTarget()), TheCXXABI(cgm.getCXXABI()),
     TheABIInfo(cgm.getTargetCodeGenInfo().getABIInfo()) {
-  SkippedLayout = false;
 }
 
 CodeGenTypes::~CodeGenTypes() {
   for (llvm::FoldingSet<CGFunctionInfo>::iterator
        I = FunctionInfos.begin(), E = FunctionInfos.end(); I != E; )
     delete &*I++;
 }
 
if (!isFuncTypeConvertible(FT)) {
     // we re-convert the FunctionType when appropriate.
     if (const RecordType *RT = FT->getReturnType()->getAs<RecordType>())
       ConvertRecordDeclType(RT->getDecl());
     if (const FunctionProtoType *FPT = dyn_cast<FunctionProtoType>(FT))
       for (unsigned i = 0, e = FPT->getNumParams(); i != e; i++)
         if (const RecordType *RT = FPT->getParamType(i)->getAs<RecordType>())
           ConvertRecordDeclType(RT->getDecl());
 
-    SkippedLayout = true;
-
     // Return a placeholder type.
     return llvm::StructType::get(getLLVMContext());
   }
 
   // The function type can be built; call the appropriate routines to
   // build it.
   const CGFunctionInfo *FI;
   if (const FunctionProtoType *FPT = dyn_cast<FunctionProtoType>(FT)) {
     FI = &arrangeFreeFunctionType(
         CanQual<FunctionProtoType>::CreateUnsafe(QualType(FPT, 0)));
   } else {
     const FunctionNoProtoType *FNPT = cast<FunctionNoProtoType>(FT);
     FI = &arrangeFreeFunctionType(
         CanQual<FunctionNoProtoType>::CreateUnsafe(QualType(FNPT, 0)));
   }
 
   llvm::Type *ResultType = nullptr;
   // If there is something higher level prodding our CGFunctionInfo, then
   // don't recurse into it again.
   if (FunctionsBeingProcessed.count(FI)) {
 
     ResultType = llvm::StructType::get(getLLVMContext());
-    SkippedLayout = true;
   } else {
 
     // Otherwise, we're good to go, go ahead and convert it.
     ResultType = GetFunctionType(*FI);
   }
 
   return ResultType;
 }
#include "clang/AST/BuiltinTypes.def"
   }
   case Type::IncompleteArray: {
     const IncompleteArrayType *A = cast<IncompleteArrayType>(Ty);
     assert(A->getIndexTypeCVRQualifiers() == 0 &&
            "FIXME: We only handle trivial array types so far!");
     // int X[] -> [0 x int], unless the element type is not sized.  If it is
     // unsized (e.g. an incomplete struct) just use [0 x i8].
     ResultType = ConvertTypeForMem(A->getElementType());
-    if (!ResultType->isSized()) {
-      SkippedLayout = true;
+    if (!ResultType->isSized())
       ResultType = llvm::Type::getInt8Ty(getLLVMContext());
-    }
     ResultType = llvm::ArrayType::get(ResultType, 0);
     break;
   }
   case Type::ConstantArray: {
     const ConstantArrayType *A = cast<ConstantArrayType>(Ty);
     llvm::Type *EltTy = ConvertTypeForMem(A->getElementType());
 
     // Lower arrays of undefined struct type to arrays of i8 just to have a
     // concrete type.
-    if (!EltTy->isSized()) {
-      SkippedLayout = true;
+    if (!EltTy->isSized())
       EltTy = llvm::Type::getInt8Ty(getLLVMContext());
-    }
 
     ResultType = llvm::ArrayType::get(EltTy, A->getSize().getZExtValue());
     break;
   }
   case Type::ExtVector:
   case Type::Vector: {
     const auto *VT = cast<VectorType>(Ty);
     // An ext_vector_type of Bool is really a vector of bits.
for (const auto &I : CRD->bases()) {
       ConvertRecordDeclType(I.getType()->castAs<RecordType>()->getDecl());
     }
   }
 
   // Layout fields.
   std::unique_ptr<CGRecordLayout> Layout = ComputeRecordLayout(RD, Ty);
   CGRecordLayouts[Key] = std::move(Layout);
 
-  // If this struct blocked a FunctionType conversion, then recompute whatever
-  // was derived from that.
-  // FIXME: This is hugely overconservative.
-  if (SkippedLayout)
-    TypeCache.clear();
-
   return Ty;
 }
 
 /// getCGRecordLayout - Return record layout info for the given record decl.
 const CGRecordLayout &
 CodeGenTypes::getCGRecordLayout(const RecordDecl *RD) {
   const Type *Key = Context.getTagDeclType(RD).getTypePtr();