From c3fa0132ddfd4333673cba4951587ab257ee348c Mon Sep 17 00:00:00 2001 From: David Tarditi Date: Wed, 21 Dec 2016 12:24:52 -0800 Subject: [PATCH 1/4] Checkpoint checking bounds for variable redeclarations. This change integrates checking of bounds for variables into the process for merging declarations. However, bounds don't actually affect how declarations are merged. Also, bounds are attached to variable declarations after the declarator has been processed, so they can't be processed during merging. Checkpoint things before moving the checking later. --- include/clang/Basic/DiagnosticSemaKinds.td | 30 +++---- include/clang/Sema/Sema.h | 9 +++ lib/Sema/SemaDecl.cpp | 94 ++++++++++++---------- 3 files changed, 75 insertions(+), 58 deletions(-) diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index b5d008fd1430..ca113616ee82 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -8843,23 +8843,19 @@ def err_bounds_type_annotation_lost_checking : Error< def note_previous_bounds_decl : Note<"previous bounds declaration is here">; - def err_conflicting_parameter_bounds : Error< - "function redeclaration has conflicting parameter bounds">; - - def err_conflicting_return_bounds : Error< - "function redeclaration has conflicting return bounds">; - - def err_added_bounds_for_return : Error< - "function redeclaration added return bounds">; - - def err_missing_bounds_for_return : Error < - "function redeclaration dropped return bounds">; - - def err_added_bounds_for_parameter : Error< - "function redeclaration added bounds for parameter">; - - def err_missing_bounds_for_parameter : Error< - "function redeclaration dropped bounds for parameter">; + def err_decl_conflicting_bounds : Error< + "%select{function redeclaration has conflicting parameter bounds|function " + "redeclaration has conflicting return bounds|variable redeclaration has " + "conflicting bounds}0">; + + def err_decl_added_bounds : Error< + "%select{function redeclaration added bounds for parameter|function " + "redeclaration added return bounds|variable redeclaration added bounds}0">; + + def err_decl_dropped_bounds : Error< + "%select{function redeclaration dropped bounds for parameter|function " + "redeclaration dropped return bounds|variable declaration dropped " + "bounds}0">; def err_conflicting_bounds : Error<"conflicting bounds for %0">; diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index 282a0d5afaf7..d904a121669a 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -2249,6 +2249,7 @@ class Sema { Scope *S, bool MergeTypeWithOld); void mergeObjCMethodDecls(ObjCMethodDecl *New, ObjCMethodDecl *Old); void MergeVarDecl(VarDecl *New, LookupResult &Previous); + void MergeVarDeclBounds(VarDecl *New, VarDecl *Old); void MergeVarDeclTypes(VarDecl *New, VarDecl *Old, bool MergeTypeWithOld); void MergeVarDeclExceptionSpecs(VarDecl *New, VarDecl *Old); bool MergeCXXFunctionDecl(FunctionDecl *New, FunctionDecl *Old, Scope *S); @@ -2267,6 +2268,14 @@ class Sema { CCT_Union }; + // used for %select in diagnostics for errors involving redeclarations + // with bounds + enum class CheckedCBoundsError { + CCBE_Parameter, + CCBE_Return, + CCBE_Variable + }; + CheckedTypeClassification classifyForCheckedTypeDiagnostic(QualType qt); // AssignmentAction - This is used by all the assignment diagnostic functions diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp index e02cc4e409f4..c3b94bea49e5 100644 --- a/lib/Sema/SemaDecl.cpp +++ b/lib/Sema/SemaDecl.cpp @@ -3331,7 +3331,7 @@ static void emitBoundsErrorDiagnostic(Sema &S, int DiagId, const DeclaratorDecl *Old, const DeclaratorDecl *New, bool IsUncheckedPointerType, - bool IsReturn) { + Sema::CheckedCBoundsError Kind) { // Emit the diagnostic, pointing at the current bounds expression // if possible. Use the new declaration if there is no bounds // expression. @@ -3340,7 +3340,7 @@ static void emitBoundsErrorDiagnostic(Sema &S, int DiagId, Loc = New->getBoundsExpr()->getStartLoc(); else Loc = New->getLocation(); - S.Diag(Loc, DiagId); + S.Diag(Loc, DiagId) << (unsigned) Kind; // Emit a note pointing to the prior declaration. Try to point // at the relevant bounds expression if possible so that the user has the @@ -3354,19 +3354,16 @@ static void emitBoundsErrorDiagnostic(Sema &S, int DiagId, // was compatible with Old. If there's no bounds expression on Old, // search for a possible earlier definition. if (!PrevBoundsExpr && IsUncheckedPointerType) { - if (IsReturn) { - const FunctionDecl *Previous = dyn_cast(Old); - assert(Previous); + if (const FunctionDecl *Previous = dyn_cast(Old)) { do { PrevBoundsExpr = Previous->getBoundsExpr(); Previous = Previous->getPreviousDecl(); } while (!PrevBoundsExpr && Previous); } - else if (isa(Old)) { + else if (const ParmVarDecl *Previous = dyn_cast(Old)) { // For parameters, this is a little more work because // we can't just walk to the "prior" declaration. We // must navigate through function declarations instead. - const ParmVarDecl *Previous = dyn_cast(Old); unsigned paramNumber = Previous->getFunctionScopeIndex(); do { PrevBoundsExpr = Previous->getBoundsExpr(); @@ -3381,40 +3378,49 @@ static void emitBoundsErrorDiagnostic(Sema &S, int DiagId, else Previous = nullptr; } while (!PrevBoundsExpr && Previous); - } + } else if (const VarDecl *Previous = dyn_cast(Old)) { + do { + PrevBoundsExpr = Previous->getBoundsExpr(); + Previous = Previous->getPreviousDecl(); + } while (!PrevBoundsExpr && Previous); + } else + llvm_unreachable("unexpected declaration kind"); } if (PrevBoundsExpr) { int NoteId = diag::note_previous_bounds_decl; S.Diag(PrevBoundsExpr->getStartLoc(), NoteId); - } else if (IsReturn) { - const FunctionDecl *OldDecl = dyn_cast(Old); + } else if (const FunctionDecl *OldDecl = dyn_cast(Old)) { const FunctionDecl *NewDecl = dyn_cast(New); - int PrevDiag; - SourceLocation OldLocation; - std::tie(PrevDiag, OldLocation) - = getNoteDiagForInvalidRedeclaration(OldDecl, NewDecl); - S.Diag(OldLocation, PrevDiag); + if (NewDecl) { + int PrevDiag; + SourceLocation OldLocation; + std::tie(PrevDiag, OldLocation) + = getNoteDiagForInvalidRedeclaration(OldDecl, NewDecl); + S.Diag(OldLocation, PrevDiag); + } else + llvm_unreachable("mismatched declarations"); } else { int NoteId = diag::note_previous_decl; S.Diag(Old->getLocation(), NoteId) << Old; } } -// Shared logic for diagnosing bounds declaration conflicts for parameters and -// returns. The logic is the same, but the error messages are different. +// Shared logic for diagnosing bounds declaration conflicts for parameters, +// returns, and variables // -// * OldBounds and NewBounds are bounds expression from a function type. -// It is important to use these for bounds comparisons because they've been -// canonicalized, while the bounds on the actual declarations have not. +// * OldBounds and NewBounds are canoncialized bounds expressions. For +// parameters and returns, they are bounds expression from a function type. +// It is important to use canonicalized bound expressions these for bounds +// comparisons. // * OldDecl and NewDecl provide the declarations for use in error messages. // Usually these have source-level declarations of bounds with accurate line // number information. They may be synthesized for typedef'ed function // declarations. // * OldType and NewType are the types of the items whoses bounds declarations -// are being checked. We pass them in because it is easier to compute the -// return bounds at the caller than to to compute them here. -// * IsReturn is whether this a return bounds. +// are being checked. We pass them in because it is easier to compute them at +// the caller than than to compute them here. +// * Kind is what is being checked. // // Return true if an error involving bounds has been diagnosed, false if not. static bool diagnoseBoundsError(Sema &S, @@ -3424,7 +3430,7 @@ static bool diagnoseBoundsError(Sema &S, const DeclaratorDecl *NewDecl, QualType OldType, QualType NewType, - bool IsReturn) { + Sema::CheckedCBoundsError Kind) { int DiagId = 0; bool IsUncheckedPointerType = OldType->isUncheckedPointerType() && NewType->isUncheckedPointerType(); @@ -3435,27 +3441,20 @@ static bool diagnoseBoundsError(Sema &S, return true; if (!S.Context.EquivalentBounds(OldBounds, NewBounds)) - // Use the bounds from the declarations for error messages. - DiagId = IsReturn ? diag::err_conflicting_return_bounds : - diag::err_conflicting_parameter_bounds; + DiagId = diag::err_decl_conflicting_bounds; } else if (OldBounds || NewBounds) { - if (!IsUncheckedPointerType) { - if (IsReturn) - DiagId = NewBounds ? diag::err_added_bounds_for_return : - diag::err_missing_bounds_for_return; - else - DiagId = NewBounds ? diag::err_added_bounds_for_parameter : - diag::err_missing_bounds_for_parameter; - - } + if (!IsUncheckedPointerType) + DiagId = NewBounds ? diag::err_decl_added_bounds : + diag::err_decl_dropped_bounds; } if (DiagId) { emitBoundsErrorDiagnostic(S, DiagId, OldDecl, NewDecl, - IsUncheckedPointerType, IsReturn); + IsUncheckedPointerType, Kind); return true; } - // TODO: handle parameter or return types have bounds mismatches embedded - // within them. + // TODO: produce better error messages when types for parameters, returns, + // or variables have bounds mismatches embedded within them. The current + // diagnostic will be "type mismatch" return false; } @@ -3503,14 +3502,14 @@ bool Sema::DiagnoseCheckedCFunctionCompatibility(FunctionDecl *New, OldDecl, NewDecl, OldType->getParamType(i), NewType->getParamType(i), - /*IsReturn=*/false)) + CheckedCBoundsError::CCBE_Parameter)) Err = true; } if (diagnoseBoundsError(*this, OldType->getReturnBounds(), NewType->getReturnBounds(), Old, New, OldType->getReturnType(), NewType->getReturnType(), - /*IsReturn=*/true)) + CheckedCBoundsError::CCBE_Return)) Err = true; // See if the types are compatible if bounds are ignored. @@ -3738,6 +3737,17 @@ static bool mergeTypeWithPrevious(Sema &S, VarDecl *NewVD, VarDecl *OldVD, } } +void Sema::MergeVarDeclBounds(VarDecl *New, VarDecl *Old) { + BoundsExpr *OldBounds = Old->getBoundsExpr(); + BoundsExpr *NewBounds = New->getBoundsExpr(); + if (diagnoseBoundsError(*this, OldBounds, NewBounds, + Old, New, Old->getType(), New->getType(), + CheckedCBoundsError::CCBE_Variable)) + ActOnInvalidBoundsDecl(New); + else if (OldBounds && !NewBounds) + ActOnBoundsDecl(New, OldBounds); +} + /// MergeVarDecl - We just parsed a variable 'New' which has the same name /// and scope as a previous declaration 'Old'. Figure out how to resolve this /// situation, merging decls or emitting diagnostics as appropriate. @@ -3830,11 +3840,13 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) { mergeTypeWithPrevious(*this, New, MostRecent, Previous)); if (New->isInvalidDecl()) return; + MergeVarDeclBounds(New, MostRecent); } MergeVarDeclTypes(New, Old, mergeTypeWithPrevious(*this, New, Old, Previous)); if (New->isInvalidDecl()) return; + MergeVarDeclBounds(New, Old); diag::kind PrevDiag; SourceLocation OldLocation; From 140192bc798d12965ba16cf0e2f012ded117980d Mon Sep 17 00:00:00 2001 From: David Tarditi Date: Thu, 22 Dec 2016 10:26:00 -0800 Subject: [PATCH 2/4] Finish checking bounds for variable declarations. This completes issue #30. - Move checking for conflicting bounds expressions on variable declarations later to ActOnBoundsDecl. A bounds expression for a variable is not constructed/built until after the declarator has been processed, so we cannot check it as part of checking the declarator. - Generalize the existing code for checking for conflict bounds expression on declaration to handle variables. - I discovered an error in the parsing of bounds expressions for function declarators. I believe it was possible to write a bounds expression after a function declarator, which would have triggered an internal compiler assert: array_ptr f(int len) : count(len) : count(5), for example. The fix is to not try to parse a bounds expression after a function declarator, since the parsing of the function declarator already handled it. --- include/clang/Sema/Sema.h | 1 - lib/Parse/ParseDecl.cpp | 31 ++++---- lib/Sema/SemaDecl.cpp | 154 +++++++++++++++++++++++++------------- 3 files changed, 118 insertions(+), 68 deletions(-) diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index d904a121669a..83986b8a9e1d 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -2249,7 +2249,6 @@ class Sema { Scope *S, bool MergeTypeWithOld); void mergeObjCMethodDecls(ObjCMethodDecl *New, ObjCMethodDecl *Old); void MergeVarDecl(VarDecl *New, LookupResult &Previous); - void MergeVarDeclBounds(VarDecl *New, VarDecl *Old); void MergeVarDeclTypes(VarDecl *New, VarDecl *Old, bool MergeTypeWithOld); void MergeVarDeclExceptionSpecs(VarDecl *New, VarDecl *Old); bool MergeCXXFunctionDecl(FunctionDecl *New, FunctionDecl *Old, Scope *S); diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp index a446e1d09966..511aaf0ec61f 100644 --- a/lib/Parse/ParseDecl.cpp +++ b/lib/Parse/ParseDecl.cpp @@ -2086,22 +2086,25 @@ Decl *Parser::ParseDeclarationAfterDeclaratorAndAttributes( bool TypeContainsAuto = D.getDeclSpec().containsPlaceholderType(); - // Parse the optional Checked C bounds expression or interop type - // annotation. - if (getLangOpts().CheckedC && Tok.is(tok::colon)) { - ConsumeToken(); - ExprResult Bounds = ParseBoundsExpressionOrInteropType(D); - if (Bounds.isInvalid()) - SkipUntil(tok::comma, tok::equal, StopAtSemi | StopBeforeMatch); + // If this is a variable declarator in Checked C, parse the bounds expression + // (if any) and set the bounds expression. Function declarators are ignored + // here because return bounds expressions are/ parsed as part of function + // declarators already. + if (getLangOpts().CheckedC && isa(ThisDecl)) { VarDecl *ThisVarDecl = dyn_cast(ThisDecl); - if (ThisVarDecl) { - if (Bounds.isInvalid()) - Actions.ActOnInvalidBoundsDecl(ThisVarDecl); - else - Actions.ActOnBoundsDecl(ThisVarDecl, cast(Bounds.get())); - } else - llvm_unreachable("Unexpected decl type"); + BoundsExpr *Bounds = nullptr; + // The optional Checked C bounds expression or interop type annotation. + if (Tok.is(tok::colon)) { + ConsumeToken(); + ExprResult ParsedBounds = ParseBoundsExpressionOrInteropType(D); + if (ParsedBounds.isInvalid()) { + SkipUntil(tok::comma, tok::equal, StopAtSemi | StopBeforeMatch); + Bounds = Actions.CreateInvalidBoundsExpr(); + } else + Bounds = cast(ParsedBounds.get()); + } + Actions.ActOnBoundsDecl(ThisVarDecl, Bounds); } // Parse declarator '=' initializer. diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp index c3b94bea49e5..91b368ad0f46 100644 --- a/lib/Sema/SemaDecl.cpp +++ b/lib/Sema/SemaDecl.cpp @@ -3327,7 +3327,11 @@ void Sema::mergeObjCMethodDecls(ObjCMethodDecl *newMethod, CheckObjCMethodOverride(newMethod, oldMethod); } +// Print an error message for a declaration with a bounds error. +// Find the relevant prior bounds and emit a note pointing to it. +// If there is no prior bounds, point to the prior declaration. static void emitBoundsErrorDiagnostic(Sema &S, int DiagId, + const SourceLocation BoundsLoc, const DeclaratorDecl *Old, const DeclaratorDecl *New, bool IsUncheckedPointerType, @@ -3336,13 +3340,13 @@ static void emitBoundsErrorDiagnostic(Sema &S, int DiagId, // if possible. Use the new declaration if there is no bounds // expression. SourceLocation Loc; - if (New->hasBoundsExpr()) - Loc = New->getBoundsExpr()->getStartLoc(); + if (BoundsLoc.isValid()) + Loc = BoundsLoc; else Loc = New->getLocation(); S.Diag(Loc, DiagId) << (unsigned) Kind; - // Emit a note pointing to the prior declaration. Try to point + // Emit a note pointing to the prior bounds or declaration. Try to point // at the relevant bounds expression if possible so that the user has the // right context for understanding the error. If there isn't one, fall // back to the declaration. @@ -3409,14 +3413,17 @@ static void emitBoundsErrorDiagnostic(Sema &S, int DiagId, // Shared logic for diagnosing bounds declaration conflicts for parameters, // returns, and variables // +// * BoundsLoc is the source location of the bounds expression at which to +// report the error. If there is no bounds expression, this will be an invalid +// source location. (We don't use the location from the new bounds expression +// beacuse it is canonicalized and may not have valid line information.) // * OldBounds and NewBounds are canoncialized bounds expressions. For // parameters and returns, they are bounds expression from a function type. // It is important to use canonicalized bound expressions these for bounds // comparisons. // * OldDecl and NewDecl provide the declarations for use in error messages. -// Usually these have source-level declarations of bounds with accurate line -// number information. They may be synthesized for typedef'ed function -// declarations. +// OldDecl may have a bounds expression attached to it. A bounds expression +// may not be attached to NewDecl yet. // * OldType and NewType are the types of the items whoses bounds declarations // are being checked. We pass them in because it is easier to compute them at // the caller than than to compute them here. @@ -3424,6 +3431,7 @@ static void emitBoundsErrorDiagnostic(Sema &S, int DiagId, // // Return true if an error involving bounds has been diagnosed, false if not. static bool diagnoseBoundsError(Sema &S, + SourceLocation BoundsLoc, const BoundsExpr *OldBounds, const BoundsExpr *NewBounds, const DeclaratorDecl *OldDecl, @@ -3448,7 +3456,7 @@ static bool diagnoseBoundsError(Sema &S, diag::err_decl_dropped_bounds; } if (DiagId) { - emitBoundsErrorDiagnostic(S, DiagId, OldDecl, NewDecl, + emitBoundsErrorDiagnostic(S, DiagId, BoundsLoc, OldDecl, NewDecl, IsUncheckedPointerType, Kind); return true; } @@ -3458,6 +3466,14 @@ static bool diagnoseBoundsError(Sema &S, return false; } +static SourceLocation getBoundsLocation(const DeclaratorDecl *D) { + const BoundsExpr *Bounds = D->getBoundsExpr(); + if (Bounds) + return Bounds->getLocStart(); + else + return SourceLocation(); +} + /// \brief Diagnose Checked C-specific compatibility issues for function decls. /// Handle cases where (1) there are mismatched bounds declarations for /// parameters or return types or (2) cases where one declaration has no @@ -3498,14 +3514,16 @@ bool Sema::DiagnoseCheckedCFunctionCompatibility(FunctionDecl *New, const DeclaratorDecl *NewDecl = New->getParamDecl(i); // Use the bounds from the types; they've been canonicalized, // while the bounds in the declarations have not been. - if (diagnoseBoundsError(*this, OldTypeBounds, NewTypeBounds, + if (diagnoseBoundsError(*this, getBoundsLocation(NewDecl), + OldTypeBounds, NewTypeBounds, OldDecl, NewDecl, OldType->getParamType(i), NewType->getParamType(i), CheckedCBoundsError::CCBE_Parameter)) Err = true; } - if (diagnoseBoundsError(*this, OldType->getReturnBounds(), + if (diagnoseBoundsError(*this, getBoundsLocation(New), + OldType->getReturnBounds(), NewType->getReturnBounds(), Old, New, OldType->getReturnType(), NewType->getReturnType(), @@ -3737,17 +3755,6 @@ static bool mergeTypeWithPrevious(Sema &S, VarDecl *NewVD, VarDecl *OldVD, } } -void Sema::MergeVarDeclBounds(VarDecl *New, VarDecl *Old) { - BoundsExpr *OldBounds = Old->getBoundsExpr(); - BoundsExpr *NewBounds = New->getBoundsExpr(); - if (diagnoseBoundsError(*this, OldBounds, NewBounds, - Old, New, Old->getType(), New->getType(), - CheckedCBoundsError::CCBE_Variable)) - ActOnInvalidBoundsDecl(New); - else if (OldBounds && !NewBounds) - ActOnBoundsDecl(New, OldBounds); -} - /// MergeVarDecl - We just parsed a variable 'New' which has the same name /// and scope as a previous declaration 'Old'. Figure out how to resolve this /// situation, merging decls or emitting diagnostics as appropriate. @@ -3840,13 +3847,11 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) { mergeTypeWithPrevious(*this, New, MostRecent, Previous)); if (New->isInvalidDecl()) return; - MergeVarDeclBounds(New, MostRecent); } MergeVarDeclTypes(New, Old, mergeTypeWithPrevious(*this, New, Old, Previous)); if (New->isInvalidDecl()) return; - MergeVarDeclBounds(New, Old); diag::kind PrevDiag; SourceLocation OldLocation; @@ -11772,49 +11777,92 @@ bool Sema::DiagnoseBoundsDeclType(QualType Ty, DeclaratorDecl *D, return result; } -// ActOnBoundsDecl: handle a Checked C bounds declaration for a declarator. -// Determine whether the bounds declaration involves a bounds expression -// or a type annotation and call the appropriate method to handle it. +// Attach a bounds expression to a variable declaration. First check that the +// bounds expression does not conflict with the bounds expression for the prior +// declaration, if there is a prior declaration. +static void HandleVarDeclBounds(Sema &S, VarDecl *D, BoundsExpr *Expr) { + VarDecl *Old = D->getPreviousDecl(); + if (!Old) { + // There is no prior declaration, so there is nothing more to do. + D->setBoundsExpr(Expr); + return; + } + SourceLocation BoundsLoc = Expr ? Expr->getStartLoc() : SourceLocation(); + BoundsExpr *OldBounds = Old->getBoundsExpr(); + if (diagnoseBoundsError(S, BoundsLoc, OldBounds, Expr, Old, D, + Old->getType(), D->getType(), + Sema::CheckedCBoundsError::CCBE_Variable)) { + S.ActOnInvalidBoundsDecl(D); + return; + } + + if (OldBounds && !Expr) + Expr = OldBounds; + + D->setBoundsExpr(Expr); +} + +// Attach a Checked C bounds expression to a declaration. If there is no +// bounds expression, Expr is null. (This method needs to be called even when +// a bounds expression is omitted for a variable because dropping a bounds +// expression may be an error for a variable redeclaration). +// - Check that the bounds expression is valid for the declarator. +// It must meet typing requirements and be valid for the declaration. +// - For VarDecls, make sure that a bounds expression on a redeclaration +// is valid. void Sema::ActOnBoundsDecl(DeclaratorDecl *D, BoundsExpr *Expr) { - if (!D || !Expr) + if (!D) return; + assert(!isa(D) && "unexpected function decl"); QualType Ty = D->getType(); if (Ty.isNull()) return; - if (VarDecl *Var = dyn_cast(D)) { - if (Var->isLocalVarDecl()) { - // - Local variables cannot have bounds-safe interface type annotations. - // - Local variables with unchecked pointer or array types cannot have - // bounds declarations. For bounds declarations, this reduces the chance - // that programmers accidentally declare variables with unchecked types - // to have bounds declarations and think uses of the variables will be - // bounds checked. - // - // Other declarations (parameters, globally-scoped variables, and members) - // can have unchecked pointer or array types with bounds declarations - // because that is a way bounds-safe interfaces are declared. - int DiagId = 0; - if (Expr->isInteropTypeAnnotation()) - DiagId = diag::err_bounds_safe_interface_type_annotation_local_variable; - else { - if (Ty->isPointerType() && !Ty->isCheckedPointerType()) - DiagId = diag::err_bounds_declaration_unchecked_local_pointer; - else if (Ty->isArrayType() && !Ty->isCheckedArrayType()) - DiagId = diag::err_bounds_declaration_unchecked_local_array; - } + if (Expr && Expr->isInvalid()) { + D->setBoundsExpr(Expr); + return; + } - if (DiagId) { - Diag(Expr->getLocStart(), DiagId) << D; - ActOnInvalidBoundsDecl(D); - return; - } + VarDecl *VD = dyn_cast(D); + if (Expr && VD && VD->isLocalVarDecl()) { + // - Local variables cannot have bounds-safe interface type annotations. + // - Local variables with unchecked pointer or array types cannot have + // bounds declarations. For bounds declarations, this reduces the chance + // that programmers accidentally declare variables with unchecked types + // to have bounds declarations and think uses of the variables will be + // bounds checked. + // + // Other declarations (parameters, globally-scoped variables, and members) + // can have unchecked pointer or array types with bounds declarations + // because that is a way bounds-safe interfaces are declared. + int DiagId = 0; + if (Expr->isInteropTypeAnnotation()) + DiagId = diag::err_bounds_safe_interface_type_annotation_local_variable; + else { + if (Ty->isPointerType() && !Ty->isCheckedPointerType()) + DiagId = diag::err_bounds_declaration_unchecked_local_pointer; + else if (Ty->isArrayType() && !Ty->isCheckedArrayType()) + DiagId = diag::err_bounds_declaration_unchecked_local_array; + } + + if (DiagId) { + Diag(Expr->getLocStart(), DiagId) << D; + ActOnInvalidBoundsDecl(D); + return; } } - if (DiagnoseBoundsDeclType(Ty, D, Expr, /*IsReturnBounds=*/false)) + if (Expr && DiagnoseBoundsDeclType(Ty, D, Expr, /*IsReturnBounds=*/false)) { ActOnInvalidBoundsDecl(D); + return; + } + + // Handle VarDecls specially. There needs to be a check that the bounds + // do not conflict with bounds for a prior declaration (if there is one) + // before attaching the bounds. + if (VD) + HandleVarDeclBounds(*this, VD, Expr); else D->setBoundsExpr(Expr); } From e5b8ec17b9e0207d792561a817c744f9aa36871f Mon Sep 17 00:00:00 2001 From: David Tarditi Date: Thu, 22 Dec 2016 11:42:59 -0800 Subject: [PATCH 3/4] Fix overly strict checking for unchecked array variables. The more relaxed compatiblity rules for bounds declarations were not being applied to unchecked array variables. Generalize the check for uncheckedness to include unchecked arrays. --- include/clang/AST/Type.h | 7 +++++++ include/clang/Basic/DiagnosticSemaKinds.td | 2 +- lib/Parse/ParseDecl.cpp | 1 - lib/Sema/SemaDecl.cpp | 16 +++++++++------- 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/include/clang/AST/Type.h b/include/clang/AST/Type.h index 0ee8d9d5aee1..7f7108d5f2db 100644 --- a/include/clang/AST/Type.h +++ b/include/clang/AST/Type.h @@ -1713,6 +1713,7 @@ class Type : public ExtQualsTypeCommonBase { bool isDependentSizedArrayType() const; /// \brief whether this is a Checked C checked array type. bool isCheckedArrayType() const; + bool isUncheckedArrayType() const; bool isRecordType() const; bool isClassType() const; bool isStructureType() const; @@ -5670,6 +5671,12 @@ inline bool Type::isCheckedArrayType() const { else return false; } +inline bool Type::isUncheckedArrayType() const { + if (const ArrayType *T = dyn_cast(CanonicalType)) + return !T->isChecked(); + else + return false; +} inline bool Type::isBuiltinType() const { return isa(CanonicalType); } diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index ca113616ee82..24a24da0d0e4 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -8854,7 +8854,7 @@ def err_bounds_type_annotation_lost_checking : Error< def err_decl_dropped_bounds : Error< "%select{function redeclaration dropped bounds for parameter|function " - "redeclaration dropped return bounds|variable declaration dropped " + "redeclaration dropped return bounds|variable redeclaration dropped " "bounds}0">; def err_conflicting_bounds : Error<"conflicting bounds for %0">; diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp index 511aaf0ec61f..7321f518b116 100644 --- a/lib/Parse/ParseDecl.cpp +++ b/lib/Parse/ParseDecl.cpp @@ -2086,7 +2086,6 @@ Decl *Parser::ParseDeclarationAfterDeclaratorAndAttributes( bool TypeContainsAuto = D.getDeclSpec().containsPlaceholderType(); - // If this is a variable declarator in Checked C, parse the bounds expression // (if any) and set the bounds expression. Function declarators are ignored // here because return bounds expressions are/ parsed as part of function diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp index 91b368ad0f46..426cd172d428 100644 --- a/lib/Sema/SemaDecl.cpp +++ b/lib/Sema/SemaDecl.cpp @@ -3334,7 +3334,7 @@ static void emitBoundsErrorDiagnostic(Sema &S, int DiagId, const SourceLocation BoundsLoc, const DeclaratorDecl *Old, const DeclaratorDecl *New, - bool IsUncheckedPointerType, + bool IsUncheckedType, Sema::CheckedCBoundsError Kind) { // Emit the diagnostic, pointing at the current bounds expression // if possible. Use the new declaration if there is no bounds @@ -3353,11 +3353,11 @@ static void emitBoundsErrorDiagnostic(Sema &S, int DiagId, // First determine the prior relevant bounds expression, if there is one. const BoundsExpr *PrevBoundsExpr = Old->getBoundsExpr(); - // The bounds expression for an unchecked pointer type may have + // The bounds expression for an unchecked pointer or array type may have // been inherited from an earlier declaration than Old that // was compatible with Old. If there's no bounds expression on Old, // search for a possible earlier definition. - if (!PrevBoundsExpr && IsUncheckedPointerType) { + if (!PrevBoundsExpr && IsUncheckedType) { if (const FunctionDecl *Previous = dyn_cast(Old)) { do { PrevBoundsExpr = Previous->getBoundsExpr(); @@ -3440,8 +3440,10 @@ static bool diagnoseBoundsError(Sema &S, QualType NewType, Sema::CheckedCBoundsError Kind) { int DiagId = 0; - bool IsUncheckedPointerType = OldType->isUncheckedPointerType() && - NewType->isUncheckedPointerType(); + bool IsUncheckedType = + (OldType->isUncheckedPointerType() && NewType->isUncheckedPointerType()) || + (OldType->isUncheckedArrayType() && NewType->isUncheckedArrayType()); + if (OldBounds && NewBounds) { if (OldBounds->isInvalid() || NewBounds->isInvalid()) // There must have been an earlier error involving @@ -3451,13 +3453,13 @@ static bool diagnoseBoundsError(Sema &S, if (!S.Context.EquivalentBounds(OldBounds, NewBounds)) DiagId = diag::err_decl_conflicting_bounds; } else if (OldBounds || NewBounds) { - if (!IsUncheckedPointerType) + if (!IsUncheckedType) DiagId = NewBounds ? diag::err_decl_added_bounds : diag::err_decl_dropped_bounds; } if (DiagId) { emitBoundsErrorDiagnostic(S, DiagId, BoundsLoc, OldDecl, NewDecl, - IsUncheckedPointerType, Kind); + IsUncheckedType, Kind); return true; } // TODO: produce better error messages when types for parameters, returns, From 9d221e380aa9e1dfeb3416121ac4d00528ffc421 Mon Sep 17 00:00:00 2001 From: David Tarditi Date: Thu, 22 Dec 2016 13:04:10 -0800 Subject: [PATCH 4/4] Fix stray typos in comments. --- lib/Parse/ParseDecl.cpp | 2 +- lib/Sema/SemaDecl.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp index 7321f518b116..8ddd2df02ccd 100644 --- a/lib/Parse/ParseDecl.cpp +++ b/lib/Parse/ParseDecl.cpp @@ -2088,7 +2088,7 @@ Decl *Parser::ParseDeclarationAfterDeclaratorAndAttributes( // If this is a variable declarator in Checked C, parse the bounds expression // (if any) and set the bounds expression. Function declarators are ignored - // here because return bounds expressions are/ parsed as part of function + // here because return bounds expressions are parsed as part of function // declarators already. if (getLangOpts().CheckedC && isa(ThisDecl)) { VarDecl *ThisVarDecl = dyn_cast(ThisDecl); diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp index 426cd172d428..524c294defb5 100644 --- a/lib/Sema/SemaDecl.cpp +++ b/lib/Sema/SemaDecl.cpp @@ -3411,14 +3411,14 @@ static void emitBoundsErrorDiagnostic(Sema &S, int DiagId, } // Shared logic for diagnosing bounds declaration conflicts for parameters, -// returns, and variables +// returns, and variables. // // * BoundsLoc is the source location of the bounds expression at which to // report the error. If there is no bounds expression, this will be an invalid // source location. (We don't use the location from the new bounds expression // beacuse it is canonicalized and may not have valid line information.) // * OldBounds and NewBounds are canoncialized bounds expressions. For -// parameters and returns, they are bounds expression from a function type. +// parameters and returns, they are bounds expressions from function types. // It is important to use canonicalized bound expressions these for bounds // comparisons. // * OldDecl and NewDecl provide the declarations for use in error messages. @@ -3426,7 +3426,7 @@ static void emitBoundsErrorDiagnostic(Sema &S, int DiagId, // may not be attached to NewDecl yet. // * OldType and NewType are the types of the items whoses bounds declarations // are being checked. We pass them in because it is easier to compute them at -// the caller than than to compute them here. +// the caller than to compute them here. // * Kind is what is being checked. // // Return true if an error involving bounds has been diagnosed, false if not.