Index: clang/lib/Analysis/UnsafeBufferUsage.cpp =================================================================== --- clang/lib/Analysis/UnsafeBufferUsage.cpp +++ clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -1117,42 +1117,42 @@ // Return the source location of the last character of the AST `Node`. template -static SourceLocation getEndCharLoc(const NodeTy *Node, const SourceManager &SM, +static std::optional getEndCharLoc(const NodeTy *Node, const SourceManager &SM, const LangOptions &LangOpts) { unsigned TkLen = Lexer::MeasureTokenLength(Node->getEndLoc(), SM, LangOpts); SourceLocation Loc = Node->getEndLoc().getLocWithOffset(TkLen - 1); - - // We expect `Loc` to be valid. The location is obtained by moving forward - // from the beginning of the token 'len(token)-1' characters. The file ID of - // the locations within a token must be consistent. - assert(Loc.isValid() && "Expected the source location of the last" - "character of an AST Node to be always valid"); - return Loc; + + if (Loc.isValid()) + return Loc; + + return std::nullopt; } // Return the source location just past the last character of the AST `Node`. template -static SourceLocation getPastLoc(const NodeTy *Node, const SourceManager &SM, +static std::optional getPastLoc(const NodeTy *Node, const SourceManager &SM, const LangOptions &LangOpts) { SourceLocation Loc = Lexer::getLocForEndOfToken(Node->getEndLoc(), 0, SM, LangOpts); - // We expect `Loc` to be valid as it is either associated to a file ID or - // it must be the end of a macro expansion. (see - // `Lexer::getLocForEndOfToken`) - assert(Loc.isValid() && "Expected the source location just past the last " - "character of an AST Node to be always valid"); - return Loc; + if (Loc.isValid()) + return Loc; + + return std::nullopt; + } // Return text representation of an `Expr`. -static StringRef getExprText(const Expr *E, const SourceManager &SM, +static std::optional getExprText(const Expr *E, const SourceManager &SM, const LangOptions &LangOpts) { - SourceLocation LastCharLoc = getPastLoc(E, SM, LangOpts); + std::optional LastCharLoc = getPastLoc(E, SM, LangOpts); + + if (LastCharLoc) + return Lexer::getSourceText( + CharSourceRange::getCharRange(E->getBeginLoc(), *LastCharLoc), SM, + LangOpts); - return Lexer::getSourceText( - CharSourceRange::getCharRange(E->getBeginLoc(), LastCharLoc), SM, - LangOpts); + return std::nullopt; } std::optional @@ -1191,12 +1191,20 @@ CharSourceRange StarWithTrailWhitespace = clang::CharSourceRange::getCharRange(DerefOp->getOperatorLoc(), LHS->getBeginLoc()); + + std::optional LHSLocation = getPastLoc(LHS, SM, LangOpts); + if(!LHSLocation) return std::nullopt; + CharSourceRange PlusWithSurroundingWhitespace = - clang::CharSourceRange::getCharRange(getPastLoc(LHS, SM, LangOpts), - RHS->getBeginLoc()); + clang::CharSourceRange::getCharRange(*LHSLocation, RHS->getBeginLoc()); + + std::optional AddOpLocation = getPastLoc(AddOp, SM, LangOpts); + std::optional DerefOpLocation = getPastLoc(DerefOp, SM, LangOpts); + + if(!AddOpLocation || !DerefOpLocation) return std::nullopt; + CharSourceRange ClosingParenWithPrecWhitespace = - clang::CharSourceRange::getCharRange(getPastLoc(AddOp, SM, LangOpts), - getPastLoc(DerefOp, SM, LangOpts)); + clang::CharSourceRange::getCharRange(*AddOpLocation, *DerefOpLocation); return FixItList{ {FixItHint::CreateRemoval(StarWithTrailWhitespace), @@ -1223,7 +1231,7 @@ if (endOfOperand) { return FixItList{{FixItHint::CreateRemoval(derefRange), FixItHint::CreateInsertion( - endOfOperand.value().getLocWithOffset(1), "[0]")}}; + (*endOfOperand).getLocWithOffset(1), "[0]")}}; } } [[fallthrough]]; @@ -1248,10 +1256,11 @@ ASTContext &Ctx = VD->getASTContext(); SourceManager &SM = Ctx.getSourceManager(); // Inserts the .data() after the DRE - SourceLocation endOfOperand = getEndCharLoc(Node, SM, Ctx.getLangOpts()); - - return FixItList{{FixItHint::CreateInsertion( - endOfOperand.getLocWithOffset(1), ".data()")}}; + std::optional endOfOperand = getEndCharLoc(Node, SM, Ctx.getLangOpts()); + + if(endOfOperand) + return FixItList{{FixItHint::CreateInsertion( + (*endOfOperand).getLocWithOffset(1), ".data()")}}; } case Strategy::Kind::Wontfix: case Strategy::Kind::Iterator: @@ -1281,12 +1290,18 @@ if (auto ICE = Idx->getIntegerConstantExpr(Ctx)) if ((*ICE).isZero()) IdxIsLitZero = true; + std::optional dreString = getExprText(DRE, SM, LangOpts); + if (!dreString) return std::nullopt; + if (IdxIsLitZero) { // If the index is literal zero, we produce the most concise fix-it: - SS << getExprText(DRE, SM, LangOpts).str() << ".data()"; + SS << (*dreString).str() << ".data()"; } else { - SS << "&" << getExprText(DRE, SM, LangOpts).str() << ".data()" - << "[" << getExprText(Idx, SM, LangOpts).str() << "]"; + std::optional indexString = getExprText(Idx, SM, LangOpts); + if (!indexString) return std::nullopt; + + SS << "&" << (*dreString).str() << ".data()" + << "[" << (*indexString).str() << "]"; } return FixItList{ FixItHint::CreateReplacement(Node->getSourceRange(), SS.str())}; @@ -1310,11 +1325,12 @@ // To transform UPC(++p) to UPC((p = p.subspan(1)).data()): SS << "(" << varName.data() << " = " << varName.data() << ".subspan(1)).data()"; + std::optional preIncLocation = getEndCharLoc(PreIncNode, + Ctx.getSourceManager(),Ctx.getLangOpts()); + if(!preIncLocation) return std::nullopt; + Fixes.push_back(FixItHint::CreateReplacement( - SourceRange(PreIncNode->getBeginLoc(), - getEndCharLoc(PreIncNode, Ctx.getSourceManager(), - Ctx.getLangOpts())), - SS.str())); + SourceRange(PreIncNode->getBeginLoc(), *preIncLocation), SS.str())); return Fixes; } } @@ -1350,7 +1366,10 @@ // &`? It should. Maybe worth an NFC patch later. Expr::NullPointerConstantValueDependence:: NPC_ValueDependentIsNotNull)) { - SourceRange SR(Init->getBeginLoc(), getEndCharLoc(Init, SM, LangOpts)); + std::optional InitLocation = getEndCharLoc(Init, SM, LangOpts); + if(!InitLocation) return {}; + + SourceRange SR(Init->getBeginLoc(), *InitLocation); return {FixItHint::CreateRemoval(SR)}; } @@ -1368,8 +1387,11 @@ // of `T`. So the extent is `n` unless `n` has side effects. Similar but // simpler for the case where `Init` is `new T`. if (const Expr *Ext = CxxNew->getArraySize().value_or(nullptr)) { - if (!Ext->HasSideEffects(Ctx)) - ExtentText = getExprText(Ext, SM, LangOpts); + if (!Ext->HasSideEffects(Ctx)) { + std::optional extentString = getExprText(Ext, SM, LangOpts); + if (!extentString) return {}; + ExtentText = *extentString; + } } else if (!CxxNew->isArray()) // Although the initializer is not allocating a buffer, the pointer // variable could still be used in buffer access operations. @@ -1392,12 +1414,15 @@ } SmallString<32> StrBuffer{}; - SourceLocation LocPassInit = getPastLoc(Init, SM, LangOpts); + std::optional LocPassInit = getPastLoc(Init, SM, LangOpts); + + if (!LocPassInit) + return {}; StrBuffer.append(", "); StrBuffer.append(ExtentText); StrBuffer.append("}"); - FixIts.push_back(FixItHint::CreateInsertion(LocPassInit, StrBuffer.str())); + FixIts.push_back(FixItHint::CreateInsertion(*LocPassInit, StrBuffer.str())); return FixIts; } @@ -1420,7 +1445,7 @@ assert(!SpanEltT.isNull() && "Trying to fix a non-pointer type variable!"); FixItList FixIts{}; - SourceLocation + std::optional ReplacementLastLoc; // the inclusive end location of the replacement const SourceManager &SM = Ctx.getSourceManager(); @@ -1428,8 +1453,9 @@ FixItList InitFixIts = populateInitializerFixItWithSpan(Init, Ctx, UserFillPlaceHolder); - assert(!InitFixIts.empty() && - "Unable to fix initializer of unsafe variable declaration"); + if(InitFixIts.empty()) + return {}; + // The loc right before the initializer: ReplacementLastLoc = Init->getBeginLoc().getLocWithOffset(-1); FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts.begin()), @@ -1442,8 +1468,12 @@ raw_svector_ostream OS(Replacement); OS << "std::span<" << SpanEltT.getAsString() << "> " << D->getName(); + + if (!ReplacementLastLoc) + return {}; + FixIts.push_back(FixItHint::CreateReplacement( - SourceRange{D->getBeginLoc(), ReplacementLastLoc}, OS.str())); + SourceRange{D->getBeginLoc(), *ReplacementLastLoc}, OS.str())); return FixIts; } Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp =================================================================== --- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp @@ -2,6 +2,8 @@ typedef int * Int_ptr_t; typedef int Int_t; +#define DEFINE_PTR(X) int* ptr = (X); + void local_array_subscript_simple() { int tmp; int *p = new int[10]; @@ -222,3 +224,28 @@ tmp = r[j] + r[k]; // both `j` and `k` are unsigned so they must be non-negative tmp = r[(unsigned int)-1]; // a cast-to-unsigned-expression is also non-negative } + +void all_vars_in_macro() { + int* local; + DEFINE_PTR(local) + ptr[1] = 0; +} + +void few_vars_in_macro() { + int* local; + DEFINE_PTR(local) + ptr[1] = 0; + int tmp; + ptr[2] = 30; + auto p = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + tmp = p[5]; + int val = *p; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"[0]" + val = *p + 30; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:11-[[@LINE-2]]:11}:"[0]" +}