[PATCH] D26435: Use unique_ptr for cached tokens for default arguments in C++.
dtarditi added a comment. I sync'ed to the head of the tree. I tried to reproduce the failures that you saw and couldn't.I'm building on Windows 10 x64 using Visual Studio. What platform/OS did you build on? I built and test both Debug and RelWithDebugInfo. Here's what I saw for RelWithDebugInfo Running the Clang regression tests -- Testing: 10026 tests, 24 threads -- Testing Time: 604.17s Expected Passes: 9922 Expected Failures : 21 Unsupported Tests : 83 https://reviews.llvm.org/D26435 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26435: Use unique_ptr for cached tokens for default arguments in C++.
dtarditi updated this revision to Diff 78375.
dtarditi added a comment.
The parameter array needed to be initialized so that assignments involving
unique pointers work properly. The memory could be uninitialized according to
C++ semantics.. Visual C++ was zeroing the memory and GCC was not. This is
why the tests passed on Windows and failed on Linux. I updated
Sema/DeclSpec.cpp to zero the parameter array before it is used.
Here are the test results from an x64 Linux Ubuntu box:
Testing Time: 465.12s
Expected Passes: 10017
Expected Failures : 18
Unsupported Tests : 27
https://reviews.llvm.org/D26435
Files:
include/clang/Parse/Parser.h
include/clang/Sema/DeclSpec.h
lib/Parse/ParseCXXInlineMethods.cpp
lib/Parse/ParseDecl.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Sema/DeclSpec.cpp
lib/Sema/SemaDeclCXX.cpp
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -395,17 +395,15 @@
++argIdx) {
ParmVarDecl *Param = cast(chunk.Fun.Params[argIdx].Param);
if (Param->hasUnparsedDefaultArg()) {
- CachedTokens *Toks = chunk.Fun.Params[argIdx].DefaultArgTokens;
+ std::unique_ptr Toks = std::move(chunk.Fun.Params[argIdx].DefaultArgTokens);
SourceRange SR;
if (Toks->size() > 1)
SR = SourceRange((*Toks)[1].getLocation(),
Toks->back().getLocation());
else
SR = UnparsedDefaultArgLocs[Param];
Diag(Param->getLocation(), diag::err_param_default_argument_nonfunc)
<< SR;
- delete Toks;
- chunk.Fun.Params[argIdx].DefaultArgTokens = nullptr;
} else if (Param->getDefaultArg()) {
Diag(Param->getLocation(), diag::err_param_default_argument_nonfunc)
<< Param->getDefaultArg()->getSourceRange();
Index: lib/Sema/DeclSpec.cpp
===
--- lib/Sema/DeclSpec.cpp
+++ lib/Sema/DeclSpec.cpp
@@ -223,13 +223,19 @@
if (!TheDeclarator.InlineStorageUsed &&
NumParams <= llvm::array_lengthof(TheDeclarator.InlineParams)) {
I.Fun.Params = TheDeclarator.InlineParams;
+ // Zero the memory block so that unique pointers are initialized
+ // properly.
+ memset(I.Fun.Params, 0, sizeof(Params[0]) * NumParams);
I.Fun.DeleteParams = false;
TheDeclarator.InlineStorageUsed = true;
} else {
- I.Fun.Params = new DeclaratorChunk::ParamInfo[NumParams];
+ // Call the version of new that zeroes memory so that unique pointers
+ // are initialized properly.
+ I.Fun.Params = new DeclaratorChunk::ParamInfo[NumParams]();
I.Fun.DeleteParams = true;
}
-memcpy(I.Fun.Params, Params, sizeof(Params[0]) * NumParams);
+for (unsigned i = 0; i < NumParams; i++)
+ I.Fun.Params[i] = std::move(Params[i]);
}
// Check what exception specification information we should actually store.
Index: lib/Parse/ParseDeclCXX.cpp
===
--- lib/Parse/ParseDeclCXX.cpp
+++ lib/Parse/ParseDeclCXX.cpp
@@ -2039,7 +2039,7 @@
LateMethod->DefaultArgs.reserve(FTI.NumParams);
for (unsigned ParamIdx = 0; ParamIdx < FTI.NumParams; ++ParamIdx)
LateMethod->DefaultArgs.push_back(LateParsedDefaultArgument(
-FTI.Params[ParamIdx].Param, FTI.Params[ParamIdx].DefaultArgTokens));
+FTI.Params[ParamIdx].Param, std::move(FTI.Params[ParamIdx].DefaultArgTokens)));
}
}
Index: lib/Parse/ParseDecl.cpp
===
--- lib/Parse/ParseDecl.cpp
+++ lib/Parse/ParseDecl.cpp
@@ -6022,7 +6022,7 @@
// DefArgToks is used when the parsing of default arguments needs
// to be delayed.
-CachedTokens *DefArgToks = nullptr;
+std::unique_ptr DefArgToks;
// If no parameter was specified, verify that *something* was specified,
// otherwise we have a missing type and identifier.
@@ -6058,13 +6058,11 @@
// If we're inside a class definition, cache the tokens
// corresponding to the default argument. We'll actually parse
// them when we see the end of the class definition.
- // FIXME: Can we use a smart pointer for Toks?
- DefArgToks = new CachedTokens;
+ DefArgToks.reset(new CachedTokens);
SourceLocation ArgStartLoc = NextToken().getLocation();
if (!ConsumeAndStoreInitializer(*DefArgToks, CIK_DefaultArgument)) {
-delete DefArgToks;
-DefArgToks = nullptr;
+DefArgToks.reset();
Actions.ActOnParamDefaultArgumentError(Param, EqualLoc);
} else {
Actions.ActOnParamUnparsedDefaultArgument(Param, EqualLoc,
@@ -6100,7 +6098,7 @@
ParamInfo.push_back(DeclaratorChunk::ParamInfo(ParmII,
[PATCH] D26435: Use unique_ptr for cached tokens for default arguments in C++.
dtarditi created this revision.
dtarditi added a subscriber: cfe-commits.
This changes pointers to cached tokens for default arguments in C++ from raw
pointers to unique_ptrs. There was a fixme in the code where the cached tokens
are created about using a smart pointer.
The change is straightforward, though I did have to track down and fix a memory
corruption caused by the change. memcpy was being used to copy parameter
information. This duplicated the unique_ptr, which led to the cached token
buffer being deleted prematurely.
https://reviews.llvm.org/D26435
Files:
include/clang/Parse/Parser.h
include/clang/Sema/DeclSpec.h
lib/Parse/ParseCXXInlineMethods.cpp
lib/Parse/ParseDecl.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Sema/DeclSpec.cpp
lib/Sema/SemaDeclCXX.cpp
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -395,17 +395,15 @@
++argIdx) {
ParmVarDecl *Param = cast(chunk.Fun.Params[argIdx].Param);
if (Param->hasUnparsedDefaultArg()) {
- CachedTokens *Toks = chunk.Fun.Params[argIdx].DefaultArgTokens;
+ std::unique_ptr Toks = std::move(chunk.Fun.Params[argIdx].DefaultArgTokens);
SourceRange SR;
if (Toks->size() > 1)
SR = SourceRange((*Toks)[1].getLocation(),
Toks->back().getLocation());
else
SR = UnparsedDefaultArgLocs[Param];
Diag(Param->getLocation(), diag::err_param_default_argument_nonfunc)
<< SR;
- delete Toks;
- chunk.Fun.Params[argIdx].DefaultArgTokens = nullptr;
} else if (Param->getDefaultArg()) {
Diag(Param->getLocation(), diag::err_param_default_argument_nonfunc)
<< Param->getDefaultArg()->getSourceRange();
Index: lib/Sema/DeclSpec.cpp
===
--- lib/Sema/DeclSpec.cpp
+++ lib/Sema/DeclSpec.cpp
@@ -229,7 +229,8 @@
I.Fun.Params = new DeclaratorChunk::ParamInfo[NumParams];
I.Fun.DeleteParams = true;
}
-memcpy(I.Fun.Params, Params, sizeof(Params[0]) * NumParams);
+for (unsigned i = 0; i < NumParams; i++)
+ I.Fun.Params[i] = std::move(Params[i]);
}
// Check what exception specification information we should actually store.
Index: lib/Parse/ParseDeclCXX.cpp
===
--- lib/Parse/ParseDeclCXX.cpp
+++ lib/Parse/ParseDeclCXX.cpp
@@ -2039,7 +2039,7 @@
LateMethod->DefaultArgs.reserve(FTI.NumParams);
for (unsigned ParamIdx = 0; ParamIdx < FTI.NumParams; ++ParamIdx)
LateMethod->DefaultArgs.push_back(LateParsedDefaultArgument(
-FTI.Params[ParamIdx].Param, FTI.Params[ParamIdx].DefaultArgTokens));
+FTI.Params[ParamIdx].Param, std::move(FTI.Params[ParamIdx].DefaultArgTokens)));
}
}
Index: lib/Parse/ParseDecl.cpp
===
--- lib/Parse/ParseDecl.cpp
+++ lib/Parse/ParseDecl.cpp
@@ -6021,7 +6021,7 @@
// DefArgToks is used when the parsing of default arguments needs
// to be delayed.
-CachedTokens *DefArgToks = nullptr;
+std::unique_ptr DefArgToks;
// If no parameter was specified, verify that *something* was specified,
// otherwise we have a missing type and identifier.
@@ -6057,13 +6057,11 @@
// If we're inside a class definition, cache the tokens
// corresponding to the default argument. We'll actually parse
// them when we see the end of the class definition.
- // FIXME: Can we use a smart pointer for Toks?
- DefArgToks = new CachedTokens;
+ DefArgToks.reset(new CachedTokens);
SourceLocation ArgStartLoc = NextToken().getLocation();
if (!ConsumeAndStoreInitializer(*DefArgToks, CIK_DefaultArgument)) {
-delete DefArgToks;
-DefArgToks = nullptr;
+DefArgToks.release();
Actions.ActOnParamDefaultArgumentError(Param, EqualLoc);
} else {
Actions.ActOnParamUnparsedDefaultArgument(Param, EqualLoc,
@@ -6099,7 +6097,7 @@
ParamInfo.push_back(DeclaratorChunk::ParamInfo(ParmII,
ParmDeclarator.getIdentifierLoc(),
- Param, DefArgToks));
+ Param, std::move(DefArgToks)));
}
if (TryConsumeToken(tok::ellipsis, EllipsisLoc)) {
Index: lib/Parse/ParseCXXInlineMethods.cpp
===
--- lib/Parse/ParseCXXInlineMethods.cpp
+++ lib/Parse/ParseCXXInlineMethods.cpp
@@ -319,7 +319,8 @@
// Introduce the parameter into scope.
bool HasUnparsed = Param->hasUnparsedDefaultArg();
Actions.ActOnDelayedCXXMethodParameter(getCurScope(), Param);
-if (CachedT
[PATCH] D26435: Use unique_ptr for cached tokens for default arguments in C++.
dtarditi updated this revision to Diff 77373.
dtarditi added a comment.
Thanks for the code review feedback - I've addressed it. Yes, we should use
reset() instead of release(). I also deleted the unnecessary brackets. I
don't have commit access, so if this looks good, could someone commit this on
my behalf?
https://reviews.llvm.org/D26435
Files:
include/clang/Parse/Parser.h
include/clang/Sema/DeclSpec.h
lib/Parse/ParseCXXInlineMethods.cpp
lib/Parse/ParseDecl.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Sema/DeclSpec.cpp
lib/Sema/SemaDeclCXX.cpp
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -395,17 +395,15 @@
++argIdx) {
ParmVarDecl *Param = cast(chunk.Fun.Params[argIdx].Param);
if (Param->hasUnparsedDefaultArg()) {
- CachedTokens *Toks = chunk.Fun.Params[argIdx].DefaultArgTokens;
+ std::unique_ptr Toks = std::move(chunk.Fun.Params[argIdx].DefaultArgTokens);
SourceRange SR;
if (Toks->size() > 1)
SR = SourceRange((*Toks)[1].getLocation(),
Toks->back().getLocation());
else
SR = UnparsedDefaultArgLocs[Param];
Diag(Param->getLocation(), diag::err_param_default_argument_nonfunc)
<< SR;
- delete Toks;
- chunk.Fun.Params[argIdx].DefaultArgTokens = nullptr;
} else if (Param->getDefaultArg()) {
Diag(Param->getLocation(), diag::err_param_default_argument_nonfunc)
<< Param->getDefaultArg()->getSourceRange();
Index: lib/Sema/DeclSpec.cpp
===
--- lib/Sema/DeclSpec.cpp
+++ lib/Sema/DeclSpec.cpp
@@ -229,7 +229,8 @@
I.Fun.Params = new DeclaratorChunk::ParamInfo[NumParams];
I.Fun.DeleteParams = true;
}
-memcpy(I.Fun.Params, Params, sizeof(Params[0]) * NumParams);
+for (unsigned i = 0; i < NumParams; i++)
+ I.Fun.Params[i] = std::move(Params[i]);
}
// Check what exception specification information we should actually store.
Index: lib/Parse/ParseDeclCXX.cpp
===
--- lib/Parse/ParseDeclCXX.cpp
+++ lib/Parse/ParseDeclCXX.cpp
@@ -2039,7 +2039,7 @@
LateMethod->DefaultArgs.reserve(FTI.NumParams);
for (unsigned ParamIdx = 0; ParamIdx < FTI.NumParams; ++ParamIdx)
LateMethod->DefaultArgs.push_back(LateParsedDefaultArgument(
-FTI.Params[ParamIdx].Param, FTI.Params[ParamIdx].DefaultArgTokens));
+FTI.Params[ParamIdx].Param, std::move(FTI.Params[ParamIdx].DefaultArgTokens)));
}
}
Index: lib/Parse/ParseDecl.cpp
===
--- lib/Parse/ParseDecl.cpp
+++ lib/Parse/ParseDecl.cpp
@@ -6021,7 +6021,7 @@
// DefArgToks is used when the parsing of default arguments needs
// to be delayed.
-CachedTokens *DefArgToks = nullptr;
+std::unique_ptr DefArgToks;
// If no parameter was specified, verify that *something* was specified,
// otherwise we have a missing type and identifier.
@@ -6057,13 +6057,11 @@
// If we're inside a class definition, cache the tokens
// corresponding to the default argument. We'll actually parse
// them when we see the end of the class definition.
- // FIXME: Can we use a smart pointer for Toks?
- DefArgToks = new CachedTokens;
+ DefArgToks.reset(new CachedTokens);
SourceLocation ArgStartLoc = NextToken().getLocation();
if (!ConsumeAndStoreInitializer(*DefArgToks, CIK_DefaultArgument)) {
-delete DefArgToks;
-DefArgToks = nullptr;
+DefArgToks.reset();
Actions.ActOnParamDefaultArgumentError(Param, EqualLoc);
} else {
Actions.ActOnParamUnparsedDefaultArgument(Param, EqualLoc,
@@ -6099,7 +6097,7 @@
ParamInfo.push_back(DeclaratorChunk::ParamInfo(ParmII,
ParmDeclarator.getIdentifierLoc(),
- Param, DefArgToks));
+ Param, std::move(DefArgToks)));
}
if (TryConsumeToken(tok::ellipsis, EllipsisLoc)) {
Index: lib/Parse/ParseCXXInlineMethods.cpp
===
--- lib/Parse/ParseCXXInlineMethods.cpp
+++ lib/Parse/ParseCXXInlineMethods.cpp
@@ -319,7 +319,8 @@
// Introduce the parameter into scope.
bool HasUnparsed = Param->hasUnparsedDefaultArg();
Actions.ActOnDelayedCXXMethodParameter(getCurScope(), Param);
-if (CachedTokens *Toks = LM.DefaultArgs[I].Toks) {
+std::unique_ptr Toks = std::move(LM.DefaultArgs[I].Toks);
+if (Toks) {
// Mark the end of the default argument so that we know when to stop when
// we parse it later
only correct delayed typos for conditional expressions when needed.
r272587 (http://reviews.llvm.org/D20490) fixed an issue where typo correction could cause a crash when compiling C programs.The problem was that a typo expression could be inadvertently processed twice.r272587 fixed this for BinOp expressions. Conditional expressions can hit the same problem too. This change adds the two line fix for them, as well as a small test case illustrating the crash. This is my first time proposing a patch for clang. I don't have commit privileges, so if someone could review/commit this for me, I'd appreciate it. If Phrabicator is the preferred way or there's another preferred process for submitting patches, let me know. I see from the prior review that the consensus was that "typo correction is in a messy state, we should fix this". I agree. There are other problematic places in the code where double-processing might or might not occur for C code. An example is the processing of subscript expressions in Parser::ParsePostfixExpressionSuffix. Without clear invariants, it is hard to know what to do. Testing: clang fails on the test case without this change, passes with it. The clang test suite results were otherwise the same before and after this change. Thanks, David c-typo-crash.patch Description: c-typo-crash.patch ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22930: [Parser] only correct delayed typos for conditional expressions when needed.
dtarditi created this revision.
dtarditi added a reviewer: erik.pilkington.
dtarditi added a subscriber: cfe-commits.
r272587 (http://reviews.llvm.org/D20490) fixed an issue where typo correction
could cause a crash when compiling C programs.The problem was that a typo
expression could be inadvertently processed twice.r272587 fixed this for
BinOp expressions. Conditional expressions can hit the same problem too.
This change adds the two line fix for conditional expressions, as well as a
small test case illustrating the crash.
Note that I don't have commit privileges for clang; someone will need to commit
this for me. I originally started the review by email and am shifting it over
to Phrabicator. The change incorporates the following feedback from Erik
Pilkington on the original patch submitted by email:
1. If we’re doing the check at the end of both branches of the if, we might as
well just move it to after.
2. It doesn’t make sense to have a failure that only occurs in C mode in
test/SemaCXX. Maybe just append it to the end of test/Sema/typo-correction.c.
I see from the prior review that the consensus was that “typo correction is in
a messy state, we should fix this”. I agree. There are other problematic
places in the code where double-processing might or might not occur for C code.
An example is the processing of subscript expressions in
Parser::ParsePostfixExpressionSuffix. Without clear invariants, it is hard to
know what to do.
Testing: clang fails on the test case without this change, passes with it.
The clang test suite results were otherwise the same before and after this
change.
https://reviews.llvm.org/D22930
Files:
lib/Parse/ParseExpr.cpp
test/Sema/typo-correction.c
Index: test/Sema/typo-correction.c
===
--- test/Sema/typo-correction.c
+++ test/Sema/typo-correction.c
@@ -65,3 +65,18 @@
void fn_with_unknown(int a, int b) {
fn_with_unknown(unknown, unknown | unknown); // expected-error 3 {{use of
undeclared identifier}}
}
+
+// Two typos in a parenthesized expression or argument list with a conditional
+// expression caused a crash in C mode.
+//
+// r272587 fixed a similar bug for binary operations. The same fix was needed
for
+// conditional expressions.
+
+int g(int x, int y) {
+ return x + y;
+}
+
+int h() {
+ g(x, 5 ? z : 0); // expected-error 2 {{use of undeclared identifier}}
+ (x, 5 ? z : 0); // expected-error 2 {{use of undeclared identifier}}
+}
Index: lib/Parse/ParseExpr.cpp
===
--- lib/Parse/ParseExpr.cpp
+++ lib/Parse/ParseExpr.cpp
@@ -446,14 +446,15 @@
LHS = Actions.ActOnBinOp(getCurScope(), OpToken.getLocation(),
OpToken.getKind(), LHS.get(), RHS.get());
-// In this case, ActOnBinOp performed the CorrectDelayedTyposInExpr
check.
-if (!getLangOpts().CPlusPlus)
- continue;
} else {
LHS = Actions.ActOnConditionalOp(OpToken.getLocation(), ColonLoc,
LHS.get(), TernaryMiddle.get(),
RHS.get());
}
+ // In this case, ActOnBinOp or ActOnConditionalOp performed the
+ // CorrectDelayedTyposInExpr check.
+ if (!getLangOpts().CPlusPlus)
+continue;
}
// Ensure potential typos aren't left undiagnosed.
if (LHS.isInvalid()) {
Index: test/Sema/typo-correction.c
===
--- test/Sema/typo-correction.c
+++ test/Sema/typo-correction.c
@@ -65,3 +65,18 @@
void fn_with_unknown(int a, int b) {
fn_with_unknown(unknown, unknown | unknown); // expected-error 3 {{use of undeclared identifier}}
}
+
+// Two typos in a parenthesized expression or argument list with a conditional
+// expression caused a crash in C mode.
+//
+// r272587 fixed a similar bug for binary operations. The same fix was needed for
+// conditional expressions.
+
+int g(int x, int y) {
+ return x + y;
+}
+
+int h() {
+ g(x, 5 ? z : 0); // expected-error 2 {{use of undeclared identifier}}
+ (x, 5 ? z : 0); // expected-error 2 {{use of undeclared identifier}}
+}
Index: lib/Parse/ParseExpr.cpp
===
--- lib/Parse/ParseExpr.cpp
+++ lib/Parse/ParseExpr.cpp
@@ -446,14 +446,15 @@
LHS = Actions.ActOnBinOp(getCurScope(), OpToken.getLocation(),
OpToken.getKind(), LHS.get(), RHS.get());
-// In this case, ActOnBinOp performed the CorrectDelayedTyposInExpr check.
-if (!getLangOpts().CPlusPlus)
- continue;
} else {
LHS = Actions.ActOnConditionalOp(OpToken.getLocation(), ColonLoc,
LHS.get(), TernaryMiddle.get(),
RHS.get());
}
+ // In this cas
[clang] [analyzer] Update the undefined assignment checker diagnostics to not use the term 'garbage' (PR #126596)
https://github.com/dtarditi created
https://github.com/llvm/llvm-project/pull/126596
A clang user pointed out that messages for the static analyzer undefined
assignment checker use the term 'garbage'. This is kind of snarky and also
imprecise. This change replaces the term 'garbage' in those messages with 'not
meaningful'. It moves the term 'undefined' to be first in the messages because
of the possible ambiguous parsing of the term 'not
meaningful and undefined'. That could be parsed as '(not meaningful) and
undefined' or 'not (meaningful and undefined').
The use of the term 'meaningless' was considered, but not chosen because it has
two meanings in English. One meaning is 'without meaning'. The other meaning is
'having no point'. The 2nd meaning could be construed
as indicating the computation could be deleted.
rdar://133418644
>From 06eb6682196249f4cae9801963380d0881d27296 Mon Sep 17 00:00:00 2001
From: David Tarditi
Date: Mon, 10 Feb 2025 11:35:45 -0800
Subject: [PATCH 1/2] [analyzer] Update undefined assignment diagnostics to not
use 'garbage'
A clang user pointed out that messages for the static analyzer undefined
assignment checker use the term 'garbage'. This is kind of snarky and
also imprecise. This change replaces the term 'garbage' in those messages
with 'not meaningful'. It moves the term 'undefined' to be first in the
messages because of the possible ambiguous parsing of the term 'not
meaningful and undefined'. That could be parsed as '(not meaningful)
and undefined' or 'not (meaningful and undefined').
The use of the term 'meaningless' was considered, but not chosen because
it has two meanings in English. One meaning is 'without meaning'. The
other meaning is 'having no point'. The 2nd meaning could be construed
as indicating the computation could be deleted.
rdar://133418644
---
.../Checkers/UndefinedAssignmentChecker.cpp | 13
.../Inputs/expected-plists/edges-new.mm.plist | 10 +++
.../expected-plists/plist-output.m.plist | 10 +++
clang/test/Analysis/a_flaky_crash.cpp | 2 +-
.../analysis-after-multiple-dtors.cpp | 2 +-
clang/test/Analysis/array-init-loop.cpp | 6 ++--
clang/test/Analysis/array-punned-region.c | 2 +-
clang/test/Analysis/builtin_overflow_notes.c | 4 +--
clang/test/Analysis/call-invalidation.cpp | 4 +--
clang/test/Analysis/ctor-array.cpp| 22 +++---
.../diagnostics/no-store-func-path-notes.m| 4 +--
clang/test/Analysis/fread.c | 20 ++---
.../Analysis/implicit-ctor-undef-value.cpp| 12
clang/test/Analysis/initialization.c | 16 +-
clang/test/Analysis/initialization.cpp| 26
clang/test/Analysis/misc-ps.c | 4 +--
clang/test/Analysis/operator-calls.cpp| 8 ++---
clang/test/Analysis/stack-addr-ps.cpp | 2 +-
clang/test/Analysis/uninit-const.c| 20 ++---
clang/test/Analysis/uninit-const.cpp | 4 +--
.../uninit-structured-binding-array.cpp | 30 +--
.../uninit-structured-binding-struct.cpp | 12
.../uninit-structured-binding-tuple.cpp | 4 +--
clang/test/Analysis/uninit-vals.m | 8 ++---
.../test/Analysis/zero-size-non-pod-array.cpp | 4 +--
25 files changed, 125 insertions(+), 124 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
index ddc6cc9e8202c7c..f13de315ed7b5e8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
@@ -23,7 +23,7 @@ using namespace ento;
namespace {
class UndefinedAssignmentChecker
: public Checker {
- const BugType BT{this, "Assigned value is garbage or undefined"};
+ const BugType BT{this, "Assigned value is undefined and not meaningful"};
public:
void checkBind(SVal location, SVal val, const Stmt *S,
@@ -57,8 +57,8 @@ void UndefinedAssignmentChecker::checkBind(SVal location,
SVal val,
while (StoreE) {
if (const UnaryOperator *U = dyn_cast(StoreE)) {
- OS << "The expression is an uninitialized value. "
-"The computed value will also be garbage";
+ OS << "The expression is an uninitialized value, so the computed value "
+ << "is not meaningful";
ex = U->getSubExpr();
break;
@@ -68,7 +68,7 @@ void UndefinedAssignmentChecker::checkBind(SVal location,
SVal val,
if (B->isCompoundAssignmentOp()) {
if (C.getSVal(B->getLHS()).isUndef()) {
OS << "The left expression of the compound assignment is an "
-"uninitialized value. The computed value will also be garbage";
+ << "uninitialized value, so the computed value is not meaningful";
ex = B->getLHS();
break;
}
@@ -88,8 +88,9 @@ void UndefinedAssign
[clang] [analyzer] Update the undefined assignment checker diagnostics to not use the term 'garbage' (PR #126596)
dtarditi wrote: @Xazax-hun and @haoNoQ. Could one of you review this or assign it to someone to review? Thanks. https://github.com/llvm/llvm-project/pull/126596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Update the undefined assignment checker diagnostics to not use the term 'garbage' (PR #126596)
dtarditi wrote: @haoNoQ Thank you for the welcome and the helpful information! Your suggestion sounds good to me. I had actually started down this route initially of pointing out the logical error of using a variable before it is initialized. I changed direction because I was not sure where the undefined values were coming from and how they propagate. I prefer to focus on the logical error that a programmer should take action to fix, which in this case is using a variable or data before it is initialized. I've found that programmers are often unfamiliar with standards terms like 'undefined behavior'. https://github.com/llvm/llvm-project/pull/126596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Update the undefined assignment checker diagnostics to not use the term 'garbage' (PR #126596)
dtarditi wrote: @steakhal Yes, the term garbage has a negative connotation when used as an adjective in English. By a little snarky, the user meant that the message could be seen as a little overly critical. The user suggested using more neutral terminology. https://github.com/llvm/llvm-project/pull/126596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Update the undefined assignment checker diagnostics to not use the term 'garbage' (PR #126596)
dtarditi wrote: @haoNoQ thanks for the explanation! Yes, this makes sense. I agree that the right long-term approach is that the ArrayBoundChecker issues warnings about out-of-bounds accesses. In general, I think it is desirable to report a problem about undefined behavior as close to its cause as possible, using a logical explanation where possible. https://github.com/llvm/llvm-project/pull/126596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Update the undefined assignment checker diagnostics to not use the term 'garbage' (PR #126596)
dtarditi wrote: @haoNoQ I believe the static analyzer can produce undefined values from out-of-bounds memory accesses also. There are some test cases in this change that show that. Some possible error message that I think cover both cases are: - `use of uninitialized memory or out-of-bounds memory` - 'use of uninitialized or out-of-bounds memory` https://github.com/llvm/llvm-project/pull/126596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Wunsafe-buffer-usage] Turn off unsafe-buffer warning for methods annotated with clang::unsafe_buffer_usage attribute (PR #125671)
dtarditi wrote: Also, please avoid force pushes if you can. It confuses the PR history. https://github.com/llvm/llvm-project/pull/125671 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Update the undefined assignment checker diagnostics to not use the term 'garbage' (PR #126596)
https://github.com/dtarditi updated
https://github.com/llvm/llvm-project/pull/126596
>From 06eb6682196249f4cae9801963380d0881d27296 Mon Sep 17 00:00:00 2001
From: David Tarditi
Date: Mon, 10 Feb 2025 11:35:45 -0800
Subject: [PATCH 1/3] [analyzer] Update undefined assignment diagnostics to not
use 'garbage'
A clang user pointed out that messages for the static analyzer undefined
assignment checker use the term 'garbage'. This is kind of snarky and
also imprecise. This change replaces the term 'garbage' in those messages
with 'not meaningful'. It moves the term 'undefined' to be first in the
messages because of the possible ambiguous parsing of the term 'not
meaningful and undefined'. That could be parsed as '(not meaningful)
and undefined' or 'not (meaningful and undefined').
The use of the term 'meaningless' was considered, but not chosen because
it has two meanings in English. One meaning is 'without meaning'. The
other meaning is 'having no point'. The 2nd meaning could be construed
as indicating the computation could be deleted.
rdar://133418644
---
.../Checkers/UndefinedAssignmentChecker.cpp | 13
.../Inputs/expected-plists/edges-new.mm.plist | 10 +++
.../expected-plists/plist-output.m.plist | 10 +++
clang/test/Analysis/a_flaky_crash.cpp | 2 +-
.../analysis-after-multiple-dtors.cpp | 2 +-
clang/test/Analysis/array-init-loop.cpp | 6 ++--
clang/test/Analysis/array-punned-region.c | 2 +-
clang/test/Analysis/builtin_overflow_notes.c | 4 +--
clang/test/Analysis/call-invalidation.cpp | 4 +--
clang/test/Analysis/ctor-array.cpp| 22 +++---
.../diagnostics/no-store-func-path-notes.m| 4 +--
clang/test/Analysis/fread.c | 20 ++---
.../Analysis/implicit-ctor-undef-value.cpp| 12
clang/test/Analysis/initialization.c | 16 +-
clang/test/Analysis/initialization.cpp| 26
clang/test/Analysis/misc-ps.c | 4 +--
clang/test/Analysis/operator-calls.cpp| 8 ++---
clang/test/Analysis/stack-addr-ps.cpp | 2 +-
clang/test/Analysis/uninit-const.c| 20 ++---
clang/test/Analysis/uninit-const.cpp | 4 +--
.../uninit-structured-binding-array.cpp | 30 +--
.../uninit-structured-binding-struct.cpp | 12
.../uninit-structured-binding-tuple.cpp | 4 +--
clang/test/Analysis/uninit-vals.m | 8 ++---
.../test/Analysis/zero-size-non-pod-array.cpp | 4 +--
25 files changed, 125 insertions(+), 124 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
index ddc6cc9e8202c..f13de315ed7b5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
@@ -23,7 +23,7 @@ using namespace ento;
namespace {
class UndefinedAssignmentChecker
: public Checker {
- const BugType BT{this, "Assigned value is garbage or undefined"};
+ const BugType BT{this, "Assigned value is undefined and not meaningful"};
public:
void checkBind(SVal location, SVal val, const Stmt *S,
@@ -57,8 +57,8 @@ void UndefinedAssignmentChecker::checkBind(SVal location,
SVal val,
while (StoreE) {
if (const UnaryOperator *U = dyn_cast(StoreE)) {
- OS << "The expression is an uninitialized value. "
-"The computed value will also be garbage";
+ OS << "The expression is an uninitialized value, so the computed value "
+ << "is not meaningful";
ex = U->getSubExpr();
break;
@@ -68,7 +68,7 @@ void UndefinedAssignmentChecker::checkBind(SVal location,
SVal val,
if (B->isCompoundAssignmentOp()) {
if (C.getSVal(B->getLHS()).isUndef()) {
OS << "The left expression of the compound assignment is an "
-"uninitialized value. The computed value will also be garbage";
+ << "uninitialized value, so the computed value is not meaningful";
ex = B->getLHS();
break;
}
@@ -88,8 +88,9 @@ void UndefinedAssignmentChecker::checkBind(SVal location,
SVal val,
if (CD->isImplicit()) {
for (auto *I : CD->inits()) {
if (I->getInit()->IgnoreImpCasts() == StoreE) {
-OS << "Value assigned to field '" << I->getMember()->getName()
- << "' in implicit constructor is garbage or undefined";
+OS << "Value assigned to field '"
+ << I->getMember()->getName()
+ << "' in implicit constructor is undefined and not meaningful.";
break;
}
}
diff --git a/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
b/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
index 31b6286b4465e..dd731e705c9b0 100644
--- a/clang/test/Analysis/Inputs/expected-plists/
[clang] [analyzer] Update the undefined assignment checker diagnostics to not use the term 'garbage' (PR #126596)
dtarditi wrote: @haoNoQ I think sticking with uninitialized is good. I've updated the patch with new error messages using that. Please take a look at it and let me know what you think. I think the right path is to issue a more precise error message for out-of-bounds reads. As you point, we could do that by including the bounds checker as part of core checkers. If that checker is too noisy for that, we could create a more limited version that covers the cases that are making it to this checker. https://github.com/llvm/llvm-project/pull/126596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Update the undefined assignment checker diagnostics to not use the term 'garbage' (PR #126596)
https://github.com/dtarditi updated
https://github.com/llvm/llvm-project/pull/126596
>From 06eb6682196249f4cae9801963380d0881d27296 Mon Sep 17 00:00:00 2001
From: David Tarditi
Date: Mon, 10 Feb 2025 11:35:45 -0800
Subject: [PATCH 1/3] [analyzer] Update undefined assignment diagnostics to not
use 'garbage'
A clang user pointed out that messages for the static analyzer undefined
assignment checker use the term 'garbage'. This is kind of snarky and
also imprecise. This change replaces the term 'garbage' in those messages
with 'not meaningful'. It moves the term 'undefined' to be first in the
messages because of the possible ambiguous parsing of the term 'not
meaningful and undefined'. That could be parsed as '(not meaningful)
and undefined' or 'not (meaningful and undefined').
The use of the term 'meaningless' was considered, but not chosen because
it has two meanings in English. One meaning is 'without meaning'. The
other meaning is 'having no point'. The 2nd meaning could be construed
as indicating the computation could be deleted.
rdar://133418644
---
.../Checkers/UndefinedAssignmentChecker.cpp | 13
.../Inputs/expected-plists/edges-new.mm.plist | 10 +++
.../expected-plists/plist-output.m.plist | 10 +++
clang/test/Analysis/a_flaky_crash.cpp | 2 +-
.../analysis-after-multiple-dtors.cpp | 2 +-
clang/test/Analysis/array-init-loop.cpp | 6 ++--
clang/test/Analysis/array-punned-region.c | 2 +-
clang/test/Analysis/builtin_overflow_notes.c | 4 +--
clang/test/Analysis/call-invalidation.cpp | 4 +--
clang/test/Analysis/ctor-array.cpp| 22 +++---
.../diagnostics/no-store-func-path-notes.m| 4 +--
clang/test/Analysis/fread.c | 20 ++---
.../Analysis/implicit-ctor-undef-value.cpp| 12
clang/test/Analysis/initialization.c | 16 +-
clang/test/Analysis/initialization.cpp| 26
clang/test/Analysis/misc-ps.c | 4 +--
clang/test/Analysis/operator-calls.cpp| 8 ++---
clang/test/Analysis/stack-addr-ps.cpp | 2 +-
clang/test/Analysis/uninit-const.c| 20 ++---
clang/test/Analysis/uninit-const.cpp | 4 +--
.../uninit-structured-binding-array.cpp | 30 +--
.../uninit-structured-binding-struct.cpp | 12
.../uninit-structured-binding-tuple.cpp | 4 +--
clang/test/Analysis/uninit-vals.m | 8 ++---
.../test/Analysis/zero-size-non-pod-array.cpp | 4 +--
25 files changed, 125 insertions(+), 124 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
index ddc6cc9e8202c..f13de315ed7b5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
@@ -23,7 +23,7 @@ using namespace ento;
namespace {
class UndefinedAssignmentChecker
: public Checker {
- const BugType BT{this, "Assigned value is garbage or undefined"};
+ const BugType BT{this, "Assigned value is undefined and not meaningful"};
public:
void checkBind(SVal location, SVal val, const Stmt *S,
@@ -57,8 +57,8 @@ void UndefinedAssignmentChecker::checkBind(SVal location,
SVal val,
while (StoreE) {
if (const UnaryOperator *U = dyn_cast(StoreE)) {
- OS << "The expression is an uninitialized value. "
-"The computed value will also be garbage";
+ OS << "The expression is an uninitialized value, so the computed value "
+ << "is not meaningful";
ex = U->getSubExpr();
break;
@@ -68,7 +68,7 @@ void UndefinedAssignmentChecker::checkBind(SVal location,
SVal val,
if (B->isCompoundAssignmentOp()) {
if (C.getSVal(B->getLHS()).isUndef()) {
OS << "The left expression of the compound assignment is an "
-"uninitialized value. The computed value will also be garbage";
+ << "uninitialized value, so the computed value is not meaningful";
ex = B->getLHS();
break;
}
@@ -88,8 +88,9 @@ void UndefinedAssignmentChecker::checkBind(SVal location,
SVal val,
if (CD->isImplicit()) {
for (auto *I : CD->inits()) {
if (I->getInit()->IgnoreImpCasts() == StoreE) {
-OS << "Value assigned to field '" << I->getMember()->getName()
- << "' in implicit constructor is garbage or undefined";
+OS << "Value assigned to field '"
+ << I->getMember()->getName()
+ << "' in implicit constructor is undefined and not meaningful.";
break;
}
}
diff --git a/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
b/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
index 31b6286b4465e..dd731e705c9b0 100644
--- a/clang/test/Analysis/Inputs/expected-plists/
[clang] [analyzer] Update the undefined assignment checker diagnostics to not use the term 'garbage' (PR #126596)
https://github.com/dtarditi edited https://github.com/llvm/llvm-project/pull/126596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Update the undefined assignment checker diagnostics to not use the term 'garbage' (PR #126596)
dtarditi wrote: Thanks - I updated the PR title and description so that it can be used for the commit. It could be a little confusing to read the PR thread with the updated description. Here is the original description, in case anyone reading the thread needs the context. > A clang user pointed out that messages for the static analyzer undefined > assignment checker use the term 'garbage'. This is kind of snarky and also > imprecise. This change replaces the term 'garbage' in those messages with > 'not meaningful'. It moves the term 'undefined' to be first in the messages > because of the possible ambiguous parsing of the term 'not meaningful and > undefined'. That could be parsed as '(not meaningful) and undefined' or 'not > (meaningful and undefined'). > The use of the term 'meaningless' was considered, but not chosen because it > has two meanings in English. One meaning is 'without meaning'. The other > meaning is 'having no point'. The 2nd meaning could be construed as > indicating the computation could be deleted. > rdar://133418644 https://github.com/llvm/llvm-project/pull/126596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Update the undefined assignment checker diagnostics to not use the term 'garbage' (PR #126596)
dtarditi wrote: Thanks everyone for the reviews and feedback. I don't have write access for LLVM. @NagyDonat or @haoNoQ, could you merge it on my behalf? Do you want me to condense the change to a single commit as recommended [here](https://llvm.org/docs/GitHub.html#landing-your-change)? Alternately, I could post a draft commit message in a comment for use with GitHub's UI for a squash-merge. https://github.com/llvm/llvm-project/pull/126596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Wunsafe-buffer-usage] Turn off unsafe-buffer warning for methods annotated with clang::unsafe_buffer_usage attribute (PR #125671)
https://github.com/dtarditi approved this pull request. Looks good - thank you for adding the tests. https://github.com/llvm/llvm-project/pull/125671 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Fix a potential overflow bug reported by #126334 (PR #129169)
https://github.com/dtarditi approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/129169 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Add alloc_size knowledge to the 2-param span constructor warning (PR #114894)
dtarditi wrote: I agree that it is fine to not handle this for now. If we do decide to handle this, we need to be careful because this equivalence does not hold for signed integer arithmetic expressions in C. The equivalence relies on commutativity and associativity of integer arithmetic. Reassociation can change whether overflow occurs for integer expressions at runtime and signed integer overflow is undefined behavior in C. If, however, we assume that signed integer arithmetic is 2's complement arithmetic (which can be specified via a compiler flag), then this equivalence holds for signed integer expressions. https://github.com/llvm/llvm-project/pull/114894 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Add alloc_size knowledge to the 2-param span constructor warning (PR #114894)
https://github.com/dtarditi edited https://github.com/llvm/llvm-project/pull/114894 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Add alloc_size knowledge to the 2-param span constructor warning (PR #114894)
@@ -353,23 +355,90 @@ isInUnspecifiedUntypedContext(internal::Matcher
InnerMatcher) {
return stmt(anyOf(CompStmt, IfStmtThen, IfStmtElse));
}
+// Returns true iff integer E1 is equivalent to integer E2.
+//
+// For now we only support such expressions:
+//expr := DRE | const-value | expr BO expr
+//BO := '*' | '+'
+//
+// FIXME: We can reuse the expression comparator of the interop analysis after
dtarditi wrote:
@ziqingluo-90 Is the expression comparator of the interop analysis the same or
different from what is done here?
I believe this expression comparison can result in exponential work in the
size of an expression.. AreEquaIntegralBinaryOperator makes 2 calls to
AreEqualIntegers per subexpression. AreEqualIntegers can call back into
EqualIntegeralBinaryOperator on subexpressions of its argument.
If the plan is to eventually replace this code with the expression comparator
from interop analysis, then this is not worth fixing now. However, if the
expression comparator has the same issue or that is not the plan, then this
should be fixed. It can be fixed be bounding the amount of work that is
allowed, for example, by limiting the recursive depth that is allowed.
https://github.com/llvm/llvm-project/pull/114894
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Add alloc_size knowledge to the 2-param span constructor warning (PR #114894)
https://github.com/dtarditi commented: @ziqingluo-90 @malavikasamak thanks for adding me to this review. I left one informational comment and one piece of feedback. https://github.com/llvm/llvm-project/pull/114894 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
