Skip to content

Commit

Permalink
Fix compiler crash rewriting return function pointer type. (#483)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@dtarditi
dtarditi authored Apr 26, 2018
1 parent 161847c commit 9f9ccfc
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 59 deletions.
129 changes: 70 additions & 59 deletions lib/Sema/CheckedCInterop.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,17 +77,12 @@ class TransformFunctionTypeToChecked :

typedef TreeTransform<TransformFunctionTypeToChecked> 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<QualType, 4> ExceptionStorage;
Expand All @@ -99,58 +94,40 @@ class TransformFunctionTypeToChecked :
ExceptionStorage, Changed);
});
}

template<typename Fn>
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<QualType, 4> ParamTypes;
SmallVector<ParmVarDecl*, 4> ParamDecls;
SmallVector<BoundsAnnotations, 4> 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<QualType, 4> CurrentParamTypes;
SmallVector<BoundsAnnotations, 4> 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)
Expand All @@ -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);
Expand All @@ -176,21 +149,25 @@ 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();
if (ParamType.isNull()) {
#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";
Expand All @@ -200,29 +177,64 @@ 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())
hasParamAnnots = true;
}
}

// 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<QualType, 4> ParamTypes;
SmallVector<ParmVarDecl*, 4> 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<BoundsAnnotations, 4> 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() ||
Expand All @@ -235,7 +247,6 @@ class TransformFunctionTypeToChecked :
return QualType();
}
}

FunctionProtoTypeLoc NewTL = TLB.push<FunctionProtoTypeLoc>(Result);
NewTL.setLocalRangeBegin(TL.getLocalRangeBegin());
NewTL.setLParenLoc(TL.getLParenLoc());
Expand Down
10 changes: 10 additions & 0 deletions lib/Sema/TreeTransform.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ParmVarDecl *> Params,
Expand Down

0 comments on commit 9f9ccfc

Please sign in to comment.