[clang] 84ec1f7 - Revert "[-Wunsafe-buffer-usage] Bug fix: Handles the assertion violations for code within macros"
via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 24 16:48:56 PDT 2023
Author: MalavikaSamak
Date: 2023-04-24T16:48:46-07:00
New Revision: 84ec1f7725d4f4575474b59467e598d7c5528a4e
URL: https://github.com/llvm/llvm-project/commit/84ec1f7725d4f4575474b59467e598d7c5528a4e
DIFF: https://github.com/llvm/llvm-project/commit/84ec1f7725d4f4575474b59467e598d7c5528a4e.diff
LOG: Revert "[-Wunsafe-buffer-usage] Bug fix: Handles the assertion violations for code within macros"
This reverts commit 9bd0db80784e30d40a4a65f1b47109c833f05b54.
Added:
Modified:
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
Removed:
################################################################################
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 3b58a51195f8..c53ecb52384d 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1117,44 +1117,42 @@ static std::string getAPIntText(APInt Val) {
// Return the source location of the last character of the AST `Node`.
template <typename NodeTy>
-static std::optional<SourceLocation>
-getEndCharLoc(const NodeTy *Node, const SourceManager &SM,
- const LangOptions &LangOpts) {
+static SourceLocation 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);
- if (Loc.isValid())
- return Loc;
-
- return std::nullopt;
+ // 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;
}
// Return the source location just past the last character of the AST `Node`.
template <typename NodeTy>
-static std::optional<SourceLocation> getPastLoc(const NodeTy *Node,
- const SourceManager &SM,
- const LangOptions &LangOpts) {
+static SourceLocation getPastLoc(const NodeTy *Node, const SourceManager &SM,
+ const LangOptions &LangOpts) {
SourceLocation Loc =
Lexer::getLocForEndOfToken(Node->getEndLoc(), 0, SM, LangOpts);
- if (Loc.isValid())
- return Loc;
-
- return std::nullopt;
+ // 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;
}
// Return text representation of an `Expr`.
-static std::optional<StringRef> getExprText(const Expr *E,
- const SourceManager &SM,
- const LangOptions &LangOpts) {
- std::optional<SourceLocation> LastCharLoc = getPastLoc(E, SM, LangOpts);
-
- if (LastCharLoc)
- return Lexer::getSourceText(
- CharSourceRange::getCharRange(E->getBeginLoc(), *LastCharLoc), SM,
- LangOpts);
+static StringRef getExprText(const Expr *E, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ SourceLocation LastCharLoc = getPastLoc(E, SM, LangOpts);
- return std::nullopt;
+ return Lexer::getSourceText(
+ CharSourceRange::getCharRange(E->getBeginLoc(), LastCharLoc), SM,
+ LangOpts);
}
std::optional<FixItList>
@@ -1193,24 +1191,12 @@ DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const {
CharSourceRange StarWithTrailWhitespace =
clang::CharSourceRange::getCharRange(DerefOp->getOperatorLoc(),
LHS->getBeginLoc());
-
- std::optional<SourceLocation> LHSLocation = getPastLoc(LHS, SM, LangOpts);
- if (!LHSLocation)
- return std::nullopt;
-
CharSourceRange PlusWithSurroundingWhitespace =
- clang::CharSourceRange::getCharRange(*LHSLocation, RHS->getBeginLoc());
-
- std::optional<SourceLocation> AddOpLocation =
- getPastLoc(AddOp, SM, LangOpts);
- std::optional<SourceLocation> DerefOpLocation =
- getPastLoc(DerefOp, SM, LangOpts);
-
- if (!AddOpLocation || !DerefOpLocation)
- return std::nullopt;
-
+ clang::CharSourceRange::getCharRange(getPastLoc(LHS, SM, LangOpts),
+ RHS->getBeginLoc());
CharSourceRange ClosingParenWithPrecWhitespace =
- clang::CharSourceRange::getCharRange(*AddOpLocation, *DerefOpLocation);
+ clang::CharSourceRange::getCharRange(getPastLoc(AddOp, SM, LangOpts),
+ getPastLoc(DerefOp, SM, LangOpts));
return FixItList{
{FixItHint::CreateRemoval(StarWithTrailWhitespace),
@@ -1232,12 +1218,12 @@ PointerDereferenceGadget::getFixits(const Strategy &S) const {
CharSourceRange derefRange = clang::CharSourceRange::getCharRange(
Op->getBeginLoc(), Op->getBeginLoc().getLocWithOffset(1));
// Inserts the [0]
- std::optional<SourceLocation> EndOfOperand =
+ std::optional<SourceLocation> endOfOperand =
getEndCharLoc(BaseDeclRefExpr, SM, Ctx.getLangOpts());
- if (EndOfOperand) {
+ if (endOfOperand) {
return FixItList{{FixItHint::CreateRemoval(derefRange),
FixItHint::CreateInsertion(
- (*EndOfOperand).getLocWithOffset(1), "[0]")}};
+ endOfOperand.value().getLocWithOffset(1), "[0]")}};
}
}
[[fallthrough]];
@@ -1262,12 +1248,10 @@ std::optional<FixItList> UPCStandalonePointerGadget::getFixits(const Strategy &S
ASTContext &Ctx = VD->getASTContext();
SourceManager &SM = Ctx.getSourceManager();
// Inserts the .data() after the DRE
- std::optional<SourceLocation> EndOfOperand =
- getPastLoc(Node, SM, Ctx.getLangOpts());
+ SourceLocation endOfOperand = getEndCharLoc(Node, SM, Ctx.getLangOpts());
- if (EndOfOperand)
- return FixItList{{FixItHint::CreateInsertion(
- *EndOfOperand, ".data()")}};
+ return FixItList{{FixItHint::CreateInsertion(
+ endOfOperand.getLocWithOffset(1), ".data()")}};
}
[[fallthrough]];
case Strategy::Kind::Wontfix:
@@ -1298,20 +1282,12 @@ fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node) {
if (auto ICE = Idx->getIntegerConstantExpr(Ctx))
if ((*ICE).isZero())
IdxIsLitZero = true;
- std::optional<StringRef> 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 << (*DreString).str() << ".data()";
+ SS << getExprText(DRE, SM, LangOpts).str() << ".data()";
} else {
- std::optional<StringRef> IndexString = getExprText(Idx, SM, LangOpts);
- if (!IndexString)
- return std::nullopt;
-
- SS << "&" << (*DreString).str() << ".data()"
- << "[" << (*IndexString).str() << "]";
+ SS << "&" << getExprText(DRE, SM, LangOpts).str() << ".data()"
+ << "[" << getExprText(Idx, SM, LangOpts).str() << "]";
}
return FixItList{
FixItHint::CreateReplacement(Node->getSourceRange(), SS.str())};
@@ -1335,13 +1311,11 @@ std::optional<FixItList> UPCPreIncrementGadget::getFixits(const Strategy &S) con
// To transform UPC(++p) to UPC((p = p.subspan(1)).data()):
SS << "(" << varName.data() << " = " << varName.data()
<< ".subspan(1)).data()";
- std::optional<SourceLocation> PreIncLocation =
- getEndCharLoc(PreIncNode, Ctx.getSourceManager(), Ctx.getLangOpts());
- if (!PreIncLocation)
- return std::nullopt;
-
Fixes.push_back(FixItHint::CreateReplacement(
- SourceRange(PreIncNode->getBeginLoc(), *PreIncLocation), SS.str()));
+ SourceRange(PreIncNode->getBeginLoc(),
+ getEndCharLoc(PreIncNode, Ctx.getSourceManager(),
+ Ctx.getLangOpts())),
+ SS.str()));
return Fixes;
}
}
@@ -1377,12 +1351,7 @@ populateInitializerFixItWithSpan(const Expr *Init, const ASTContext &Ctx,
// &`? It should. Maybe worth an NFC patch later.
Expr::NullPointerConstantValueDependence::
NPC_ValueDependentIsNotNull)) {
- std::optional<SourceLocation> InitLocation =
- getEndCharLoc(Init, SM, LangOpts);
- if (!InitLocation)
- return {};
-
- SourceRange SR(Init->getBeginLoc(), *InitLocation);
+ SourceRange SR(Init->getBeginLoc(), getEndCharLoc(Init, SM, LangOpts));
return {FixItHint::CreateRemoval(SR)};
}
@@ -1400,12 +1369,8 @@ populateInitializerFixItWithSpan(const Expr *Init, const ASTContext &Ctx,
// 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)) {
- std::optional<StringRef> ExtentString = getExprText(Ext, SM, LangOpts);
- if (!ExtentString)
- return {};
- ExtentText = *ExtentString;
- }
+ if (!Ext->HasSideEffects(Ctx))
+ ExtentText = getExprText(Ext, SM, LangOpts);
} else if (!CxxNew->isArray())
// Although the initializer is not allocating a buffer, the pointer
// variable could still be used in buffer access operations.
@@ -1428,15 +1393,12 @@ populateInitializerFixItWithSpan(const Expr *Init, const ASTContext &Ctx,
}
SmallString<32> StrBuffer{};
- std::optional<SourceLocation> LocPassInit = getPastLoc(Init, SM, LangOpts);
-
- if (!LocPassInit)
- return {};
+ SourceLocation LocPassInit = getPastLoc(Init, SM, LangOpts);
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;
}
@@ -1459,7 +1421,7 @@ static FixItList fixVarDeclWithSpan(const VarDecl *D, const ASTContext &Ctx,
assert(!SpanEltT.isNull() && "Trying to fix a non-pointer type variable!");
FixItList FixIts{};
- std::optional<SourceLocation>
+ SourceLocation
ReplacementLastLoc; // the inclusive end location of the replacement
const SourceManager &SM = Ctx.getSourceManager();
@@ -1467,9 +1429,8 @@ static FixItList fixVarDeclWithSpan(const VarDecl *D, const ASTContext &Ctx,
FixItList InitFixIts =
populateInitializerFixItWithSpan(Init, Ctx, UserFillPlaceHolder);
- if (InitFixIts.empty())
- return {};
-
+ assert(!InitFixIts.empty() &&
+ "Unable to fix initializer of unsafe variable declaration");
// The loc right before the initializer:
ReplacementLastLoc = Init->getBeginLoc().getLocWithOffset(-1);
FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts.begin()),
@@ -1482,12 +1443,8 @@ static FixItList fixVarDeclWithSpan(const VarDecl *D, const ASTContext &Ctx,
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;
}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
index cb6519a153fe..ca3f7a4b172a 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -2,8 +2,6 @@
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];
@@ -224,28 +222,3 @@ void unsupported_subscript_negative(int i, unsigned j, unsigned long k) {
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<int> 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]"
-}
More information about the cfe-commits
mailing list