Skip to content

Commit 9f9ccfc

Browse files
dtarditidtarditi
authored
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.
1 parent 161847c commit 9f9ccfc

File tree

2 files changed

+80
-59
lines changed

2 files changed

+80
-59
lines changed

‎lib/Sema/CheckedCInterop.cpp

Lines changed: 70 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -77,17 +77,12 @@ class TransformFunctionTypeToChecked :
7777

7878
typedef TreeTransform<TransformFunctionTypeToChecked> BaseTransform;
7979

80-
// This method has been copied from TreeTransform.h and modified to add
81-
// an additional rewrite step that updates parameter/return types based
82-
// on bounds information.
83-
//
84-
// This code has been specialized to assert on trailing returning types
85-
// instead of handling them. That's a C++ feature that we could not test
86-
// for now. The code could be added back later.
87-
80+
// This code has been copied from TreeTransform.h and modified to first
81+
// update parameter/return types based on bounds annotations.
8882
public:
8983
TransformFunctionTypeToChecked(Sema &SemaRef) : BaseTransform(SemaRef) {}
9084

85+
9186
QualType TransformFunctionProtoType(TypeLocBuilder &TLB,
9287
FunctionProtoTypeLoc TL) {
9388
SmallVector<QualType, 4> ExceptionStorage;
@@ -99,58 +94,40 @@ class TransformFunctionTypeToChecked :
9994
ExceptionStorage, Changed);
10095
});
10196
}
97+
10298
template<typename Fn>
10399
QualType TransformFunctionProtoType(
104100
TypeLocBuilder &TLB, FunctionProtoTypeLoc TL, CXXRecordDecl *ThisContext,
105101
unsigned ThisTypeQuals, Fn TransformExceptionSpec) {
106-
// First rewrite any subcomponents so that nested function types are
107-
// handled.
108102

109-
// Transform the parameters and return type.
110-
//
111-
// We are required to instantiate the params and return type in source order.
112-
//
113-
SmallVector<QualType, 4> ParamTypes;
114-
SmallVector<ParmVarDecl*, 4> ParamDecls;
115-
SmallVector<BoundsAnnotations, 4> ParamAnnots;
116-
Sema::ExtParameterInfoBuilder ExtParamInfos;
117103
const FunctionProtoType *T = TL.getTypePtr();
118104

119-
QualType ResultType;
105+
// Assert on trailing returning type for now, instead of handling them.
106+
// That's a C++ feature that we cannot test right now.
120107
if (T->hasTrailingReturn()) {
121108
assert("Unexpected trailing return type for Checked C");
122109
return QualType();
123110
}
124111

125-
ResultType = getDerived().TransformType(TLB, TL.getReturnLoc());
126-
if (ResultType.isNull())
127-
return QualType();
128-
129-
if (getDerived().TransformFunctionTypeParams(
130-
TL.getBeginLoc(), TL.getParams(),
131-
TL.getTypePtr()->param_type_begin(),
132-
T->getExtParameterInfosOrNull(),
133-
ParamTypes, &ParamDecls, ExtParamInfos))
134-
return QualType();
135-
112+
// Update the parameters and return types to be their interop types. Also
113+
// update the extend prototype inofmration to remove interop type annotations.
114+
SmallVector<QualType, 4> CurrentParamTypes;
115+
SmallVector<BoundsAnnotations, 4> CurrentParamAnnots;
116+
TypeLoc ResultLoc = TL.getReturnLoc();
136117
FunctionProtoType::ExtProtoInfo EPI = T->getExtProtoInfo();
137118
bool EPIChanged = false;
138-
if (getDerived().TransformExtendedParameterInfo(EPI, ParamTypes, ParamAnnots,
139-
ExtParamInfos, TL,
140-
TransformExceptionSpec,
141-
EPIChanged))
142-
return QualType();
143119

144-
// Now rewrite types based on interop type information, and remove
145-
// the interop types.
146-
const BoundsAnnotations Annots = EPI.ReturnAnnots;
147-
if (!Annots.IsEmpty()) {
148-
if (ResultType->isUncheckedPointerType()) {
149-
InteropTypeExpr *IT = Annots.getInteropTypeExpr();
150-
BoundsExpr *Bounds = Annots.getBoundsExpr();
120+
CurrentParamTypes.append(TL.getTypePtr()->param_type_begin(),
121+
TL.getTypePtr()->param_type_end());
122+
123+
// Update return type information.
124+
if (!EPI.ReturnAnnots.IsEmpty()) {
125+
if (ResultLoc.getType()->isUncheckedPointerType()) {
126+
InteropTypeExpr *IT = EPI.ReturnAnnots.getInteropTypeExpr();
127+
BoundsExpr *Bounds = EPI.ReturnAnnots.getBoundsExpr();
151128
assert(Bounds == nullptr || (Bounds != nullptr && IT));
152129
if (IT) {
153-
ResultType = IT->getType();
130+
ResultLoc = IT->getTypeInfoAsWritten()->getTypeLoc();
154131
#if TRACE_INTEROP
155132
llvm::outs() << "return bounds = ";
156133
if (Bounds)
@@ -160,12 +137,8 @@ class TransformFunctionTypeToChecked :
160137
llvm::outs() << "interop type = ";
161138
IT->dump(llvm::outs());
162139
llvm::outs() << "\nresult type = ";
163-
ResultType.dump(llvm::outs());
140+
ResultLoc.getType().dump(llvm::outs());
164141
#endif
165-
// The types are structurally identical except for the checked bit,
166-
// so the type location information can still be used.
167-
TLB.TypeWasModifiedSafely(ResultType);
168-
169142
// Construct new annotations that do not have the bounds-safe interface type.
170143
if (Bounds) {
171144
EPI.ReturnAnnots = BoundsAnnotations(Bounds, nullptr);
@@ -176,21 +149,25 @@ class TransformFunctionTypeToChecked :
176149
}
177150
}
178151

152+
// Update the parameter types and annotations. The EPI parameter array is still used
153+
// by the original type, so a create and update a copy in CurrentParamAnnots.
179154
if (EPI.ParamAnnots) {
180155
// Track whether there are parameter annotations left after removing interop
181156
// annotations.
182157
bool hasParamAnnots = false;
183-
for (unsigned int i = 0; i < ParamTypes.size(); i++) {
184-
BoundsAnnotations IndividualAnnots = ParamAnnots[i];
185-
if (ParamTypes[i]->isUncheckedPointerType() &&
158+
for (unsigned int i = 0; i < CurrentParamTypes.size(); i++) {
159+
BoundsAnnotations IndividualAnnots = EPI.ParamAnnots[i];
160+
CurrentParamAnnots.push_back(IndividualAnnots);
161+
162+
if (CurrentParamTypes[i]->isUncheckedPointerType() &&
186163
IndividualAnnots.getInteropTypeExpr()) {
187164
InteropTypeExpr *IT = IndividualAnnots.getInteropTypeExpr();
188165
QualType ParamType = IT->getType();
189166
if (ParamType.isNull()) {
190167
#if TRACE_INTEROP
191168
llvm::outs() << "encountered null parameter type with bounds";
192169
llvm::outs() << "\noriginal param type = ";
193-
ParamTypes[i]->dump(llvm::outs());
170+
CurrentParamTypes[i]->dump(llvm::outs());
194171
llvm::outs() << "\nparam interop type is:";
195172
IT->dump(llvm::outs());
196173
llvm::outs() << "\n";
@@ -200,29 +177,64 @@ class TransformFunctionTypeToChecked :
200177
}
201178
if (ParamType->isArrayType())
202179
ParamType = SemaRef.Context.getDecayedType(ParamType);
203-
ParamTypes[i] = ParamType;
180+
CurrentParamTypes[i] = ParamType;
204181
// Remove the interop type annotation.
205182
BoundsExpr *Bounds = IndividualAnnots.getBoundsExpr();
206183
if (IT->getType()->isCheckedArrayType() && !Bounds)
207184
Bounds = SemaRef.CreateCountForArrayType(IT->getType());
208185
if (Bounds) {
209186
hasParamAnnots = true;
210-
ParamAnnots[i] = BoundsAnnotations(Bounds, nullptr);
187+
CurrentParamAnnots[i] = BoundsAnnotations(Bounds, nullptr);
211188
} else
212-
ParamAnnots[i] = BoundsAnnotations();
189+
CurrentParamAnnots[i] = BoundsAnnotations();
213190
EPIChanged = true;
214191
} else {
215192
if (!IndividualAnnots.IsEmpty())
216193
hasParamAnnots = true;
217194
}
218195
}
219196

220-
// If there are no parameter bounds left, null out the pointer to the
221-
// param annotations array.
222-
if (!hasParamAnnots)
197+
if (hasParamAnnots)
198+
EPI.ParamAnnots = CurrentParamAnnots.data();
199+
else
200+
// If there are no parameter annotations left, null out the pointer to
201+
// the param annotations array.
223202
EPI.ParamAnnots = nullptr;
224203
}
225204

205+
// Now transform the parameter/return types, parameter declarations, and
206+
// the EPI.
207+
208+
// For the type location information, we must transform the return type
209+
// before pushing the function prototype type location record.
210+
211+
// These hold the transformed results.
212+
SmallVector<QualType, 4> ParamTypes;
213+
SmallVector<ParmVarDecl*, 4> ParamDecls;
214+
Sema::ExtParameterInfoBuilder ExtParamInfos;
215+
// ParamAnnotsStorage is pre-allocated storage that is used when updating EPI
216+
// in TransformExtendedParameterInfo. Its lifetime must last until the end of
217+
// the lifetimie of EPI.
218+
SmallVector<BoundsAnnotations, 4> ParamAnnotsStorage;
219+
220+
QualType ResultType = getDerived().TransformType(TLB, ResultLoc);
221+
if (ResultType.isNull())
222+
return QualType();
223+
224+
// Places transformed data ParamTypes, ParamDecls, and ExtParamInfos.
225+
if (getDerived().TransformFunctionTypeParams(
226+
TL.getBeginLoc(), TL.getParams(),
227+
CurrentParamTypes.begin(),
228+
T->getExtParameterInfosOrNull(),
229+
ParamTypes, &ParamDecls, ExtParamInfos))
230+
return QualType();
231+
232+
if (getDerived().TransformExtendedParameterInfo(EPI, ParamTypes, ParamAnnotsStorage,
233+
ExtParamInfos, TL,
234+
TransformExceptionSpec,
235+
EPIChanged))
236+
return QualType();
237+
226238
// Rebuild the type if something changed.
227239
QualType Result = TL.getType();
228240
if (getDerived().AlwaysRebuild() || ResultType != T->getReturnType() ||
@@ -235,7 +247,6 @@ class TransformFunctionTypeToChecked :
235247
return QualType();
236248
}
237249
}
238-
239250
FunctionProtoTypeLoc NewTL = TLB.push<FunctionProtoTypeLoc>(Result);
240251
NewTL.setLocalRangeBegin(TL.getLocalRangeBegin());
241252
NewTL.setLParenLoc(TL.getLParenLoc());

‎lib/Sema/TreeTransform.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -628,6 +628,16 @@ class TreeTransform {
628628
/// The result vectors should be kept in sync; null entries in the
629629
/// variables vector are acceptable.
630630
///
631+
/// Inputs: Params, ParamTypes, and ParamInfos are the inputs to the
632+
/// method.
633+
///
634+
/// Output: PTypes, PVars, and PInfos are the outputs of the method.
635+
/// The updated parameter types, param var declarations, and PInfo
636+
/// are stored in the method.
637+
///
638+
/// For correctness, the inputs and outputs shoudl be disjoint data
639+
/// structures.
640+
///
631641
/// Return true on error.
632642
bool TransformFunctionTypeParams(
633643
SourceLocation Loc, ArrayRef<ParmVarDecl *> Params,

0 commit comments

Comments
 (0)