[-Wunsafe-buffer-usage] Fix false warnings when const sized array is safely accessed (array [idx %const]) #140113
Conversation
… accessed array [idx %const]
The -Wunsafe-buffer-usage analysis currently warns when a const sized array is safely accessed,
with an index that is bound by the remainder operator. The false alarm is now suppressed.
Example:
int circular_buffer(int array[10], uint idx) {
return array[idx %10]; // Safe operation
}
rdar://148443453
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-analysis Author: Malavika Samak (malavikasamak) ChangesThe -Wunsafe-buffer-usage analysis currently warns when a const sized array is safely accessed, with an index that is bound by the remainder operator. The false alarm is now suppressed. Example: int circular_buffer(int array[10], uint idx) { rdar://148443453 Full diff: https://github.com/llvm/llvm-project/pull/140113.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 5b72382ca9772..2f669637a2731 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -600,12 +600,27 @@ static bool isSafeArraySubscript(const ArraySubscriptExpr &Node,
} else if (const auto *BE = dyn_cast<BinaryOperator>(IndexExpr)) {
// For an integer expression `e` and an integer constant `n`, `e & n` and
// `n & e` are bounded by `n`:
- if (BE->getOpcode() != BO_And)
+ if (BE->getOpcode() != BO_And && BE->getOpcode() != BO_Rem)
return false;
const Expr *LHS = BE->getLHS();
const Expr *RHS = BE->getRHS();
+ if(BE->getOpcode() == BO_Rem) {
+ // If n is a negative number, then n % const can be greater than const
+ if(!LHS->getType()->isUnsignedIntegerType()) {
+ return false;
+ }
+
+ if(!RHS->isValueDependent() && RHS->EvaluateAsInt(EVResult, Ctx)) {
+ llvm::APSInt result = EVResult.Val.getInt();
+ if (result.isNonNegative() && result.getLimitedValue() <= limit)
+ return true;
+ }
+
+ return false;
+ }
+
if ((!LHS->isValueDependent() &&
LHS->EvaluateAsInt(EVResult, Ctx)) || // case: `n & e`
(!RHS->isValueDependent() &&
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
index 3233999ea8ea2..b2358bb9f6be3 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
@@ -35,7 +35,15 @@ void constant_idx_safe0(unsigned idx) {
buffer[0] = 0;
}
-int array[10]; // expected-warning {{'array' is an unsafe buffer that does not perform bounds checks}}
+int array[10]; // expected-warning 2{{'array' is an unsafe buffer that does not perform bounds checks}}
+
+void circular_access_safe(unsigned idx) {
+ array[idx % 10];
+}
+
+void circular_access_unsafe(int idx) {
+ array[idx % 10]; // expected-note {{used in buffer access here}}
+}
void masked_idx1(unsigned long long idx, Foo f) {
// Bitwise and operation
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes false warnings triggered by the -Wunsafe-buffer-usage analysis when a const sized array is safely accessed using the modulo operator.
- Updated test expectations in warn-unsafe-buffer-usage-array.cpp to account for dual warnings and added new test functions for circular access.
- Extended and refined the safety check logic in UnsafeBufferUsage.cpp to correctly handle the BO_Rem operator for remainder operations.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp | Updated test comments and added test functions for safe and unsafe circular array access. |
| clang/lib/Analysis/UnsafeBufferUsage.cpp | Modified the safety check to allow BO_Rem and added specific handling for unsigned types. |
| } | ||
|
|
||
| int array[10]; // expected-warning {{'array' is an unsafe buffer that does not perform bounds checks}} | ||
| int array[10]; // expected-warning 2{{'array' is an unsafe buffer that does not perform bounds checks}} |
There was a problem hiding this comment.
Ensure that the update from a single expected warning to two is intentional and that the test comment clearly communicates the new warning count rationale.
|
|
||
| if(!RHS->isValueDependent() && RHS->EvaluateAsInt(EVResult, Ctx)) { | ||
| llvm::APSInt result = EVResult.Val.getInt(); | ||
| if (result.isNonNegative() && result.getLimitedValue() <= limit) |
There was a problem hiding this comment.
[nitpick] Consider adding a comment to explain the purpose and derivation of the 'limit' variable to enhance clarity on how it determines the safe boundary for remainder operations.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
malavikasamak
requested review from
devincoughlin,
jkorous-apple,
t-rasmud and
ziqingluo-90
|
CC @dtarditi |
ziqingluo-90
left a comment
ziqingluo-90
left a comment
There was a problem hiding this comment.
LGTM!
(AI comments in this PR aren't very helpful IMO 😅)
3 participants
The -Wunsafe-buffer-usage analysis currently warns when a const sized array is safely accessed, with an index that is bound by the remainder operator. The false alarm is now suppressed.
Example:
int circular_buffer(int array[10], uint idx) {
return array[idx %10]; // Safe operation
}
rdar://148443453