Index: clang/lib/Analysis/UnsafeBufferUsage.cpp =================================================================== --- clang/lib/Analysis/UnsafeBufferUsage.cpp +++ clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -1177,42 +1177,40 @@ // 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 is alwasy valid"); - return Loc; + if(Loc.isValid()) + return Loc; + else + 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 is alwasy valid"); - return Loc; + if(Loc.isValid()) + return Loc; + else + 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); - return Lexer::getSourceText( - CharSourceRange::getCharRange(E->getBeginLoc(), LastCharLoc), SM, - LangOpts); + if(LastCharLoc) + return Lexer::getSourceText( + CharSourceRange::getCharRange(E->getBeginLoc(), *LastCharLoc), SM, + LangOpts); + else + return std::nullopt; } std::optional @@ -1256,10 +1254,15 @@ const LangOptions &LangOpts = Ctx.getLangOpts(); SmallString<32> StrBuffer; + std::optional dreString = getExprText(DRE, SM, LangOpts); + if(!dreString) return std::nullopt; + std::optional indexString = getExprText(Idx, SM, LangOpts); + if(!indexString) return std::nullopt; + StrBuffer.append("("); - StrBuffer.append(getExprText(DRE, SM, LangOpts)); + StrBuffer.append(*dreString); StrBuffer.append(".data() + "); - StrBuffer.append(getExprText(Idx, SM, LangOpts)); + StrBuffer.append(*indexString); StrBuffer.append(")"); return FixItList{FixItHint::CreateReplacement(Node->getSourceRange(), StrBuffer.str())}; @@ -1311,12 +1314,14 @@ // To transform UPC(++p) to UPC((p = p.subspan(1)).data()): SS << "(" << varName.data() << " = " << varName.data() << ".subspan(1)).data()"; - Fixes.push_back(FixItHint::CreateReplacement( - SourceRange(PreIncNode->getBeginLoc(), - getEndCharLoc(PreIncNode, Ctx.getSourceManager(), - Ctx.getLangOpts())), - SS.str())); - return Fixes; + std::optional location = + getEndCharLoc(PreIncNode, Ctx.getSourceManager(), Ctx.getLangOpts()); + + if(location) { + Fixes.push_back(FixItHint::CreateReplacement( + SourceRange(PreIncNode->getBeginLoc(), *location), SS.str())); + return Fixes; + } } } return std::nullopt; // Not in the cases that we can handle for now, give up. @@ -1350,9 +1355,13 @@ // &`? It should. Maybe worth an NFC patch later. Expr::NullPointerConstantValueDependence:: NPC_ValueDependentIsNotNull)) { - SourceRange SR(Init->getBeginLoc(), getEndCharLoc(Init, SM, LangOpts)); - - return {FixItHint::CreateRemoval(SR)}; + std::optional location = getEndCharLoc(Init, SM, LangOpts); + if(location) { + SourceRange SR(Init->getBeginLoc(), *location); + return {FixItHint::CreateRemoval(SR)}; + } else { + return {}; + } } FixItList FixIts{}; @@ -1368,8 +1377,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 +1404,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 +1435,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,9 +1443,8 @@ FixItList InitFixIts = populateInitializerFixItWithSpan(Init, Ctx, UserFillPlaceHolder); - assert(!InitFixIts.empty() && - "Expected at least one fix-it for an initializer of an 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()), @@ -1443,8 +1457,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 @@ -3,6 +3,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]; @@ -223,3 +225,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]" +}