From 9f9ccfcdb94ee95416b36bf0c32037457293536f Mon Sep 17 00:00:00 2001 From: David Tarditi Date: Thu, 26 Apr 2018 13:45:15 -0700 Subject: [PATCH] Fix compiler crash rewriting return function pointer type. (#483) Summary: The compiler hit an assert while rewrting types with bounds-safe interfaces to make them checked types. This problem was found by trying to call the signal functionin a checked scope. The problem occurred when a function return type was a function pointer type with a bounds-safe interface. The construction of new type location information asserted about a mismatch in the expected size of the type location buffer. The problem was that we were rewriting parameter and return types and then modifying types using interop type information. We need to do this in the opposite order to construct new type location information properly. Detailed explanation: Clang has a binary serialization of type locations, where it expects source locations for types that are components of a type to be serialized as part of the type location information for the enclosing type. For types with type location information, the representation divides data into "local data" - data specific to the type and "child data" (type location information for the child). The local data is followed by the child data when laying out data. The local data may be variable length. The code is in include\clang\ast\TypeLoc.h. See getInnerTypeLoc() for the computation of this data layout. During tree transforms, if types are rewritten, the transform maintains this serialization. That takes some work because the tree transform typically operates bottom-up: transform child nodes of an AST node and then transform the AST node. This means that the type location information for a child type node is rewritten before the parent type node information (which is variable length and may change the position of the child node's type location). The solution is to use a type location builder: the data for a child node is accumulated into the type location builder. For a parent node, the type location build buffer is expanded if necessary. The data for the child node is moved to the back of the buffer. The problem was that the type location information for a child type was out-of-sync with what was expected, if we replaced the child type with a new type. based on bounds-safe information. The specific child node in question was the return type. The solution is to choose return type, generate the type location information by recursively transforming the return type, and then filling in the data for the parent (the function prototype). Testing: - Add tests in the Checked C repo of calls in checked scopes to functions that return pointer types with bounds-safe interfaces. - Passed local testing on Windows. - Passed automated testing on Linux. --- lib/Sema/CheckedCInterop.cpp | 129 +++++++++++++++++++---------------- lib/Sema/TreeTransform.h | 10 +++ 2 files changed, 80 insertions(+), 59 deletions(-) diff --git a/lib/Sema/CheckedCInterop.cpp b/lib/Sema/CheckedCInterop.cpp index cb80f56e75b4..fd4c22131207 100644 --- a/lib/Sema/CheckedCInterop.cpp +++ b/lib/Sema/CheckedCInterop.cpp @@ -77,17 +77,12 @@ class TransformFunctionTypeToChecked : typedef TreeTransform BaseTransform; - // This method has been copied from TreeTransform.h and modified to add - // an additional rewrite step that updates parameter/return types based - // on bounds information. - // - // This code has been specialized to assert on trailing returning types - // instead of handling them. That's a C++ feature that we could not test - // for now. The code could be added back later. - + // This code has been copied from TreeTransform.h and modified to first + // update parameter/return types based on bounds annotations. public: TransformFunctionTypeToChecked(Sema &SemaRef) : BaseTransform(SemaRef) {} + QualType TransformFunctionProtoType(TypeLocBuilder &TLB, FunctionProtoTypeLoc TL) { SmallVector ExceptionStorage; @@ -99,58 +94,40 @@ class TransformFunctionTypeToChecked : ExceptionStorage, Changed); }); } + template QualType TransformFunctionProtoType( TypeLocBuilder &TLB, FunctionProtoTypeLoc TL, CXXRecordDecl *ThisContext, unsigned ThisTypeQuals, Fn TransformExceptionSpec) { - // First rewrite any subcomponents so that nested function types are - // handled. - // Transform the parameters and return type. - // - // We are required to instantiate the params and return type in source order. - // - SmallVector ParamTypes; - SmallVector ParamDecls; - SmallVector ParamAnnots; - Sema::ExtParameterInfoBuilder ExtParamInfos; const FunctionProtoType *T = TL.getTypePtr(); - QualType ResultType; + // Assert on trailing returning type for now, instead of handling them. + // That's a C++ feature that we cannot test right now. if (T->hasTrailingReturn()) { assert("Unexpected trailing return type for Checked C"); return QualType(); } - ResultType = getDerived().TransformType(TLB, TL.getReturnLoc()); - if (ResultType.isNull()) - return QualType(); - - if (getDerived().TransformFunctionTypeParams( - TL.getBeginLoc(), TL.getParams(), - TL.getTypePtr()->param_type_begin(), - T->getExtParameterInfosOrNull(), - ParamTypes, &ParamDecls, ExtParamInfos)) - return QualType(); - + // Update the parameters and return types to be their interop types. Also + // update the extend prototype inofmration to remove interop type annotations. + SmallVector CurrentParamTypes; + SmallVector CurrentParamAnnots; + TypeLoc ResultLoc = TL.getReturnLoc(); FunctionProtoType::ExtProtoInfo EPI = T->getExtProtoInfo(); bool EPIChanged = false; - if (getDerived().TransformExtendedParameterInfo(EPI, ParamTypes, ParamAnnots, - ExtParamInfos, TL, - TransformExceptionSpec, - EPIChanged)) - return QualType(); - // Now rewrite types based on interop type information, and remove - // the interop types. - const BoundsAnnotations Annots = EPI.ReturnAnnots; - if (!Annots.IsEmpty()) { - if (ResultType->isUncheckedPointerType()) { - InteropTypeExpr *IT = Annots.getInteropTypeExpr(); - BoundsExpr *Bounds = Annots.getBoundsExpr(); + CurrentParamTypes.append(TL.getTypePtr()->param_type_begin(), + TL.getTypePtr()->param_type_end()); + + // Update return type information. + if (!EPI.ReturnAnnots.IsEmpty()) { + if (ResultLoc.getType()->isUncheckedPointerType()) { + InteropTypeExpr *IT = EPI.ReturnAnnots.getInteropTypeExpr(); + BoundsExpr *Bounds = EPI.ReturnAnnots.getBoundsExpr(); assert(Bounds == nullptr || (Bounds != nullptr && IT)); if (IT) { - ResultType = IT->getType(); + ResultLoc = IT->getTypeInfoAsWritten()->getTypeLoc(); #if TRACE_INTEROP llvm::outs() << "return bounds = "; if (Bounds) @@ -160,12 +137,8 @@ class TransformFunctionTypeToChecked : llvm::outs() << "interop type = "; IT->dump(llvm::outs()); llvm::outs() << "\nresult type = "; - ResultType.dump(llvm::outs()); + ResultLoc.getType().dump(llvm::outs()); #endif - // The types are structurally identical except for the checked bit, - // so the type location information can still be used. - TLB.TypeWasModifiedSafely(ResultType); - // Construct new annotations that do not have the bounds-safe interface type. if (Bounds) { EPI.ReturnAnnots = BoundsAnnotations(Bounds, nullptr); @@ -176,13 +149,17 @@ class TransformFunctionTypeToChecked : } } + // Update the parameter types and annotations. The EPI parameter array is still used + // by the original type, so a create and update a copy in CurrentParamAnnots. if (EPI.ParamAnnots) { // Track whether there are parameter annotations left after removing interop // annotations. bool hasParamAnnots = false; - for (unsigned int i = 0; i < ParamTypes.size(); i++) { - BoundsAnnotations IndividualAnnots = ParamAnnots[i]; - if (ParamTypes[i]->isUncheckedPointerType() && + for (unsigned int i = 0; i < CurrentParamTypes.size(); i++) { + BoundsAnnotations IndividualAnnots = EPI.ParamAnnots[i]; + CurrentParamAnnots.push_back(IndividualAnnots); + + if (CurrentParamTypes[i]->isUncheckedPointerType() && IndividualAnnots.getInteropTypeExpr()) { InteropTypeExpr *IT = IndividualAnnots.getInteropTypeExpr(); QualType ParamType = IT->getType(); @@ -190,7 +167,7 @@ class TransformFunctionTypeToChecked : #if TRACE_INTEROP llvm::outs() << "encountered null parameter type with bounds"; llvm::outs() << "\noriginal param type = "; - ParamTypes[i]->dump(llvm::outs()); + CurrentParamTypes[i]->dump(llvm::outs()); llvm::outs() << "\nparam interop type is:"; IT->dump(llvm::outs()); llvm::outs() << "\n"; @@ -200,16 +177,16 @@ class TransformFunctionTypeToChecked : } if (ParamType->isArrayType()) ParamType = SemaRef.Context.getDecayedType(ParamType); - ParamTypes[i] = ParamType; + CurrentParamTypes[i] = ParamType; // Remove the interop type annotation. BoundsExpr *Bounds = IndividualAnnots.getBoundsExpr(); if (IT->getType()->isCheckedArrayType() && !Bounds) Bounds = SemaRef.CreateCountForArrayType(IT->getType()); if (Bounds) { hasParamAnnots = true; - ParamAnnots[i] = BoundsAnnotations(Bounds, nullptr); + CurrentParamAnnots[i] = BoundsAnnotations(Bounds, nullptr); } else - ParamAnnots[i] = BoundsAnnotations(); + CurrentParamAnnots[i] = BoundsAnnotations(); EPIChanged = true; } else { if (!IndividualAnnots.IsEmpty()) @@ -217,12 +194,47 @@ class TransformFunctionTypeToChecked : } } - // If there are no parameter bounds left, null out the pointer to the - // param annotations array. - if (!hasParamAnnots) + if (hasParamAnnots) + EPI.ParamAnnots = CurrentParamAnnots.data(); + else + // If there are no parameter annotations left, null out the pointer to + // the param annotations array. EPI.ParamAnnots = nullptr; } + // Now transform the parameter/return types, parameter declarations, and + // the EPI. + + // For the type location information, we must transform the return type + // before pushing the function prototype type location record. + + // These hold the transformed results. + SmallVector ParamTypes; + SmallVector ParamDecls; + Sema::ExtParameterInfoBuilder ExtParamInfos; + // ParamAnnotsStorage is pre-allocated storage that is used when updating EPI + // in TransformExtendedParameterInfo. Its lifetime must last until the end of + // the lifetimie of EPI. + SmallVector ParamAnnotsStorage; + + QualType ResultType = getDerived().TransformType(TLB, ResultLoc); + if (ResultType.isNull()) + return QualType(); + + // Places transformed data ParamTypes, ParamDecls, and ExtParamInfos. + if (getDerived().TransformFunctionTypeParams( + TL.getBeginLoc(), TL.getParams(), + CurrentParamTypes.begin(), + T->getExtParameterInfosOrNull(), + ParamTypes, &ParamDecls, ExtParamInfos)) + return QualType(); + + if (getDerived().TransformExtendedParameterInfo(EPI, ParamTypes, ParamAnnotsStorage, + ExtParamInfos, TL, + TransformExceptionSpec, + EPIChanged)) + return QualType(); + // Rebuild the type if something changed. QualType Result = TL.getType(); if (getDerived().AlwaysRebuild() || ResultType != T->getReturnType() || @@ -235,7 +247,6 @@ class TransformFunctionTypeToChecked : return QualType(); } } - FunctionProtoTypeLoc NewTL = TLB.push(Result); NewTL.setLocalRangeBegin(TL.getLocalRangeBegin()); NewTL.setLParenLoc(TL.getLParenLoc()); diff --git a/lib/Sema/TreeTransform.h b/lib/Sema/TreeTransform.h index 6479c1eec5db..8c12bea58a10 100644 --- a/lib/Sema/TreeTransform.h +++ b/lib/Sema/TreeTransform.h @@ -628,6 +628,16 @@ class TreeTransform { /// The result vectors should be kept in sync; null entries in the /// variables vector are acceptable. /// + /// Inputs: Params, ParamTypes, and ParamInfos are the inputs to the + /// method. + /// + /// Output: PTypes, PVars, and PInfos are the outputs of the method. + /// The updated parameter types, param var declarations, and PInfo + /// are stored in the method. + /// + /// For correctness, the inputs and outputs shoudl be disjoint data + /// structures. + /// /// Return true on error. bool TransformFunctionTypeParams( SourceLocation Loc, ArrayRef Params,