[Sema] Fix tautological bounds check warning with -fwrapv #120480

nathanchance · 2024-12-18T21:09:39Z

The tautological bounds check warning added in #120222 does not take into account whether signed integer overflow is well defined or not, which could result in a developer removing a bounds check that may not actually be always false because of different overflow semantics.

int check(const int* foo, unsigned int idx)
{
    return foo + idx < foo;
}

$ clang -O2 -c test.c
test.c:3:19: warning: pointer comparison always evaluates to false [-Wtautological-compare]
    3 |         return foo + idx < foo;
      |                          ^
1 warning generated.

# Bounds check is eliminated without -fwrapv, warning was correct
$ llvm-objdump -dr test.o
...
0000000000000000 <check>:
       0: 31 c0                         xorl    %eax, %eax
       2: c3                            retq

$ clang -O2 -fwrapv -c test.c
test.c:3:19: warning: pointer comparison always evaluates to false [-Wtautological-compare]
    3 |         return foo + idx < foo;
      |                          ^
1 warning generated.

# Bounds check remains, warning was wrong
$ llvm-objdump -dr test.o
0000000000000000 <check>:
       0: 89 f0                         movl    %esi, %eax
       2: 48 8d 0c 87                   leaq    (%rdi,%rax,4), %rcx
       6: 31 c0                         xorl    %eax, %eax
       8: 48 39 f9                      cmpq    %rdi, %rcx
       b: 0f 92 c0                      setb    %al
       e: c3                            retq

Prevent the warning from firing when -fwrapv is enabled.

The tautological bounds check warning added in llvm#120222 does not take into account whether signed integer overflow is well defined or not, which could result in a developer removing a bounds check that may not actually be always false because of different overflow semantics. $ cat test.c int check(const int* foo, unsigned int idx) { return foo + idx < foo; } $ clang -O2 -c test.c test.c:3:19: warning: pointer comparison always evaluates to false [-Wtautological-compare] 3 | return foo + idx < foo; | ^ 1 warning generated. # Bounds check is eliminated without -fwrapv, warning was correct $ llvm-objdump -dr test.o ... 0000000000000000 <check>: 0: 31 c0 xorl %eax, %eax 2: c3 retq $ clang -O2 -fwrapv -c test.c test.c:3:19: warning: pointer comparison always evaluates to false [-Wtautological-compare] 3 | return foo + idx < foo; | ^ 1 warning generated. # Bounds check remains, warning was wrong $ llvm-objdump -dr test.o 0000000000000000 <check>: 0: 89 f0 movl %esi, %eax 2: 48 8d 0c 87 leaq (%rdi,%rax,4), %rcx 6: 31 c0 xorl %eax, %eax 8: 48 39 f9 cmpq %rdi, %rcx b: 0f 92 c0 setb %al e: c3 retq Prevent the warning from firing when -fwrapv is enabled.

llvmbot · 2024-12-18T21:10:12Z

@llvm/pr-subscribers-clang

Author: Nathan Chancellor (nathanchance)

Changes

The tautological bounds check warning added in #120222 does not take into account whether signed integer overflow is well defined or not, which could result in a developer removing a bounds check that may not actually be always false because of different overflow semantics.

int check(const int* foo, unsigned int idx)
{
    return foo + idx &lt; foo;
}

$ clang -O2 -c test.c
test.c:3:19: warning: pointer comparison always evaluates to false [-Wtautological-compare]
    3 |         return foo + idx &lt; foo;
      |                          ^
1 warning generated.

# Bounds check is eliminated without -fwrapv, warning was correct
$ llvm-objdump -dr test.o
...
0000000000000000 &lt;check&gt;:
       0: 31 c0                         xorl    %eax, %eax
       2: c3                            retq

$ clang -O2 -fwrapv -c test.c
test.c:3:19: warning: pointer comparison always evaluates to false [-Wtautological-compare]
    3 |         return foo + idx &lt; foo;
      |                          ^
1 warning generated.

# Bounds check remains, warning was wrong
$ llvm-objdump -dr test.o
0000000000000000 &lt;check&gt;:
       0: 89 f0                         movl    %esi, %eax
       2: 48 8d 0c 87                   leaq    (%rdi,%rax,4), %rcx
       6: 31 c0                         xorl    %eax, %eax
       8: 48 39 f9                      cmpq    %rdi, %rcx
       b: 0f 92 c0                      setb    %al
       e: c3                            retq

Prevent the warning from firing when -fwrapv is enabled.

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

2 Files Affected:

(modified) clang/lib/Sema/SemaExpr.cpp (+4-3)
(modified) clang/test/Sema/tautological-pointer-comparison.c (+41-9)

diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index e06a092177ef02..24f7d27c691154 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -11789,10 +11789,11 @@ static bool checkForArray(const Expr *E) {
 /// Detect patterns ptr + size >= ptr and ptr + size < ptr, where ptr is a
 /// pointer and size is an unsigned integer. Return whether the result is
 /// always true/false.
-static std::optional<bool> isTautologicalBoundsCheck(const Expr *LHS,
+static std::optional<bool> isTautologicalBoundsCheck(Sema &S, const Expr *LHS,
                                                      const Expr *RHS,
                                                      BinaryOperatorKind Opc) {
-  if (!LHS->getType()->isPointerType())
+  if (!LHS->getType()->isPointerType() ||
+      S.getLangOpts().isSignedOverflowDefined())
     return std::nullopt;
 
   // Canonicalize to >= or < predicate.
@@ -11940,7 +11941,7 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
                                 << 1 /*array comparison*/
                                 << Result);
     } else if (std::optional<bool> Res =
-                   isTautologicalBoundsCheck(LHS, RHS, Opc)) {
+                   isTautologicalBoundsCheck(S, LHS, RHS, Opc)) {
       S.DiagRuntimeBehavior(Loc, nullptr,
                             S.PDiag(diag::warn_comparison_always)
                                 << 2 /*pointer comparison*/
diff --git a/clang/test/Sema/tautological-pointer-comparison.c b/clang/test/Sema/tautological-pointer-comparison.c
index 19cd20e5f7d21c..22734ecab6a2f3 100644
--- a/clang/test/Sema/tautological-pointer-comparison.c
+++ b/clang/test/Sema/tautological-pointer-comparison.c
@@ -1,40 +1,72 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -DFWRAPV -fwrapv -verify %s
+
+#ifdef FWRAPV
+// expected-no-diagnostics
+#endif
 
 int add_ptr_idx_ult_ptr(const char *ptr, unsigned index) {
-  return ptr + index < ptr; // expected-warning {{pointer comparison always evaluates to false}}
+#ifndef FWRAPV
+  // expected-warning@+2 {{pointer comparison always evaluates to false}}
+#endif
+  return ptr + index < ptr;
 }
 
 int add_idx_ptr_ult_ptr(const char *ptr, unsigned index) {
-  return index + ptr < ptr; // expected-warning {{pointer comparison always evaluates to false}}
+#ifndef FWRAPV
+  // expected-warning@+2 {{pointer comparison always evaluates to false}}
+#endif
+  return index + ptr < ptr;
 }
 
 int ptr_ugt_add_ptr_idx(const char *ptr, unsigned index) {
-  return ptr > ptr + index; // expected-warning {{pointer comparison always evaluates to false}}
+#ifndef FWRAPV
+  // expected-warning@+2 {{pointer comparison always evaluates to false}}
+#endif
+  return ptr > ptr + index;
 }
 
 int ptr_ugt_add_idx_ptr(const char *ptr, unsigned index) {
-  return ptr > index + ptr; // expected-warning {{pointer comparison always evaluates to false}}
+#ifndef FWRAPV
+  // expected-warning@+2 {{pointer comparison always evaluates to false}}
+#endif
+  return ptr > index + ptr;
 }
 
 int add_ptr_idx_uge_ptr(const char *ptr, unsigned index) {
-  return ptr + index >= ptr; // expected-warning {{pointer comparison always evaluates to true}}
+#ifndef FWRAPV
+  // expected-warning@+2 {{pointer comparison always evaluates to true}}
+#endif
+  return ptr + index >= ptr;
 }
 
 int add_idx_ptr_uge_ptr(const char *ptr, unsigned index) {
-  return index + ptr >= ptr; // expected-warning {{pointer comparison always evaluates to true}}
+#ifndef FWRAPV
+  // expected-warning@+2 {{pointer comparison always evaluates to true}}
+#endif
+  return index + ptr >= ptr;
 }
 
 int ptr_ule_add_ptr_idx(const char *ptr, unsigned index) {
-  return ptr <= ptr + index; // expected-warning {{pointer comparison always evaluates to true}}
+#ifndef FWRAPV
+  // expected-warning@+2 {{pointer comparison always evaluates to true}}
+#endif
+  return ptr <= ptr + index;
 }
 
 int ptr_ule_add_idx_ptr(const char *ptr, unsigned index) {
-  return ptr <= index + ptr; // expected-warning {{pointer comparison always evaluates to true}}
+#ifndef FWRAPV
+  // expected-warning@+2 {{pointer comparison always evaluates to true}}
+#endif
+  return ptr <= index + ptr;
 }
 
 int add_ptr_idx_ult_ptr_array(unsigned index) {
   char ptr[10];
-  return ptr + index < ptr; // expected-warning {{pointer comparison always evaluates to false}}
+#ifndef FWRAPV
+  // expected-warning@+2 {{pointer comparison always evaluates to false}}
+#endif
+  return ptr + index < ptr;
 }
 
 // Negative tests with wrong predicate.

clang/test/Sema/tautological-pointer-comparison.c

Signed-off-by: Nathan Chancellor <[email protected]>

-verify=expected is the same thing as -verify Signed-off-by: Nathan Chancellor <[email protected]>

nikic

LGTM

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 18, 2024

nathanchance requested review from nikic, cor3ntin and tbaederr December 18, 2024 21:11

nathanchance mentioned this pull request Dec 18, 2024

[Sema] Diagnose tautological bounds checks #120222

Merged

nikic reviewed Dec 18, 2024

View reviewed changes

clang/test/Sema/tautological-pointer-comparison.c Outdated Show resolved Hide resolved

nathanchance added 2 commits December 18, 2024 14:21

fixup! [Sema] Fix tautological bounds check warning with -fwrapv

8504a0d

Signed-off-by: Nathan Chancellor <[email protected]>

fixup! [Sema] Fix tautological bounds check warning with -fwrapv

6483ce9

-verify=expected is the same thing as -verify Signed-off-by: Nathan Chancellor <[email protected]>

nikic approved these changes Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sema] Fix tautological bounds check warning with -fwrapv #120480

[Sema] Fix tautological bounds check warning with -fwrapv #120480

nathanchance commented Dec 18, 2024

llvmbot commented Dec 18, 2024

nikic left a comment

[Sema] Fix tautological bounds check warning with -fwrapv #120480

Are you sure you want to change the base?

[Sema] Fix tautological bounds check warning with -fwrapv #120480

Conversation

nathanchance commented Dec 18, 2024

llvmbot commented Dec 18, 2024

nikic left a comment

Choose a reason for hiding this comment