[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
@@ -406,6 +406,39 @@ AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct)
{
}
return false;
}
+
+AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
+ // FIXME: Proper solution:
+ // - refactor Sema::CheckArrayAccess
+ //- split safe/OOB/unknown decision logic from diagnostics emitting code
+ //- e. g. "Try harder to find a NamedDecl to point at in the note."
+ //already duplicated
+ // - call both from Sema and from here
+
+ const DeclRefExpr *BaseDRE =
+ dyn_cast_or_null(Node.getBase()->IgnoreParenImpCasts());
haoNoQ wrote:
I don't think `IgnoreParenImpCasts()` can ever return null.
```suggestion
const auto *BaseDRE =
dyn_cast(Node.getBase()->IgnoreParenImpCasts());
```
https://github.com/llvm/llvm-project/pull/80504
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
https://github.com/jkorous-apple closed https://github.com/llvm/llvm-project/pull/80504 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
https://github.com/jkorous-apple updated
https://github.com/llvm/llvm-project/pull/80504
>From e075dc3ac10c0cd2e12b223988ec4821b40b55d2 Mon Sep 17 00:00:00 2001
From: Jan Korous
Date: Fri, 2 Feb 2024 14:46:59 -0800
Subject: [PATCH 1/7] [-Wunsafe-buffer-usage] Ignore safe array subscripts
Don't emit warnings for array subscripts on constant size arrays where the
index is constant and within bounds.
Example:
int arr[10];
arr[5] = 0; //safe, no warning
This patch recognizes only array indices that are integer literals - it doesn't
understand more complex expressions (arithmetic on constants, etc.).
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 39 +++
.../warn-unsafe-buffer-usage-array.cpp| 18 -
2 files changed, 49 insertions(+), 8 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index ca346444e047e5..d150a679597a92 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -406,6 +406,31 @@ AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct)
{
}
return false;
}
+
+AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
+ const DeclRefExpr * BaseDRE =
dyn_cast_or_null(Node.getBase()->IgnoreParenImpCasts());
+ if (!BaseDRE)
+return false;
+ if (!BaseDRE->getDecl())
+return false;
+ auto BaseVarDeclTy = BaseDRE->getDecl()->getType();
+ if (!BaseVarDeclTy->isConstantArrayType())
+return false;
+ const auto * CATy = dyn_cast_or_null(BaseVarDeclTy);
+ if (!CATy)
+return false;
+ const APInt ArrSize = CATy->getSize();
+
+ if (const auto * IdxLit = dyn_cast(Node.getIdx())) {
+const APInt ArrIdx = IdxLit->getValue();
+// FIXME: ArrIdx.isNegative() we could immediately emit an error as that's
a bug
+if (ArrIdx.isNonNegative() && ArrIdx.getLimitedValue() <
ArrSize.getLimitedValue())
+ return true;
+ }
+
+ return false;
+}
+
} // namespace clang::ast_matchers
namespace {
@@ -598,16 +623,16 @@ class ArraySubscriptGadget : public WarningGadget {
}
static Matcher matcher() {
-// FIXME: What if the index is integer literal 0? Should this be
-// a safe gadget in this case?
- // clang-format off
+// clang-format off
return stmt(arraySubscriptExpr(
hasBase(ignoringParenImpCasts(
anyOf(hasPointerType(), hasArrayType(,
-unless(hasIndex(
-anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
- )))
-.bind(ArraySubscrTag));
+unless(anyOf(
+ isSafeArraySubscript(),
+ hasIndex(
+ anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
+ )
+))).bind(ArraySubscrTag));
// clang-format on
}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
index 90c11b1be95c25..4804223e8be058 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN: %clang_cc1 -std=c++20 -Wno-everything -Wunsafe-buffer-usage \
// RUN:-fsafe-buffer-usage-suggestions \
// RUN:-verify %s
@@ -22,3 +22,19 @@ struct Foo {
void foo2(Foo& f, unsigned idx) {
f.member_buffer[idx] = 0; // expected-warning{{unsafe buffer access}}
}
+
+void constant_idx_safe(unsigned idx) {
+ int buffer[10];
+ buffer[9] = 0;
+}
+
+void constant_idx_safe0(unsigned idx) {
+ int buffer[10];
+ buffer[0] = 0;
+}
+
+void constant_idx_unsafe(unsigned idx) {
+ int buffer[10]; // expected-warning{{'buffer' is an unsafe buffer that
does not perform bounds checks}}
+// expected-note@-1{{change type of 'buffer' to
'std::array' to harden it}}
+ buffer[10] = 0; // expected-note{{used in buffer access here}}
+}
>From 9f3940e5e1a8555a784521999d296ed46b1ae0c2 Mon Sep 17 00:00:00 2001
From: Jan Korous
Date: Fri, 2 Feb 2024 14:48:41 -0800
Subject: [PATCH 2/7] [-Wunsafe-buffer-usage][NFC] Update existing tests after
constant safe index is ignored
---
...afe-buffer-usage-fixits-pointer-access.cpp | 8 +--
...ge-fixits-pointer-arg-to-func-ptr-call.cpp | 3 +-
.../test/SemaCXX/warn-unsafe-buffer-usage.cpp | 53 ++-
3 files changed, 33 insertions(+), 31 deletions(-)
diff --git
a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
index f94072015ff87d..b3c64f1b0d085e 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
@@ -83,11 +83,11 @@ void unsafe_method_invocation_single_param() {
}
-void unsafe_method_invocation_single_param_array() {
+void
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
https://github.com/haoNoQ approved this pull request. LGTM! https://github.com/llvm/llvm-project/pull/80504 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
@@ -598,16 +623,16 @@ class ArraySubscriptGadget : public WarningGadget {
}
static Matcher matcher() {
-// FIXME: What if the index is integer literal 0? Should this be
-// a safe gadget in this case?
- // clang-format off
+// clang-format off
return stmt(arraySubscriptExpr(
hasBase(ignoringParenImpCasts(
anyOf(hasPointerType(), hasArrayType(,
-unless(hasIndex(
-anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
- )))
-.bind(ArraySubscrTag));
+unless(anyOf(
+ isSafeArraySubscript(),
+ hasIndex(
+ anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
haoNoQ wrote:
So you want to suppress the warning here right? In this case yeah makes sense.
It's also somewhat covered by
```
warning: zero size arrays are an extension [-Wzero-length-array]
```
https://github.com/llvm/llvm-project/pull/80504
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
@@ -598,16 +623,16 @@ class ArraySubscriptGadget : public WarningGadget {
}
static Matcher matcher() {
-// FIXME: What if the index is integer literal 0? Should this be
-// a safe gadget in this case?
- // clang-format off
+// clang-format off
return stmt(arraySubscriptExpr(
hasBase(ignoringParenImpCasts(
anyOf(hasPointerType(), hasArrayType(,
-unless(hasIndex(
-anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
- )))
-.bind(ArraySubscrTag));
+unless(anyOf(
+ isSafeArraySubscript(),
+ hasIndex(
+ anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
jkorous-apple wrote:
Actually, it is still necessary - we need to somehow cover:
```
int arr[0];
arr[0] = 5;
```
And that has been defined as out of scope for the warning. I don't feel like
calling it "safe" and I'd rather have it visible in the top-level matcher.
https://github.com/llvm/llvm-project/pull/80504
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
https://github.com/jkorous-apple updated
https://github.com/llvm/llvm-project/pull/80504
>From e075dc3ac10c0cd2e12b223988ec4821b40b55d2 Mon Sep 17 00:00:00 2001
From: Jan Korous
Date: Fri, 2 Feb 2024 14:46:59 -0800
Subject: [PATCH 1/6] [-Wunsafe-buffer-usage] Ignore safe array subscripts
Don't emit warnings for array subscripts on constant size arrays where the
index is constant and within bounds.
Example:
int arr[10];
arr[5] = 0; //safe, no warning
This patch recognizes only array indices that are integer literals - it doesn't
understand more complex expressions (arithmetic on constants, etc.).
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 39 +++
.../warn-unsafe-buffer-usage-array.cpp| 18 -
2 files changed, 49 insertions(+), 8 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index ca346444e047e5..d150a679597a92 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -406,6 +406,31 @@ AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct)
{
}
return false;
}
+
+AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
+ const DeclRefExpr * BaseDRE =
dyn_cast_or_null(Node.getBase()->IgnoreParenImpCasts());
+ if (!BaseDRE)
+return false;
+ if (!BaseDRE->getDecl())
+return false;
+ auto BaseVarDeclTy = BaseDRE->getDecl()->getType();
+ if (!BaseVarDeclTy->isConstantArrayType())
+return false;
+ const auto * CATy = dyn_cast_or_null(BaseVarDeclTy);
+ if (!CATy)
+return false;
+ const APInt ArrSize = CATy->getSize();
+
+ if (const auto * IdxLit = dyn_cast(Node.getIdx())) {
+const APInt ArrIdx = IdxLit->getValue();
+// FIXME: ArrIdx.isNegative() we could immediately emit an error as that's
a bug
+if (ArrIdx.isNonNegative() && ArrIdx.getLimitedValue() <
ArrSize.getLimitedValue())
+ return true;
+ }
+
+ return false;
+}
+
} // namespace clang::ast_matchers
namespace {
@@ -598,16 +623,16 @@ class ArraySubscriptGadget : public WarningGadget {
}
static Matcher matcher() {
-// FIXME: What if the index is integer literal 0? Should this be
-// a safe gadget in this case?
- // clang-format off
+// clang-format off
return stmt(arraySubscriptExpr(
hasBase(ignoringParenImpCasts(
anyOf(hasPointerType(), hasArrayType(,
-unless(hasIndex(
-anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
- )))
-.bind(ArraySubscrTag));
+unless(anyOf(
+ isSafeArraySubscript(),
+ hasIndex(
+ anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
+ )
+))).bind(ArraySubscrTag));
// clang-format on
}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
index 90c11b1be95c25..4804223e8be058 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN: %clang_cc1 -std=c++20 -Wno-everything -Wunsafe-buffer-usage \
// RUN:-fsafe-buffer-usage-suggestions \
// RUN:-verify %s
@@ -22,3 +22,19 @@ struct Foo {
void foo2(Foo& f, unsigned idx) {
f.member_buffer[idx] = 0; // expected-warning{{unsafe buffer access}}
}
+
+void constant_idx_safe(unsigned idx) {
+ int buffer[10];
+ buffer[9] = 0;
+}
+
+void constant_idx_safe0(unsigned idx) {
+ int buffer[10];
+ buffer[0] = 0;
+}
+
+void constant_idx_unsafe(unsigned idx) {
+ int buffer[10]; // expected-warning{{'buffer' is an unsafe buffer that
does not perform bounds checks}}
+// expected-note@-1{{change type of 'buffer' to
'std::array' to harden it}}
+ buffer[10] = 0; // expected-note{{used in buffer access here}}
+}
>From 9f3940e5e1a8555a784521999d296ed46b1ae0c2 Mon Sep 17 00:00:00 2001
From: Jan Korous
Date: Fri, 2 Feb 2024 14:48:41 -0800
Subject: [PATCH 2/6] [-Wunsafe-buffer-usage][NFC] Update existing tests after
constant safe index is ignored
---
...afe-buffer-usage-fixits-pointer-access.cpp | 8 +--
...ge-fixits-pointer-arg-to-func-ptr-call.cpp | 3 +-
.../test/SemaCXX/warn-unsafe-buffer-usage.cpp | 53 ++-
3 files changed, 33 insertions(+), 31 deletions(-)
diff --git
a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
index f94072015ff87d..b3c64f1b0d085e 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
@@ -83,11 +83,11 @@ void unsafe_method_invocation_single_param() {
}
-void unsafe_method_invocation_single_param_array() {
+void
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
https://github.com/jkorous-apple updated
https://github.com/llvm/llvm-project/pull/80504
>From e075dc3ac10c0cd2e12b223988ec4821b40b55d2 Mon Sep 17 00:00:00 2001
From: Jan Korous
Date: Fri, 2 Feb 2024 14:46:59 -0800
Subject: [PATCH 1/4] [-Wunsafe-buffer-usage] Ignore safe array subscripts
Don't emit warnings for array subscripts on constant size arrays where the
index is constant and within bounds.
Example:
int arr[10];
arr[5] = 0; //safe, no warning
This patch recognizes only array indices that are integer literals - it doesn't
understand more complex expressions (arithmetic on constants, etc.).
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 39 +++
.../warn-unsafe-buffer-usage-array.cpp| 18 -
2 files changed, 49 insertions(+), 8 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index ca346444e047e5..d150a679597a92 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -406,6 +406,31 @@ AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct)
{
}
return false;
}
+
+AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
+ const DeclRefExpr * BaseDRE =
dyn_cast_or_null(Node.getBase()->IgnoreParenImpCasts());
+ if (!BaseDRE)
+return false;
+ if (!BaseDRE->getDecl())
+return false;
+ auto BaseVarDeclTy = BaseDRE->getDecl()->getType();
+ if (!BaseVarDeclTy->isConstantArrayType())
+return false;
+ const auto * CATy = dyn_cast_or_null(BaseVarDeclTy);
+ if (!CATy)
+return false;
+ const APInt ArrSize = CATy->getSize();
+
+ if (const auto * IdxLit = dyn_cast(Node.getIdx())) {
+const APInt ArrIdx = IdxLit->getValue();
+// FIXME: ArrIdx.isNegative() we could immediately emit an error as that's
a bug
+if (ArrIdx.isNonNegative() && ArrIdx.getLimitedValue() <
ArrSize.getLimitedValue())
+ return true;
+ }
+
+ return false;
+}
+
} // namespace clang::ast_matchers
namespace {
@@ -598,16 +623,16 @@ class ArraySubscriptGadget : public WarningGadget {
}
static Matcher matcher() {
-// FIXME: What if the index is integer literal 0? Should this be
-// a safe gadget in this case?
- // clang-format off
+// clang-format off
return stmt(arraySubscriptExpr(
hasBase(ignoringParenImpCasts(
anyOf(hasPointerType(), hasArrayType(,
-unless(hasIndex(
-anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
- )))
-.bind(ArraySubscrTag));
+unless(anyOf(
+ isSafeArraySubscript(),
+ hasIndex(
+ anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
+ )
+))).bind(ArraySubscrTag));
// clang-format on
}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
index 90c11b1be95c25..4804223e8be058 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN: %clang_cc1 -std=c++20 -Wno-everything -Wunsafe-buffer-usage \
// RUN:-fsafe-buffer-usage-suggestions \
// RUN:-verify %s
@@ -22,3 +22,19 @@ struct Foo {
void foo2(Foo& f, unsigned idx) {
f.member_buffer[idx] = 0; // expected-warning{{unsafe buffer access}}
}
+
+void constant_idx_safe(unsigned idx) {
+ int buffer[10];
+ buffer[9] = 0;
+}
+
+void constant_idx_safe0(unsigned idx) {
+ int buffer[10];
+ buffer[0] = 0;
+}
+
+void constant_idx_unsafe(unsigned idx) {
+ int buffer[10]; // expected-warning{{'buffer' is an unsafe buffer that
does not perform bounds checks}}
+// expected-note@-1{{change type of 'buffer' to
'std::array' to harden it}}
+ buffer[10] = 0; // expected-note{{used in buffer access here}}
+}
>From 9f3940e5e1a8555a784521999d296ed46b1ae0c2 Mon Sep 17 00:00:00 2001
From: Jan Korous
Date: Fri, 2 Feb 2024 14:48:41 -0800
Subject: [PATCH 2/4] [-Wunsafe-buffer-usage][NFC] Update existing tests after
constant safe index is ignored
---
...afe-buffer-usage-fixits-pointer-access.cpp | 8 +--
...ge-fixits-pointer-arg-to-func-ptr-call.cpp | 3 +-
.../test/SemaCXX/warn-unsafe-buffer-usage.cpp | 53 ++-
3 files changed, 33 insertions(+), 31 deletions(-)
diff --git
a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
index f94072015ff87d..b3c64f1b0d085e 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
@@ -83,11 +83,11 @@ void unsafe_method_invocation_single_param() {
}
-void unsafe_method_invocation_single_param_array() {
+void
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
@@ -406,6 +406,31 @@ AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct)
{
}
return false;
}
+
+AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
+ const DeclRefExpr * BaseDRE =
dyn_cast_or_null(Node.getBase()->IgnoreParenImpCasts());
+ if (!BaseDRE)
+return false;
+ if (!BaseDRE->getDecl())
+return false;
+ auto BaseVarDeclTy = BaseDRE->getDecl()->getType();
+ if (!BaseVarDeclTy->isConstantArrayType())
+return false;
+ const auto * CATy = dyn_cast_or_null(BaseVarDeclTy);
jkorous-apple wrote:
Use `getAsArrayType` instead of `dyn_cast`.
https://github.com/llvm/llvm-project/pull/80504
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
@@ -406,6 +406,31 @@ AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct)
{
}
return false;
}
+
+AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
+ const DeclRefExpr * BaseDRE =
dyn_cast_or_null(Node.getBase()->IgnoreParenImpCasts());
+ if (!BaseDRE)
+return false;
+ if (!BaseDRE->getDecl())
+return false;
+ auto BaseVarDeclTy = BaseDRE->getDecl()->getType();
+ if (!BaseVarDeclTy->isConstantArrayType())
+return false;
+ const auto * CATy = dyn_cast_or_null(BaseVarDeclTy);
jkorous-apple wrote:
Take a look getAsConstantArrayType().
https://github.com/llvm/llvm-project/pull/80504
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
@@ -598,16 +623,16 @@ class ArraySubscriptGadget : public WarningGadget {
}
static Matcher matcher() {
-// FIXME: What if the index is integer literal 0? Should this be
-// a safe gadget in this case?
- // clang-format off
+// clang-format off
return stmt(arraySubscriptExpr(
hasBase(ignoringParenImpCasts(
anyOf(hasPointerType(), hasArrayType(,
-unless(hasIndex(
-anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
- )))
-.bind(ArraySubscrTag));
+unless(anyOf(
+ isSafeArraySubscript(),
+ hasIndex(
+ anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
jkorous-apple wrote:
This is unnecessary now.
https://github.com/llvm/llvm-project/pull/80504
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
https://github.com/jkorous-apple updated
https://github.com/llvm/llvm-project/pull/80504
>From dfc9d95c185f5228f5c9680a19aa396d20e33d19 Mon Sep 17 00:00:00 2001
From: Jan Korous
Date: Thu, 25 Jan 2024 13:52:12 -0800
Subject: [PATCH 1/7] [-Wunsafe-buffer-usage] Emit fixits for arguments of
function pointers calls
Currently we ignore calls on function pointers (unlike direct calls of
functions and class methods). This patch adds support for function pointers as
well.
The change is to simply replace use of forEachArgumentWithParam matcher in UPC
gadget with forEachArgumentWithParamType.
from the documentation of forEachArgumentWithParamType:
/// Matches all arguments and their respective types for a \c CallExpr or
/// \c CXXConstructExpr. It is very similar to \c forEachArgumentWithParam but
/// it works on calls through function pointers as well.
Currently the matcher also uses hasPointerType() which checks that the
canonical type of an argument is pointer and won't match on arrays decayed to
pointer. Replacing hasPointerType() with isAnyPointerType() which allows
implicit casts allows for the arrays to be matched as well and this way we get
fixits for array arguments to function pointer calls too.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 4 ++--
...fer-usage-fixits-pointer-arg-to-func-ptr-call.cpp | 12
2 files changed, 14 insertions(+), 2 deletions(-)
create mode 100644
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index d00c598c4b9de3..c5a87f14bc8880 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -282,8 +282,8 @@ isInUnspecifiedPointerContext(internal::Matcher
InnerMatcher) {
//(i.e., computing the distance between two pointers); or ...
auto CallArgMatcher =
- callExpr(forEachArgumentWithParam(InnerMatcher,
- hasPointerType() /* array also decays to pointer type*/),
+ callExpr(forEachArgumentWithParamType(InnerMatcher,
+ isAnyPointer() /* array also decays to pointer type*/),
unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage);
auto CastOperandMatcher =
diff --git
a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
new file mode 100644
index 00..ae761e46a98191
--- /dev/null
+++
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+void unsafe_array_func_ptr_call(void (*fn_ptr)(int *param)) {
+ int p[32];
+ // CHECK-DAG:
fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array p"
+
+ int tmp = p[5];
+ fn_ptr(p);
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:11}:".data()"
+}
>From 44dab965c459f2cd6084ea332f4a6756f57254e0 Mon Sep 17 00:00:00 2001
From: Jan Korous
Date: Thu, 1 Feb 2024 14:44:01 -0800
Subject: [PATCH 2/7] [-Wunsafe-buffer-usage][NFC] Add tests for function
pointer call fixits
---
...ge-fixits-pointer-arg-to-func-ptr-call.cpp | 38 ++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git
a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
index ae761e46a98191..0459d6549fd86f 100644
---
a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
+++
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
@@ -6,7 +6,43 @@ void unsafe_array_func_ptr_call(void (*fn_ptr)(int *param)) {
int p[32];
// CHECK-DAG:
fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array p"
- int tmp = p[5];
+ p[5] = 10;
fn_ptr(p);
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:11}:".data()"
}
+
+void unsafe_ptr_func_ptr_call(void (*fn_ptr)(int *param)) {
+ int *p;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span
p"
+
+ p[5] = 10;
+ fn_ptr(p);
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:11}:".data()"
+}
+
+void addr_of_unsafe_ptr_func_ptr_call(void (*fn_ptr)(int *param)) {
+ int *p;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span
p"
+
+ p[5] = 10;
+ fn_ptr([0]);
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:15}:"p.data()"
+}
+
+void addr_of_unsafe_ptr_w_offset_func_ptr_call(void (*fn_ptr)(int *param)) {
+ int *p;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span
p"
+
+ p[5] = 10;
+ fn_ptr([3]);
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:15}:"()[3]"
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
llvmbot wrote:
@llvm/pr-subscribers-clang-analysis
@llvm/pr-subscribers-clang
Author: None (jkorous-apple)
Changes
depends on
https://github.com/llvm/llvm-project/pull/80358
---
Full diff: https://github.com/llvm/llvm-project/pull/80504.diff
5 Files Affected:
- (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+46-11)
- (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp (+17-1)
- (modified)
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp (+4-4)
- (added)
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
(+49)
- (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp (+27-26)
``diff
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index d00c598c4b9de3..aa3240a86e562b 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -281,10 +281,13 @@ isInUnspecifiedPointerContext(internal::Matcher
InnerMatcher) {
// 4. the operand of a pointer subtraction operation
//(i.e., computing the distance between two pointers); or ...
- auto CallArgMatcher =
- callExpr(forEachArgumentWithParam(InnerMatcher,
- hasPointerType() /* array also decays to pointer type*/),
- unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage);
+ // clang-format off
+ auto CallArgMatcher = callExpr(
+forEachArgumentWithParamType(
+ InnerMatcher,
+ isAnyPointer() /* array also decays to pointer type*/),
+unless(callee(
+ functionDecl(hasAttr(attr::UnsafeBufferUsage);
auto CastOperandMatcher =
castExpr(anyOf(hasCastKind(CastKind::CK_PointerToIntegral),
@@ -306,6 +309,7 @@ isInUnspecifiedPointerContext(internal::Matcher
InnerMatcher) {
hasRHS(hasPointerType())),
eachOf(hasLHS(InnerMatcher),
hasRHS(InnerMatcher)));
+ // clang-format on
return stmt(anyOf(CallArgMatcher, CastOperandMatcher, CompOperandMatcher,
PtrSubtractionMatcher));
@@ -402,6 +406,37 @@ AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct)
{
}
return false;
}
+
+AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
+ // FIXME: Proper solution:
+ // - refactor Sema::CheckArrayAccess
+ //- split safe/OOB/unknown decision logic from diagnostics emitting code
+ //- e. g. "Try harder to find a NamedDecl to point at in the note."
already duplicated
+ // - call both from Sema and from here
+
+ const DeclRefExpr * BaseDRE =
dyn_cast_or_null(Node.getBase()->IgnoreParenImpCasts());
+ if (!BaseDRE)
+return false;
+ if (!BaseDRE->getDecl())
+return false;
+ auto BaseVarDeclTy = BaseDRE->getDecl()->getType();
+ if (!BaseVarDeclTy->isConstantArrayType())
+return false;
+ const auto * CATy = dyn_cast_or_null(BaseVarDeclTy);
+ if (!CATy)
+return false;
+ const APInt ArrSize = CATy->getSize();
+
+ if (const auto * IdxLit = dyn_cast(Node.getIdx())) {
+const APInt ArrIdx = IdxLit->getValue();
+// FIXME: ArrIdx.isNegative() we could immediately emit an error as that's
a bug
+if (ArrIdx.isNonNegative() && ArrIdx.getLimitedValue() <
ArrSize.getLimitedValue())
+ return true;
+ }
+
+ return false;
+}
+
} // namespace clang::ast_matchers
namespace {
@@ -594,16 +629,16 @@ class ArraySubscriptGadget : public WarningGadget {
}
static Matcher matcher() {
-// FIXME: What if the index is integer literal 0? Should this be
-// a safe gadget in this case?
- // clang-format off
+// clang-format off
return stmt(arraySubscriptExpr(
hasBase(ignoringParenImpCasts(
anyOf(hasPointerType(), hasArrayType(,
-unless(hasIndex(
-anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
- )))
-.bind(ArraySubscrTag));
+unless(anyOf(
+ isSafeArraySubscript(),
+ hasIndex(
+ anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
+ )
+))).bind(ArraySubscrTag));
// clang-format on
}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
index 90c11b1be95c25..4804223e8be058 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN: %clang_cc1 -std=c++20 -Wno-everything -Wunsafe-buffer-usage \
// RUN:-fsafe-buffer-usage-suggestions \
// RUN:-verify %s
@@ -22,3 +22,19 @@ struct Foo {
void foo2(Foo& f, unsigned idx) {
f.member_buffer[idx] = 0; // expected-warning{{unsafe buffer access}}
}
+
+void constant_idx_safe(unsigned idx) {
+ int buffer[10];
+ buffer[9] = 0;
+}
+
+void constant_idx_safe0(unsigned idx) {
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
https://github.com/jkorous-apple updated
https://github.com/llvm/llvm-project/pull/80504
>From dfc9d95c185f5228f5c9680a19aa396d20e33d19 Mon Sep 17 00:00:00 2001
From: Jan Korous
Date: Thu, 25 Jan 2024 13:52:12 -0800
Subject: [PATCH 1/6] [-Wunsafe-buffer-usage] Emit fixits for arguments of
function pointers calls
Currently we ignore calls on function pointers (unlike direct calls of
functions and class methods). This patch adds support for function pointers as
well.
The change is to simply replace use of forEachArgumentWithParam matcher in UPC
gadget with forEachArgumentWithParamType.
from the documentation of forEachArgumentWithParamType:
/// Matches all arguments and their respective types for a \c CallExpr or
/// \c CXXConstructExpr. It is very similar to \c forEachArgumentWithParam but
/// it works on calls through function pointers as well.
Currently the matcher also uses hasPointerType() which checks that the
canonical type of an argument is pointer and won't match on arrays decayed to
pointer. Replacing hasPointerType() with isAnyPointerType() which allows
implicit casts allows for the arrays to be matched as well and this way we get
fixits for array arguments to function pointer calls too.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 4 ++--
...fer-usage-fixits-pointer-arg-to-func-ptr-call.cpp | 12
2 files changed, 14 insertions(+), 2 deletions(-)
create mode 100644
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index d00c598c4b9de3..c5a87f14bc8880 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -282,8 +282,8 @@ isInUnspecifiedPointerContext(internal::Matcher
InnerMatcher) {
//(i.e., computing the distance between two pointers); or ...
auto CallArgMatcher =
- callExpr(forEachArgumentWithParam(InnerMatcher,
- hasPointerType() /* array also decays to pointer type*/),
+ callExpr(forEachArgumentWithParamType(InnerMatcher,
+ isAnyPointer() /* array also decays to pointer type*/),
unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage);
auto CastOperandMatcher =
diff --git
a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
new file mode 100644
index 00..ae761e46a98191
--- /dev/null
+++
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+void unsafe_array_func_ptr_call(void (*fn_ptr)(int *param)) {
+ int p[32];
+ // CHECK-DAG:
fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array p"
+
+ int tmp = p[5];
+ fn_ptr(p);
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:11}:".data()"
+}
>From 44dab965c459f2cd6084ea332f4a6756f57254e0 Mon Sep 17 00:00:00 2001
From: Jan Korous
Date: Thu, 1 Feb 2024 14:44:01 -0800
Subject: [PATCH 2/6] [-Wunsafe-buffer-usage][NFC] Add tests for function
pointer call fixits
---
...ge-fixits-pointer-arg-to-func-ptr-call.cpp | 38 ++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git
a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
index ae761e46a98191..0459d6549fd86f 100644
---
a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
+++
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
@@ -6,7 +6,43 @@ void unsafe_array_func_ptr_call(void (*fn_ptr)(int *param)) {
int p[32];
// CHECK-DAG:
fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array p"
- int tmp = p[5];
+ p[5] = 10;
fn_ptr(p);
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:11}:".data()"
}
+
+void unsafe_ptr_func_ptr_call(void (*fn_ptr)(int *param)) {
+ int *p;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span
p"
+
+ p[5] = 10;
+ fn_ptr(p);
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:11}:".data()"
+}
+
+void addr_of_unsafe_ptr_func_ptr_call(void (*fn_ptr)(int *param)) {
+ int *p;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span
p"
+
+ p[5] = 10;
+ fn_ptr([0]);
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:15}:"p.data()"
+}
+
+void addr_of_unsafe_ptr_w_offset_func_ptr_call(void (*fn_ptr)(int *param)) {
+ int *p;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span
p"
+
+ p[5] = 10;
+ fn_ptr([3]);
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:15}:"()[3]"
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
https://github.com/jkorous-apple updated
https://github.com/llvm/llvm-project/pull/80504
>From 463a9904c1ae85fbdc0bd6029c6effea3fb16ea6 Mon Sep 17 00:00:00 2001
From: Jan Korous
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 01/20] [-Wunsafe-buffer-usage] Move Strategy class to the
header
It needs to be used from handleUnsafeVariableGroup function.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 37 +++
clang/lib/Analysis/UnsafeBufferUsage.cpp | 238 --
2 files changed, 140 insertions(+), 135 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index aca1ad998822c5..622c094f5b1eb1 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -42,6 +42,43 @@ class VariableGroupsManager {
virtual VarGrpRef getGroupOfParms() const =0;
};
+// FixitStrategy is a map from variables to the way we plan to emit fixes for
+// these variables. It is figured out gradually by trying different fixes
+// for different variables depending on gadgets in which these variables
+// participate.
+class FixitStrategy {
+public:
+ enum class Kind {
+Wontfix, // We don't plan to emit a fixit for this variable.
+Span, // We recommend replacing the variable with std::span.
+Iterator, // We recommend replacing the variable with std::span::iterator.
+Array,// We recommend replacing the variable with std::array.
+Vector// We recommend replacing the variable with std::vector.
+ };
+
+private:
+ using MapTy = llvm::DenseMap;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(FixitStrategy &&) = default;
+
+ void set(const VarDecl *VD, Kind K) { Map[VD] = K; }
+
+ Kind lookup(const VarDecl *VD) const {
+auto I = Map.find(VD);
+if (I == Map.end())
+ return Kind::Wontfix;
+
+return I->second;
+ }
+};
+
/// The interface that lets the caller handle unsafe buffer usage analysis
/// results by overriding this class's handle... methods.
class UnsafeBufferUsageHandler {
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 823cd2a7b99691..924d677786275d 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -407,9 +407,6 @@ using DeclUseList = SmallVector;
// Convenience typedef.
using FixItList = SmallVector;
-
-// Defined below.
-class Strategy;
} // namespace
namespace {
@@ -486,7 +483,7 @@ class FixableGadget : public Gadget {
/// Returns a fixit that would fix the current gadget according to
/// the current strategy. Returns std::nullopt if the fix cannot be produced;
/// returns an empty list if no fixes are necessary.
- virtual std::optional getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
virtual const Stmt *getBaseStmt() const override {
// FIXME: This needs to be the entire DeclStmt, assuming that this method
@@ -789,7 +787,8 @@ class PointerAssignmentGadget : public FixableGadget {
return stmt(isInUnspecifiedUntypedContext(PtrAssignExpr));
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
virtual const Stmt *getBaseStmt() const override {
// FIXME: This should be the binary operator, assuming that this method
@@ -892,7 +891,8 @@ class ULCArraySubscriptGadget : public FixableGadget {
return expr(isInUnspecifiedLvalueContext(Target));
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
virtual const Stmt *getBaseStmt() const override { return Node; }
@@ -932,7 +932,8 @@ class UPCStandalonePointerGadget : public FixableGadget {
return stmt(isInUnspecifiedPointerContext(target));
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
virtual const Stmt *getBaseStmt() const override { return Node; }
@@ -976,7 +977,8 @@ class PointerDereferenceGadget : public FixableGadget {
virtual const Stmt *getBaseStmt() const final { return Op; }
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
};
//
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff 86cd2fbdfe67d70a7fe061ed5d3a644f50f070f5
b510cf7ca8c47c64e0b9f5fcd5634759dfe95751 --
clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
``
View the diff from clang-format here.
``diff
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 8f8751f4ee..4399d15f46 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -281,10 +281,10 @@ isInUnspecifiedPointerContext(internal::Matcher
InnerMatcher) {
// 4. the operand of a pointer subtraction operation
//(i.e., computing the distance between two pointers); or ...
- auto CallArgMatcher =
- callExpr(forEachArgumentWithParamType(InnerMatcher,
- isAnyPointer() /* array also decays to pointer type*/),
- unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage);
+ auto CallArgMatcher = callExpr(
+ forEachArgumentWithParamType(
+ InnerMatcher, isAnyPointer() /* array also decays to pointer type*/),
+ unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage);
auto CastOperandMatcher =
castExpr(anyOf(hasCastKind(CastKind::CK_PointerToIntegral),
@@ -404,7 +404,8 @@ AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct) {
}
AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
- const DeclRefExpr * BaseDRE =
dyn_cast_or_null(Node.getBase()->IgnoreParenImpCasts());
+ const DeclRefExpr *BaseDRE =
+ dyn_cast_or_null(Node.getBase()->IgnoreParenImpCasts());
if (!BaseDRE)
return false;
if (!BaseDRE->getDecl())
@@ -412,15 +413,17 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
auto BaseVarDeclTy = BaseDRE->getDecl()->getType();
if (!BaseVarDeclTy->isConstantArrayType())
return false;
- const auto * CATy = dyn_cast_or_null(BaseVarDeclTy);
+ const auto *CATy = dyn_cast_or_null(BaseVarDeclTy);
if (!CATy)
return false;
const APInt ArrSize = CATy->getSize();
- if (const auto * IdxLit = dyn_cast(Node.getIdx())) {
+ if (const auto *IdxLit = dyn_cast(Node.getIdx())) {
const APInt ArrIdx = IdxLit->getValue();
-// FIXME: ArrIdx.isNegative() we could immediately emit an error as that's
a bug
-if (ArrIdx.isNonNegative() && ArrIdx.getLimitedValue() <
ArrSize.getLimitedValue())
+// FIXME: ArrIdx.isNegative() we could immediately emit an error as that's
a
+// bug
+if (ArrIdx.isNonNegative() &&
+ArrIdx.getLimitedValue() < ArrSize.getLimitedValue())
return true;
}
``
https://github.com/llvm/llvm-project/pull/80504
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
https://github.com/jkorous-apple created
https://github.com/llvm/llvm-project/pull/80504
depends on
https://github.com/llvm/llvm-project/pull/80358
>From 463a9904c1ae85fbdc0bd6029c6effea3fb16ea6 Mon Sep 17 00:00:00 2001
From: Jan Korous
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 01/19] [-Wunsafe-buffer-usage] Move Strategy class to the
header
It needs to be used from handleUnsafeVariableGroup function.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 37 +++
clang/lib/Analysis/UnsafeBufferUsage.cpp | 238 --
2 files changed, 140 insertions(+), 135 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index aca1ad998822c..622c094f5b1eb 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -42,6 +42,43 @@ class VariableGroupsManager {
virtual VarGrpRef getGroupOfParms() const =0;
};
+// FixitStrategy is a map from variables to the way we plan to emit fixes for
+// these variables. It is figured out gradually by trying different fixes
+// for different variables depending on gadgets in which these variables
+// participate.
+class FixitStrategy {
+public:
+ enum class Kind {
+Wontfix, // We don't plan to emit a fixit for this variable.
+Span, // We recommend replacing the variable with std::span.
+Iterator, // We recommend replacing the variable with std::span::iterator.
+Array,// We recommend replacing the variable with std::array.
+Vector// We recommend replacing the variable with std::vector.
+ };
+
+private:
+ using MapTy = llvm::DenseMap;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(FixitStrategy &&) = default;
+
+ void set(const VarDecl *VD, Kind K) { Map[VD] = K; }
+
+ Kind lookup(const VarDecl *VD) const {
+auto I = Map.find(VD);
+if (I == Map.end())
+ return Kind::Wontfix;
+
+return I->second;
+ }
+};
+
/// The interface that lets the caller handle unsafe buffer usage analysis
/// results by overriding this class's handle... methods.
class UnsafeBufferUsageHandler {
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 823cd2a7b9969..924d677786275 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -407,9 +407,6 @@ using DeclUseList = SmallVector;
// Convenience typedef.
using FixItList = SmallVector;
-
-// Defined below.
-class Strategy;
} // namespace
namespace {
@@ -486,7 +483,7 @@ class FixableGadget : public Gadget {
/// Returns a fixit that would fix the current gadget according to
/// the current strategy. Returns std::nullopt if the fix cannot be produced;
/// returns an empty list if no fixes are necessary.
- virtual std::optional getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
virtual const Stmt *getBaseStmt() const override {
// FIXME: This needs to be the entire DeclStmt, assuming that this method
@@ -789,7 +787,8 @@ class PointerAssignmentGadget : public FixableGadget {
return stmt(isInUnspecifiedUntypedContext(PtrAssignExpr));
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
virtual const Stmt *getBaseStmt() const override {
// FIXME: This should be the binary operator, assuming that this method
@@ -892,7 +891,8 @@ class ULCArraySubscriptGadget : public FixableGadget {
return expr(isInUnspecifiedLvalueContext(Target));
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
virtual const Stmt *getBaseStmt() const override { return Node; }
@@ -932,7 +932,8 @@ class UPCStandalonePointerGadget : public FixableGadget {
return stmt(isInUnspecifiedPointerContext(target));
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
virtual const Stmt *getBaseStmt() const override { return Node; }
@@ -976,7 +977,8 @@ class PointerDereferenceGadget : public FixableGadget {
virtual const Stmt *getBaseStmt() const final { return Op; }
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+
