Remove warnings from -Wchar-subscripts for known positive constants #69061

wheatman · 2023-10-14T16:15:07Z

This is to address #18763

it removes warnings from using a signed char as an array bound if the char is a known positives constant.

This goes one step farther than gcc does.

For example given the following code

char upper[300];

int main() {
  upper['a'] = 'A';
  char b = 'a';
  upper[b] = 'A';
  const char c = 'a';
  upper[c] = 'A';
  constexpr char d = 'a';
  upper[d] = 'A';
  char e = -1;
  upper[e] = 'A';
  const char f = -1;
  upper[f] = 'A';
  constexpr char g = -1;
  upper[g] = 'A';
  return 1;
}

clang currently gives warnings for all cases, while gcc gives warnings for all cases except for 'a' (https://godbolt.org/z/5ahjETTv3)

with the change there is no longer any warning for 'a', 'c', or 'd'.

../test.cpp:7:8: warning: array subscript is of type 'char' [-Wchar-subscripts]
    7 |   upper[b] = 'A';
      |        ^~
../test.cpp:13:8: warning: array subscript is of type 'char' [-Wchar-subscripts]
   13 |   upper[e] = 'A';
      |        ^~
../test.cpp:15:8: warning: array subscript is of type 'char' [-Wchar-subscripts]
   15 |   upper[f] = 'A';
      |        ^~
../test.cpp:17:8: warning: array subscript is of type 'char' [-Wchar-subscripts]
   17 |   upper[g] = 'A';
      |        ^~
../test.cpp:15:3: warning: array index -1 is before the beginning of the array [-Warray-bounds]
   15 |   upper[f] = 'A';
      |   ^     ~
../test.cpp:1:1: note: array 'upper' declared here
    1 | char upper[300];
      | ^
../test.cpp:17:3: warning: array index -1 is before the beginning of the array [-Warray-bounds]
   17 |   upper[g] = 'A';
      |   ^     ~
../test.cpp:1:1: note: array 'upper' declared here
    1 | char upper[300];
      | ^
6 warnings generated.

This pull request is my first change and I would appreciate any comments or suggestions.

llvmbot · 2023-10-14T16:16:45Z

@llvm/pr-subscribers-clang

Author: None (wheatman)

Changes

This is to address #18763

it removes warnings from using a signed char as an array bound if the char is a known positives constant.

This goes one step farther than gcc does.

For example given the following code

char upper[300];

int main() {
  upper['a'] = 'A';
  char b = 'a';
  upper[b] = 'A';
  const char c = 'a';
  upper[c] = 'A';
  constexpr char d = 'a';
  upper[d] = 'A';
  char e = -1;
  upper[e] = 'A';
  const char f = -1;
  upper[f] = 'A';
  constexpr char g = -1;
  upper[g] = 'A';
  return 1;
}

clang currently gives warnings for all cases, while gcc gives warnings for all cases except for 'a' (https://godbolt.org/z/5ahjETTv3)

with the change there is no longer any warning for 'a', 'c', or 'd'.

../test.cpp:7:8: warning: array subscript is of type 'char' [-Wchar-subscripts]
    7 |   upper[b] = 'A';
      |        ^~
../test.cpp:13:8: warning: array subscript is of type 'char' [-Wchar-subscripts]
   13 |   upper[e] = 'A';
      |        ^~
../test.cpp:15:8: warning: array subscript is of type 'char' [-Wchar-subscripts]
   15 |   upper[f] = 'A';
      |        ^~
../test.cpp:17:8: warning: array subscript is of type 'char' [-Wchar-subscripts]
   17 |   upper[g] = 'A';
      |        ^~
../test.cpp:15:3: warning: array index -1 is before the beginning of the array [-Warray-bounds]
   15 |   upper[f] = 'A';
      |   ^     ~
../test.cpp:1:1: note: array 'upper' declared here
    1 | char upper[300];
      | ^
../test.cpp:17:3: warning: array index -1 is before the beginning of the array [-Warray-bounds]
   17 |   upper[g] = 'A';
      |   ^     ~
../test.cpp:1:1: note: array 'upper' declared here
    1 | char upper[300];
      | ^
6 warnings generated.

This pull request in incomplete in that this is my first change submitted and I don't know how to add tests, any guidance on what sort of tests to add, and where to find documentation on the testing infrastructure

Full diff: https://github.com/llvm/llvm-project/pull/69061.diff

1 Files Affected:

(modified) clang/lib/Sema/SemaExpr.cpp (+9-3)

diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index aa30a3a03887558..dd9ba5cecaf2404 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -6018,9 +6018,15 @@ Sema::CreateBuiltinArraySubscriptExpr(Expr *Base, SourceLocation LLoc,
                      << IndexExpr->getSourceRange());
 
   if ((IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_S) ||
-       IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_U))
-         && !IndexExpr->isTypeDependent())
-    Diag(LLoc, diag::warn_subscript_is_char) << IndexExpr->getSourceRange();
+       IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_U)) &&
+      !IndexExpr->isTypeDependent()) {
+    std::optional<llvm::APSInt> IntegerContantExpr =
+        IndexExpr->getIntegerConstantExpr(getASTContext());
+    if (!(IntegerContantExpr.has_value() &&
+          IntegerContantExpr.value().isNonNegative())) {
+      Diag(LLoc, diag::warn_subscript_is_char) << IndexExpr->getSourceRange();
+    }
+  }
 
   // C99 6.5.2.1p1: "shall have type "pointer to *object* type". Similarly,
   // C++ [expr.sub]p1: The type "T" shall be a completely-defined object

shafik

You need to make sure you tests have newlines at the end

shafik · 2023-10-25T21:18:03Z

clang/test/Sema/warn-char-subscripts.cpp

+void t6(void) {
+  int array[1] = { 0 };
+  signed char subscript = 0;
+  int val = array[subscript]; // no warning for explicit signed char


Why do we exclude the signed char case?

That is an interesting questions, the behavior for that was not changed.
The current behavior for explicitly signed or unsigned chars is to have no warning, only unspecified chars have warnings.

https://godbolt.org/z/7oc3ET4h4

We rule out explicitly signed or explicitly unsigned char type because of int8_t and uint8_t because those are pretty reasonable to use as array indexes.

AaronBallman

Thank you for this! The changes should come with a release note in clang/docs/ReleaseNotes.rst so users know about the change in behavior.

AaronBallman · 2023-11-02T16:41:07Z

clang/lib/Sema/SemaExpr.cpp

+    std::optional<llvm::APSInt> IntegerContantExpr =
+        IndexExpr->getIntegerConstantExpr(getASTContext());
+    if (!(IntegerContantExpr.has_value() &&
+          IntegerContantExpr.value().isNonNegative())) {
+      Diag(LLoc, diag::warn_subscript_is_char) << IndexExpr->getSourceRange();
+    }


Suggested change

std::optional<llvm::APSInt> IntegerContantExpr =

IndexExpr->getIntegerConstantExpr(getASTContext());

if (!(IntegerContantExpr.has_value() &&

IntegerContantExpr.value().isNonNegative())) {

Diag(LLoc, diag::warn_subscript_is_char) << IndexExpr->getSourceRange();

}

std::optional<llvm::APSInt> IntegerContantExpr =

IndexExpr->getIntegerConstantExpr(getASTContext());

if (!IntegerContantExpr.has_value() ||

IntegerContantExpr.value().isNegative())

Diag(LLoc, diag::warn_subscript_is_char) << IndexExpr->getSourceRange();

This performs extra work for each array subscript in the TU, but because it's limited to only array subscripts with a character type, I don't think the compile-time performance hit should be too bad, so this seems reasonable to me.

clang/test/Sema/warn-char-subscripts.c

AaronBallman · 2023-11-02T16:46:07Z

clang/test/Sema/warn-char-subscripts.cpp

+void t6(void) {
+  int array[1] = { 0 };
+  signed char subscript = 0;
+  int val = array[subscript]; // no warning for explicit signed char


We rule out explicitly signed or explicitly unsigned char type because of int8_t and uint8_t because those are pretty reasonable to use as array indexes.

wheatman · 2023-11-02T19:51:53Z

Thanks you for the comments. I made the updates and added the note in clang/docs/ReleaseNotes.rst

github-actions · 2023-11-02T19:55:39Z

✅ With the latest revision this PR passed the C/C++ code formatter.

AaronBallman

LGTM, thank you for the fix!

wheatman · 2023-11-03T15:05:49Z

Thank you for the review.
I don't have write access, so unless @shafik has more comments, could one of you merge it in

wheatman · 2023-11-16T19:01:57Z

@AaronBallman if there are no more comments, could you please merge it in

cor3ntin · 2023-12-03T09:07:52Z

@wheatman can you rebase again? I want to make sure CI passes before merging (and the bots were broken earlier today). Thanks.
LGTM otherwise

wheatman · 2023-12-03T14:26:33Z

@cor3ntin done, thanks for taking a look

cor3ntin · 2023-12-04T05:50:13Z

Thanks for your first contribution. Maybe the first of many :D

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 14, 2023

wheatman force-pushed the main branch from b3111e9 to 44e6663 Compare October 14, 2023 16:56

wheatman added the clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer label Oct 14, 2023

shafik reviewed Oct 25, 2023

View reviewed changes

wheatman force-pushed the main branch from 44e6663 to af3b7f9 Compare October 25, 2023 22:09

AaronBallman reviewed Nov 2, 2023

View reviewed changes

wheatman force-pushed the main branch from af3b7f9 to b46d376 Compare November 2, 2023 19:45

wheatman force-pushed the main branch from b46d376 to 20e4717 Compare November 2, 2023 20:02

AaronBallman approved these changes Nov 3, 2023

View reviewed changes

wheatman force-pushed the main branch from 20e4717 to d19da42 Compare November 16, 2023 16:25

wheatman force-pushed the main branch from d19da42 to 894e862 Compare December 3, 2023 00:49

Remove warnings from -Wchar-subscripts for known positive constants

df15b62

wheatman force-pushed the main branch from 894e862 to df15b62 Compare December 3, 2023 13:44

cor3ntin merged commit 0031efe into llvm:main Dec 4, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove warnings from -Wchar-subscripts for known positive constants #69061

Remove warnings from -Wchar-subscripts for known positive constants #69061

wheatman commented Oct 14, 2023 •

edited

llvmbot commented Oct 14, 2023

shafik left a comment

shafik Oct 25, 2023

wheatman Oct 25, 2023

AaronBallman Nov 2, 2023

AaronBallman left a comment

AaronBallman Nov 2, 2023

AaronBallman Nov 2, 2023

wheatman commented Nov 2, 2023

github-actions bot commented Nov 2, 2023 •

edited

AaronBallman left a comment

wheatman commented Nov 3, 2023

wheatman commented Nov 16, 2023

cor3ntin commented Dec 3, 2023

wheatman commented Dec 3, 2023

cor3ntin commented Dec 4, 2023

Remove warnings from -Wchar-subscripts for known positive constants #69061

Remove warnings from -Wchar-subscripts for known positive constants #69061

Conversation

wheatman commented Oct 14, 2023 • edited

llvmbot commented Oct 14, 2023

shafik left a comment

Choose a reason for hiding this comment

shafik Oct 25, 2023

Choose a reason for hiding this comment

wheatman Oct 25, 2023

Choose a reason for hiding this comment

AaronBallman Nov 2, 2023

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

AaronBallman Nov 2, 2023

Choose a reason for hiding this comment

AaronBallman Nov 2, 2023

Choose a reason for hiding this comment

wheatman commented Nov 2, 2023

github-actions bot commented Nov 2, 2023 • edited

AaronBallman left a comment

Choose a reason for hiding this comment

wheatman commented Nov 3, 2023

wheatman commented Nov 16, 2023

cor3ntin commented Dec 3, 2023

wheatman commented Dec 3, 2023

cor3ntin commented Dec 4, 2023

wheatman commented Oct 14, 2023 •

edited

github-actions bot commented Nov 2, 2023 •

edited