From 408d6357243fc8ad1939fc96daa631cb82b38601 Mon Sep 17 00:00:00 2001 From: David Tarditi Date: Fri, 19 Oct 2018 15:12:35 -0700 Subject: [PATCH 1/4] Improve transformation of positional parameters. Change the transformation of positional parameters in TreeTransform.h to use the transformed types for parameters. Do this by using the types passed into TransformExtendedParameterInfo. Before this, the code was just transforming the type for the positional parameter based on however types were being transformed., This worked fine for a type-only transform, but didn't work properly when the parameter type was transformed based on some other informaton in the program (such as a bounds-safe interface). This fixes issue #484. Testing: - Add a regression test based on the case described in issue #484. --- lib/Sema/CheckedCInterop.cpp | 10 +- lib/Sema/SemaExpr.cpp | 2 +- lib/Sema/TreeTransform.h | 107 ++++++++++++++----- test/CheckedC/static-checking/typechecking.c | 6 +- 4 files changed, 91 insertions(+), 34 deletions(-) diff --git a/lib/Sema/CheckedCInterop.cpp b/lib/Sema/CheckedCInterop.cpp index 2453e8ef4a7f..65716ff10a2a 100644 --- a/lib/Sema/CheckedCInterop.cpp +++ b/lib/Sema/CheckedCInterop.cpp @@ -82,6 +82,7 @@ class TransformFunctionTypeToChecked : public: TransformFunctionTypeToChecked(Sema &SemaRef) : BaseTransform(SemaRef) {} + bool IsInstantiation() { return false; } QualType TransformFunctionProtoType(TypeLocBuilder &TLB, FunctionProtoTypeLoc TL) { @@ -212,6 +213,7 @@ class TransformFunctionTypeToChecked : 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. @@ -221,7 +223,7 @@ class TransformFunctionTypeToChecked : if (ResultType.isNull()) return QualType(); - // Places transformed data ParamTypes, ParamDecls, and ExtParamInfos. + // Places transformed data in ParamTypes, ParamDecls, and ExtParamInfos. if (getDerived().TransformFunctionTypeParams( TL.getBeginLoc(), TL.getParams(), CurrentParamTypes.begin(), @@ -229,10 +231,10 @@ class TransformFunctionTypeToChecked : ParamTypes, &ParamDecls, ExtParamInfos)) return QualType(); - if (getDerived().TransformExtendedParameterInfo(EPI, ParamTypes, ParamAnnotsStorage, + if (getDerived().TransformExtendedParameterInfo(EPI, EPIChanged, ParamTypes, + ParamAnnotsStorage, ExtParamInfos, TL, - TransformExceptionSpec, - EPIChanged)) + TransformExceptionSpec)) return QualType(); // Rebuild the type if something changed. diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 735d29545ea4..daee4ae12559 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -1715,7 +1715,7 @@ Sema::BuildDeclRefExpr(ValueDecl *D, QualType Ty, ExprValueKind VK, return BuildDeclRefExpr(D, Ty, VK, NameInfo, SS); } -/// BuildDeclRefExpr - Build an expression that references aB +/// BuildDeclRefExpr - Build an expression that references a /// declaration that does not require a closure capture. ExprResult Sema::BuildDeclRefExpr(ValueDecl *D, QualType Ty, ExprValueKind VK, diff --git a/lib/Sema/TreeTransform.h b/lib/Sema/TreeTransform.h index 1739f4e544b8..c465808e75c3 100644 --- a/lib/Sema/TreeTransform.h +++ b/lib/Sema/TreeTransform.h @@ -119,11 +119,15 @@ class TreeTransform { /// rather than in the subclass (e.g., lambda closure types). llvm::DenseMap TransformedLocalDecls; + /// \brief The set of bound temporaries that have been transformed. This /// is needed so that we can keep uses in sync. llvm::DenseMap TransformedTemporaries; + /// \brief The set of positional parameters that have been transformed. + llvm::SmallVector TransformedPositionalParameters; + public: /// \brief Initializes a new tree transformer. TreeTransform(Sema &SemaRef) : SemaRef(SemaRef) { } @@ -154,6 +158,12 @@ class TreeTransform { /// statement node appears at most once in its containing declaration. bool AlwaysRebuild() { return SemaRef.ArgumentPackSubstitutionIndex != -1; } + /// \brief Whether this use of TreeTransform is being used for template + /// instantiation. + /// + /// Subclasses may override this function when it isn't. + bool IsInstantiation() { return true; } + /// \brief Returns the location of the entity being transformed, if that /// information was not available elsewhere in the AST. /// @@ -657,9 +667,11 @@ class TreeTransform { /// 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. + /// Output: PTypes, PVars, and PInfos are the outputs + /// of the method. + /// - The updated parameter types are stored in PTypes. + /// - The updated parameter variable declarations are stored in PVars. + /// - The updated extend parameter info is stored in PInfos. /// /// For correctness, the inputs and outputs shoudl be disjoint data /// structures. @@ -672,6 +684,10 @@ class TreeTransform { SmallVectorImpl &PTypes, SmallVectorImpl *PVars, Sema::ExtParameterInfoBuilder &PInfos); + /// Transform the bounds annotation Annot, updating Annot. Set changed + /// to true or false depending on whether Annot changed. + /// + /// Sets Changed to true if Annot changed. bool TransformBoundsAnnotations(BoundsAnnotations &Annot, bool &Changed); /// \brief Transform return bounds annotations. We provide a separate method for @@ -681,19 +697,36 @@ class TreeTransform { /// \brief Transform the extended parameter information for /// a function prototype. /// - /// Updates EPI with the transformed information. Sets - /// EPIChanged to true if something changed, false otherwise. + /// \param EPI: The current extended parameter information. Updated + /// with transformed information. /// - /// Return true on error. + /// \param EPIChanged: set to true if something changed, left + /// unchanged otherwise. + /// + /// \param ParamTypes: the new parameter types. Not changed. + /// + + /// \param ParamListAnnots: pre-allocated for holding parameter list + /// annotations. Modified by method if there are parameter bounds + /// annotations. + /// + /// \param ExtParamInfo: external parameter info builder. May be modified + /// if the number of parameters has changed. + /// + /// \param TL: the existing type location information (before transformation) + /// + /// \param TransformExceptionSpec: transforms exception specification. + /// + /// Return true on error, false on success. template bool TransformExtendedParameterInfo( FunctionProtoType::ExtProtoInfo &EPI, - SmallVector &ParamTypes, + bool &EPIChanged, + const SmallVector &ParamTypes, SmallVector &ParamListAnnots, Sema::ExtParameterInfoBuilder &ExtParamInfos, - FunctionProtoTypeLoc &TL, - Fn TransformExceptionSpec, - bool &EPIChanged); + const FunctionProtoTypeLoc &TL, + Fn TransformExceptionSpec); /// \brief Transforms a single function-type parameter. Return null /// on error. @@ -5115,6 +5148,7 @@ bool TreeTransform::TransformFunctionTypeParams( // Expand the function parameter pack into multiple, separate // parameters. getDerived().ExpandingFunctionParameterPack(OldParm); + for (unsigned I = 0; I != *NumExpansions; ++I) { Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(getSema(), I); ParmVarDecl *NewParm @@ -5331,12 +5365,12 @@ bool TreeTransform::TransformReturnBoundsAnnotations( template template bool TreeTransform::TransformExtendedParameterInfo( FunctionProtoType::ExtProtoInfo &EPI, - SmallVector &ParamTypes, + bool &EPIChanged, + const SmallVector &ParamTypes, SmallVector &ParamListAnnots, Sema::ExtParameterInfoBuilder &ExtParamInfos, - FunctionProtoTypeLoc &TL, - Fn TransformExceptionSpec, - bool &EPIChanged) { + const FunctionProtoTypeLoc &TL, + Fn TransformExceptionSpec) { EPIChanged = false; if (TransformExceptionSpec(EPI.ExceptionSpec, EPIChanged)) return false; @@ -5356,6 +5390,16 @@ bool TreeTransform::TransformExtendedParameterInfo( EPI.ExtParameterInfos = nullptr; } + // Return now if there are no bounds annotations to process. + if (EPI.ReturnAnnots.IsEmpty() && !EPI.ParamAnnots) + return false; + + // Set up transformed type information, for positional parameters (if there + // are any). + llvm::SmallVector ExistingInfo(TransformedPositionalParameters); + TransformedPositionalParameters.assign(ParamTypes.begin(), + ParamTypes.end()); + // Handle bounds annotations for return if (getDerived().TransformReturnBoundsAnnotations(EPI.ReturnAnnots, EPIChanged)) return true; @@ -5377,12 +5421,16 @@ bool TreeTransform::TransformExtendedParameterInfo( } ParamListAnnots.push_back(ParamAnnotations); } - if (ParamListAnnotsChanged) { EPIChanged = true; EPI.ParamAnnots = ParamListAnnots.data(); } } + + // Restore the transformed parameter information. + TransformedPositionalParameters.assign(ExistingInfo.begin(), + ExistingInfo.end()); + return false; } @@ -5412,9 +5460,9 @@ QualType TreeTransform::TransformFunctionProtoType( // parameters before the return type, since the return type can then refer // to the parameters themselves (via decltype, sizeof, etc.). // - SmallVector ParamTypes; - SmallVector ParamDecls; - SmallVector ParamAnnots; + SmallVector ParamTypes; // New parameter types. + SmallVector ParamDecls; // New parameter declarations. + SmallVector ParamAnnots; // New parameter annotations. Sema::ExtParameterInfoBuilder ExtParamInfos; const FunctionProtoType *T = TL.getTypePtr(); @@ -5457,10 +5505,9 @@ QualType TreeTransform::TransformFunctionProtoType( FunctionProtoType::ExtProtoInfo EPI = T->getExtProtoInfo(); bool EPIChanged = false; - if (getDerived().TransformExtendedParameterInfo(EPI, ParamTypes, ParamAnnots, + if (getDerived().TransformExtendedParameterInfo(EPI, EPIChanged, ParamTypes, ParamAnnots, ExtParamInfos, TL, - TransformExceptionSpec, - EPIChanged)) + TransformExceptionSpec)) return QualType(); QualType Result = TL.getType(); @@ -9101,8 +9148,8 @@ TreeTransform::TransformDeclRefExpr(DeclRefExpr *E) { !E->hasExplicitTemplateArgs()) { // Mark it referenced in the new context regardless. - // FIXME: this is a bit instantiation-specific. - SemaRef.MarkDeclRefReferenced(E); + if (getDerived().IsInstantiation()) + SemaRef.MarkDeclRefReferenced(E); return E; } @@ -11006,7 +11053,8 @@ TreeTransform::TransformCXXConstructExpr(CXXConstructExpr *E) { !ArgumentChanged) { // Mark the constructor as referenced. // FIXME: Instantiation-specific - SemaRef.MarkFunctionReferenced(E->getLocStart(), Constructor); + if (getDerived().IsInstantiation()) + SemaRef.MarkFunctionReferenced(E->getLocStart(), Constructor); return E; } @@ -11038,7 +11086,8 @@ ExprResult TreeTransform::TransformCXXInheritedCtorInitExpr( Constructor == E->getConstructor()) { // Mark the constructor as referenced. // FIXME: Instantiation-specific - SemaRef.MarkFunctionReferenced(E->getLocStart(), Constructor); + if (getDerived().IsInstantiation()) + SemaRef.MarkFunctionReferenced(E->getLocStart(), Constructor); return E; } @@ -11116,7 +11165,8 @@ TreeTransform::TransformCXXTemporaryObjectExpr( Constructor == E->getConstructor() && !ArgumentChanged) { // FIXME: Instantiation-specific - SemaRef.MarkFunctionReferenced(E->getLocStart(), Constructor); + if (getDerived().IsInstantiation()) + SemaRef.MarkFunctionReferenced(E->getLocStart(), Constructor); return SemaRef.MaybeBindToTemporary(E); } @@ -12395,6 +12445,7 @@ TreeTransform::TransformBlockExpr(BlockExpr *E) { QualType exprResultType = getDerived().TransformType(exprFunctionType->getReturnType()); + auto epi = exprFunctionType->getExtProtoInfo(); epi.ExtParameterInfos = extParamInfos.getPointerOrNull(paramTypes.size()); @@ -12565,7 +12616,9 @@ ExprResult TreeTransform::TransformPositionalParameterExpr( PositionalParameterExpr *E) { unsigned Index = E->getIndex(); - QualType QT = getDerived().TransformType(E->getType()); + assert(Index < TransformedPositionalParameters.size()); + QualType QT = TransformedPositionalParameters[Index]; + if (!getDerived().AlwaysRebuild() && QT == E->getType()) return E; diff --git a/test/CheckedC/static-checking/typechecking.c b/test/CheckedC/static-checking/typechecking.c index 30a59f330450..81a9acb1d402 100644 --- a/test/CheckedC/static-checking/typechecking.c +++ b/test/CheckedC/static-checking/typechecking.c @@ -69,8 +69,10 @@ f304(int i, _Ptr : byte_count(i), _Array_ptr : byte_c // Bounds in a function type reference parameters or locals. void f305(int i) { int j = i; - _Ptr<_Array_ptr(void) : count(i)> p = 0; // expected-error {{out-of-scope variable for bounds}} - _Ptr<_Array_ptr(void) : count(j)> q = 0; // expected-error {{out-of-scope variable for bounds}} + _Ptr<_Array_ptr(void) : count(i)> p = 0; // expected-error {{out-of-scope variable for bounds}} + _Ptr<_Array_ptr(int k) : count(i)> q = 0; ; // expected-error {{out-of-scope variable for bounds}} + _Ptr<_Array_ptr(void) : count(j)> r = 0; // expected-error {{out-of-scope variable for bounds}} + } // Global variable bounds are OK. From 9f4449ca20894f60395b68f1795c16fae1c0e3a4 Mon Sep 17 00:00:00 2001 From: David Tarditi Date: Sat, 20 Oct 2018 09:46:38 -0700 Subject: [PATCH 2/4] Improve comments and remove white space changes. --- lib/Sema/CheckedCInterop.cpp | 3 +- lib/Sema/TreeTransform.h | 18 ++++------ .../bug_484_rewrite_parameter_uses.c | 35 +++++++++++++++++++ 3 files changed, 42 insertions(+), 14 deletions(-) create mode 100644 test/CheckedC/regression-cases/bug_484_rewrite_parameter_uses.c diff --git a/lib/Sema/CheckedCInterop.cpp b/lib/Sema/CheckedCInterop.cpp index 65716ff10a2a..9242af6b21dc 100644 --- a/lib/Sema/CheckedCInterop.cpp +++ b/lib/Sema/CheckedCInterop.cpp @@ -213,10 +213,9 @@ class TransformFunctionTypeToChecked : 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. + // the lifetime of EPI. SmallVector ParamAnnotsStorage; QualType ResultType = getDerived().TransformType(TLB, ResultLoc); diff --git a/lib/Sema/TreeTransform.h b/lib/Sema/TreeTransform.h index c465808e75c3..cae851d6754f 100644 --- a/lib/Sema/TreeTransform.h +++ b/lib/Sema/TreeTransform.h @@ -119,13 +119,13 @@ class TreeTransform { /// rather than in the subclass (e.g., lambda closure types). llvm::DenseMap TransformedLocalDecls; - /// \brief The set of bound temporaries that have been transformed. This /// is needed so that we can keep uses in sync. llvm::DenseMap TransformedTemporaries; - /// \brief The set of positional parameters that have been transformed. + /// \brief The set of parameters that have been transformed. Used + /// to update positional parameter expression type information. llvm::SmallVector TransformedPositionalParameters; public: @@ -664,11 +664,9 @@ 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. + /// Inputs: Params, ParamTypes, and ParamInfos. /// - /// Output: PTypes, PVars, and PInfos are the outputs - /// of the method. + /// Outputs: PTypes, PVars, and PInfos: /// - The updated parameter types are stored in PTypes. /// - The updated parameter variable declarations are stored in PVars. /// - The updated extend parameter info is stored in PInfos. @@ -684,10 +682,8 @@ class TreeTransform { SmallVectorImpl &PTypes, SmallVectorImpl *PVars, Sema::ExtParameterInfoBuilder &PInfos); - /// Transform the bounds annotation Annot, updating Annot. Set changed - /// to true or false depending on whether Annot changed. - /// - /// Sets Changed to true if Annot changed. + /// Transform the bounds annotation Annot, updating Annot. Set Changed to + /// true if Annot changed. bool TransformBoundsAnnotations(BoundsAnnotations &Annot, bool &Changed); /// \brief Transform return bounds annotations. We provide a separate method for @@ -5148,7 +5144,6 @@ bool TreeTransform::TransformFunctionTypeParams( // Expand the function parameter pack into multiple, separate // parameters. getDerived().ExpandingFunctionParameterPack(OldParm); - for (unsigned I = 0; I != *NumExpansions; ++I) { Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(getSema(), I); ParmVarDecl *NewParm @@ -12445,7 +12440,6 @@ TreeTransform::TransformBlockExpr(BlockExpr *E) { QualType exprResultType = getDerived().TransformType(exprFunctionType->getReturnType()); - auto epi = exprFunctionType->getExtProtoInfo(); epi.ExtParameterInfos = extParamInfos.getPointerOrNull(paramTypes.size()); diff --git a/test/CheckedC/regression-cases/bug_484_rewrite_parameter_uses.c b/test/CheckedC/regression-cases/bug_484_rewrite_parameter_uses.c new file mode 100644 index 000000000000..6e58fe75cb2a --- /dev/null +++ b/test/CheckedC/regression-cases/bug_484_rewrite_parameter_uses.c @@ -0,0 +1,35 @@ +// +// These is a regression test case for +// https://github.com/Microsoft/checkedc-clang/issues/484 +// +// In checked scopes, we rewrite function types with bounds-safe interfaces to +// be fully checked. We need to rewrite declarations of parameters and uses of +// parameters that have bounds-safe interfaces. +// +// The following example illustrates the problem, if wed don't do the rewrite. +// We define 3 C typedefs: +// 1. The first one defines an unchecked function pointer with a bounds-safe +// interface on a parameter. +// 2. The second one defines the bounds-safe interface type the first one. +// 3. The third one defines a full checked version. +// +// If we don't rewrite the uses of parameters, the function types +// don't match and we get a type checking error. +// RUN: %clang -Wno-check-bounds-decls-checked-scope -c -o %t %s + +#pragma CHECKED_SCOPE ON + +typedef int (*callback_fn3)(int *a : count(n), int n); +typedef _Ptr bsi_callback_fn3; +typedef _Ptr a : bounds(a, a + n), int n)> checked_callback_fn3; + +_Checked callback_fn3 return_function_pointer(void) : itype(bsi_callback_fn3); + +// _Checked callback_fn3 return_function_pointer(void) : itype(_Ptr); + +_Checked void test_function_pointer_return(void) { + checked_callback_fn3 fn = return_function_pointer(); +} + + + From 6848929e4ddb181e855ab8fb3a8696e955f31bf4 Mon Sep 17 00:00:00 2001 From: David Tarditi Date: Sat, 20 Oct 2018 09:54:22 -0700 Subject: [PATCH 3/4] Clean up regression test case. --- .../regression-cases/bug_484_rewrite_parameter_uses.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/CheckedC/regression-cases/bug_484_rewrite_parameter_uses.c b/test/CheckedC/regression-cases/bug_484_rewrite_parameter_uses.c index 6e58fe75cb2a..092570abe1d2 100644 --- a/test/CheckedC/regression-cases/bug_484_rewrite_parameter_uses.c +++ b/test/CheckedC/regression-cases/bug_484_rewrite_parameter_uses.c @@ -6,12 +6,13 @@ // be fully checked. We need to rewrite declarations of parameters and uses of // parameters that have bounds-safe interfaces. // -// The following example illustrates the problem, if wed don't do the rewrite. +// The following example illustrates the problem, if we don't do the rewrite. // We define 3 C typedefs: // 1. The first one defines an unchecked function pointer with a bounds-safe // interface on a parameter. -// 2. The second one defines the bounds-safe interface type the first one. -// 3. The third one defines a full checked version. +// 2. The second one defines the bounds-safe interface type for the first +// typedef's function pointer type. +// 3. The third one defines a fully checked version. // // If we don't rewrite the uses of parameters, the function types // don't match and we get a type checking error. @@ -25,8 +26,6 @@ typedef _Ptr a : bounds(a, a + n), int n)> checked_callbac _Checked callback_fn3 return_function_pointer(void) : itype(bsi_callback_fn3); -// _Checked callback_fn3 return_function_pointer(void) : itype(_Ptr); - _Checked void test_function_pointer_return(void) { checked_callback_fn3 fn = return_function_pointer(); } From 97124407ff7216aa249c46e34fb885512223af44 Mon Sep 17 00:00:00 2001 From: David Tarditi Date: Sat, 20 Oct 2018 10:05:20 -0700 Subject: [PATCH 4/4] Fix typo in comment. --- test/CheckedC/regression-cases/bug_484_rewrite_parameter_uses.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/CheckedC/regression-cases/bug_484_rewrite_parameter_uses.c b/test/CheckedC/regression-cases/bug_484_rewrite_parameter_uses.c index 092570abe1d2..434a06dff03d 100644 --- a/test/CheckedC/regression-cases/bug_484_rewrite_parameter_uses.c +++ b/test/CheckedC/regression-cases/bug_484_rewrite_parameter_uses.c @@ -1,5 +1,5 @@ // -// These is a regression test case for +// This is a regression test case for // https://github.com/Microsoft/checkedc-clang/issues/484 // // In checked scopes, we rewrite function types with bounds-safe interfaces to