This is an archive of the discontinued LLVM Phabricator instance.

Differential D155457

[clang] Skip tautological comparison if the comparison involves the 'size_t' type
AbandonedPublic

Authored by xgupta on Jul 17 2023, 6:30 AM.

Details

Reviewers
aaron.ballman
xbolva00
Summary

The issue with size_t comes when we are trying to addd
-Wtype-limits to -Wextra for GCC compatibility in review
https://reviews.llvm.org/D142826.

Diff Detail

Event Timeline

xgupta created this revision.Jul 17 2023, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2023, 6:30 AM
xgupta requested review of this revision.Jul 17 2023, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2023, 6:30 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
xgupta added a comment.Jul 17 2023, 6:31 AM

I am not sure if this is correct but this passes ninja check-clang.

xbolva00 added a comment.Jul 17 2023, 8:37 AM

You should add a testcase which uses “expected no diagnostics”.

Harbormaster completed remote builds in B245822: Diff 540991.Jul 17 2023, 8:56 AM
xgupta added a comment.Jul 17 2023, 10:52 AM
In D155457#4506405, @xbolva00 wrote:

You should add a testcase which uses “expected no diagnostics”.

There is some issue writing test

$ cat type-limit-compare.cpp
// RUN: %clang_cc1  -fsyntax-only -Wtautological-type-limit-compare -verify %s
// expected-no-diagnostics

#include <cstddef>
#include <cstdint>
#include <limits>

bool foo(uint64_t Size) {
  if (sizeof(std::size_t) < sizeof(uint64_t) &&
     Size > static_cast<uint64_t>(std::numeric_limits<std::size_t>::max())) // no-warning
    return false;
  return true;
}

failed with

$ llvm-project/clang/test/Sema/type-limit-compare.cpp:4:10: fatal error: 'cstddef' file not found
    4 | #include <cstddef>
      |          ^~~~~~~~~
aaron.ballman added a comment.Jul 17 2023, 10:55 AM
In D155457#4507107, @xgupta wrote:
In D155457#4506405, @xbolva00 wrote:

You should add a testcase which uses “expected no diagnostics”.

There is some issue writing test

$ cat type-limit-compare.cpp
// RUN: %clang_cc1  -fsyntax-only -Wtautological-type-limit-compare -verify %s
// expected-no-diagnostics

#include <cstddef>
#include <cstdint>
#include <limits>

bool foo(uint64_t Size) {
  if (sizeof(std::size_t) < sizeof(uint64_t) &&
     Size > static_cast<uint64_t>(std::numeric_limits<std::size_t>::max())) // no-warning
    return false;
  return true;
}

failed with

$ llvm-project/clang/test/Sema/type-limit-compare.cpp:4:10: fatal error: 'cstddef' file not found
    4 | #include <cstddef>
      |          ^~~~~~~~~

We typically do not include any system headers (STL or otherwise) as part of the compiler tests; that would test whatever is found on the test system instead of a consistent test. Instead, I'd recommend doing:

namespace std {
using size_t = decltype(sizeof(0));
}

Similarly, you can replace uint64_t with unsigned long long and the numeric_limits call with __SIZE_MAX__

xgupta added a comment.Jul 18 2023, 5:45 AM
In D155457#4507124, @aaron.ballman wrote:
In D155457#4507107, @xgupta wrote:
In D155457#4506405, @xbolva00 wrote:

You should add a testcase which uses “expected no diagnostics”.

There is some issue writing test

$ cat type-limit-compare.cpp
// RUN: %clang_cc1  -fsyntax-only -Wtautological-type-limit-compare -verify %s
// expected-no-diagnostics

#include <cstddef>
#include <cstdint>
#include <limits>

bool foo(uint64_t Size) {
  if (sizeof(std::size_t) < sizeof(uint64_t) &&
     Size > static_cast<uint64_t>(std::numeric_limits<std::size_t>::max())) // no-warning
    return false;
  return true;
}

failed with

$ llvm-project/clang/test/Sema/type-limit-compare.cpp:4:10: fatal error: 'cstddef' file not found
    4 | #include <cstddef>
      |          ^~~~~~~~~

We typically do not include any system headers (STL or otherwise) as part of the compiler tests; that would test whatever is found on the test system instead of a consistent test. Instead, I'd recommend doing:

namespace std {
using size_t = decltype(sizeof(0));
}

Similarly, you can replace uint64_t with unsigned long long and the numeric_limits call with __SIZE_MAX__

I see, Thanks, but there is another thing, writing this way compiler emits a warning as the check to exclude the warning is based on size_t so the test case is not passed.

// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify

// expected-no-diagnostics

typedef unsigned long long uint64_t;
namespace std {
using size_t = decltype(sizeof(0));
} // namespace std

bool func(uint64_t Size) {
  if (sizeof(std::size_t) < sizeof(uint64_t) &&
     Size > (uint64_t)(__SIZE_MAX__))
    return false;
  return true;
}
aaron.ballman added a comment.Jul 18 2023, 9:56 AM
In D155457#4510202, @xgupta wrote:
In D155457#4507124, @aaron.ballman wrote:
In D155457#4507107, @xgupta wrote:
In D155457#4506405, @xbolva00 wrote:

You should add a testcase which uses “expected no diagnostics”.

There is some issue writing test

$ cat type-limit-compare.cpp
// RUN: %clang_cc1  -fsyntax-only -Wtautological-type-limit-compare -verify %s
// expected-no-diagnostics

#include <cstddef>
#include <cstdint>
#include <limits>

bool foo(uint64_t Size) {
  if (sizeof(std::size_t) < sizeof(uint64_t) &&
     Size > static_cast<uint64_t>(std::numeric_limits<std::size_t>::max())) // no-warning
    return false;
  return true;
}

failed with

$ llvm-project/clang/test/Sema/type-limit-compare.cpp:4:10: fatal error: 'cstddef' file not found
    4 | #include <cstddef>
      |          ^~~~~~~~~

We typically do not include any system headers (STL or otherwise) as part of the compiler tests; that would test whatever is found on the test system instead of a consistent test. Instead, I'd recommend doing:

namespace std {
using size_t = decltype(sizeof(0));
}

Similarly, you can replace uint64_t with unsigned long long and the numeric_limits call with __SIZE_MAX__

I see, Thanks, but there is another thing, writing this way compiler emits a warning as the check to exclude the warning is based on size_t so the test case is not passed.

Whether size_t comes from the system header or whether it's manually deduced from decltype(sizeof(0)) should make no difference as far as the frontend is concerned; they should resolve to the same type. Can you explain the test failure you're seeing in a bit more detail?

xgupta added a comment.EditedJul 18 2023, 10:10 AM
In D155457#4511392, @aaron.ballman wrote:

Whether size_t comes from the system header or whether it's manually deduced from decltype(sizeof(0)) should make no difference as far as the frontend is concerned; they should resolve to the same type. Can you explain the test failure you're seeing in a bit more detail?

: 'RUN: at line 1';   /home/shivam/.llvm/llvm-project/build/bin/clang -cc1 -internal-isystem /home/shivam/.llvm/llvm-project/build/lib/clang/17/include -nostdsysteminc /home/shivam/.llvm/llvm-project/clang/test/Sema/type-limit-compare.cpp -fsyntax-only -Wtautological-type-limit-compare -verify
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics seen but not expected: 
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/type-limit-compare.cpp Line 12: result of comparison 'uint64_t' (aka 'unsigned long long') > 18446744073709551615 is always false
1 error generated.

I think the issue is with (uint64_t)(____SIZE_MAX____) vs (uint64_t)std::numeric_limits<std::size_t>::max() but not sure.

aaron.ballman added a comment.Jul 18 2023, 10:47 AM
In D155457#4511446, @xgupta wrote:
In D155457#4511392, @aaron.ballman wrote:

Whether size_t comes from the system header or whether it's manually deduced from decltype(sizeof(0)) should make no difference as far as the frontend is concerned; they should resolve to the same type. Can you explain the test failure you're seeing in a bit more detail?

: 'RUN: at line 1';   /home/shivam/.llvm/llvm-project/build/bin/clang -cc1 -internal-isystem /home/shivam/.llvm/llvm-project/build/lib/clang/17/include -nostdsysteminc /home/shivam/.llvm/llvm-project/clang/test/Sema/type-limit-compare.cpp -fsyntax-only -Wtautological-type-limit-compare -verify
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics seen but not expected: 
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/type-limit-compare.cpp Line 12: result of comparison 'uint64_t' (aka 'unsigned long long') > 18446744073709551615 is always false
1 error generated.

I think the issue is with (uint64_t)(____SIZE_MAX____) vs (uint64_t)std::numeric_limits<std::size_t>::max() but not sure.

Something seems odd to me about the current behavior. Consider: https://godbolt.org/z/ofezfhaPd

In the x86 compilation: sizeof(std::size_t) < sizeof(uint64_t) is true, so we test the other expression; because Size is greater than __SIZE_MAX__ the function returns false and the second static assertion fails as expected.
In the x64 compilation: sizeof(std::size_t) < sizeof(uint64_t) is false (first static assertion fails) so we shouldn't even be evaluating the RHS of the && to see if it's tautological because it can't contribute to the expression result, right?

xgupta updated this revision to Diff 541866.Jul 19 2023, 12:41 AM

Add test case

xgupta added a comment.Jul 19 2023, 1:35 AM
In D155457#4511652, @aaron.ballman wrote:

In the x86 compilation: sizeof(std::size_t) < sizeof(uint64_t) is true, so we test the other expression; because Size is greater than __SIZE_MAX__ the function returns false and the second static assertion fails as expected.
In the x64 compilation: sizeof(std::size_t) < sizeof(uint64_t) is false (first static assertion fails) so we shouldn't even be evaluating the RHS of the && to see if it's tautological because it can't contribute to the expression result, right?

Yes, I agree with both statements but what is odd in current behavior?

It seems to me uint64_t is unsigned long in <cstdint>, not unsigned long long so the new test added in this patch (with typedef unsigned long uint64_t) is now passing.

Behavior when the patch is applied-

$ ./bin/clang++ -Wall -fsyntax-only test.cpp -m64  -Wtype-limits 
test.cpp:8:11: warning: result of comparison 'uint64_t' (aka 'unsigned long long') > 18446744073709551615 is always false [-Wtautological-type-limit-compare]
    8 |      Size > static_cast<uint64_t>(__SIZE_MAX__)) // no-warning
      |      ~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test.cpp:13:15: error: static assertion failed due to requirement 'sizeof(unsigned long) < sizeof(unsigned long long)': 
   13 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test.cpp:13:35: note: expression evaluates to '8 < 8'
   13 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
      |               ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
1 warning and 1 error generated.
$ ./bin/clang++ -Wall -fsyntax-only test1.cpp -m64  -Wtype-limits 
test1.cpp:12:15: error: static assertion failed due to requirement 'sizeof(unsigned long) < sizeof(unsigned long)': 
   12 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test1.cpp:12:35: note: expression evaluates to '8 < 8'
   12 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
      |               ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
1 error generated.

where test.cpp using wrapper and test1.cpp is using standard headers.

Harbormaster completed remote builds in B246449: Diff 541866.Jul 19 2023, 4:54 AM
xgupta updated this revision to Diff 542846.Jul 21 2023, 3:10 AM

update test for Windows system

Harbormaster completed remote builds in B247161: Diff 542846.Jul 21 2023, 7:25 AM
xgupta added a comment.EditedJul 21 2023, 8:27 AM

Just normal ping, @aaron.ballman, can you please take a look, pre-merge checks are also passing now.

aaron.ballman added a comment.Jul 21 2023, 10:19 AM
In D155457#4513812, @xgupta wrote:
In D155457#4511652, @aaron.ballman wrote:

In the x86 compilation: sizeof(std::size_t) < sizeof(uint64_t) is true, so we test the other expression; because Size is greater than __SIZE_MAX__ the function returns false and the second static assertion fails as expected.
In the x64 compilation: sizeof(std::size_t) < sizeof(uint64_t) is false (first static assertion fails) so we shouldn't even be evaluating the RHS of the && to see if it's tautological because it can't contribute to the expression result, right?

Yes, I agree with both statements but what is odd in current behavior?

It's a false positive -- the tautological bit is diagnosed but it doesn't contribute to the result of the expression. The first part of the predicate is specifically intended to ensure the second part of the predicate is *not* tautological.

It seems to me uint64_t is unsigned long in <cstdint>, not unsigned long long so the new test added in this patch (with typedef unsigned long uint64_t) is now passing.

Behavior when the patch is applied-

$ ./bin/clang++ -Wall -fsyntax-only test.cpp -m64  -Wtype-limits 
test.cpp:8:11: warning: result of comparison 'uint64_t' (aka 'unsigned long long') > 18446744073709551615 is always false [-Wtautological-type-limit-compare]
    8 |      Size > static_cast<uint64_t>(__SIZE_MAX__)) // no-warning
      |      ~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test.cpp:13:15: error: static assertion failed due to requirement 'sizeof(unsigned long) < sizeof(unsigned long long)': 
   13 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test.cpp:13:35: note: expression evaluates to '8 < 8'
   13 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
      |               ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
1 warning and 1 error generated.
$ ./bin/clang++ -Wall -fsyntax-only test1.cpp -m64  -Wtype-limits 
test1.cpp:12:15: error: static assertion failed due to requirement 'sizeof(unsigned long) < sizeof(unsigned long)': 
   12 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test1.cpp:12:35: note: expression evaluates to '8 < 8'
   12 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
      |               ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
1 error generated.

where test.cpp using wrapper and test1.cpp is using standard headers.

cor3ntin added a subscriber: cor3ntin.Jul 21 2023, 11:10 AM

I'm not sure I understand the motivation for this change. Sure, people do that but they also might do the same thing for ssize_t, intmax_t, or to compare int to int32_t.
I think a better heuristic would be to not emit a warning for any integral (and floating point?) type that have the same canonical types (but we probably still want one if their non-canonical type if the same)

xgupta added a comment.Jul 23 2023, 11:46 AM
In D155457#4523227, @aaron.ballman wrote:
In D155457#4513812, @xgupta wrote:
In D155457#4511652, @aaron.ballman wrote:

In the x86 compilation: sizeof(std::size_t) < sizeof(uint64_t) is true, so we test the other expression; because Size is greater than __SIZE_MAX__ the function returns false and the second static assertion fails as expected.
In the x64 compilation: sizeof(std::size_t) < sizeof(uint64_t) is false (first static assertion fails) so we shouldn't even be evaluating the RHS of the && to see if it's tautological because it can't contribute to the expression result, right?

Yes, I agree with both statements but what is odd in current behavior?

It's a false positive -- the tautological bit is diagnosed but it doesn't contribute to the result of the expression. The first part of the predicate is specifically intended to ensure the second part of the predicate is *not* tautological.

Ok, Is there any modification required in this patch or it fixes that false positive?

xgupta added a comment.Jul 23 2023, 11:48 AM
In D155457#4523388, @cor3ntin wrote:

I'm not sure I understand the motivation for this change. Sure, people do that but they also might do the same thing for ssize_t, intmax_t, or to compare int to int32_t.
I think a better heuristic would be to not emit a warning for any integral (and floating point?) type that have the same canonical types (but we probably still want one if their non-canonical type if the same)

I am not sure but are you expecting these changes -

// Don't warn if the comparison involves integral or floating-point types with the same canonical types.
QualType LHSCanonical = Constant->getType().getCanonicalType();
QualType RHSCanonical = Other->getType().getCanonicalType();
if ((LHSCanonical->isIntegralOrEnumerationType() || LHSCanonical->isFloatingType()) &&
    S.Context.hasSameType(LHSCanonical, RHSCanonical)) {
  return false;
}

This will silence a lot of warnings and a total 5 test case fails.

xgupta added a comment.Jul 26 2023, 5:41 AM

ping

aaron.ballman added a comment.Jul 26 2023, 10:16 AM
In D155457#4526168, @xgupta wrote:
In D155457#4523388, @cor3ntin wrote:

I'm not sure I understand the motivation for this change. Sure, people do that but they also might do the same thing for ssize_t, intmax_t, or to compare int to int32_t.
I think a better heuristic would be to not emit a warning for any integral (and floating point?) type that have the same canonical types (but we probably still want one if their non-canonical type if the same)

I am not sure but are you expecting these changes -

// Don't warn if the comparison involves integral or floating-point types with the same canonical types.
QualType LHSCanonical = Constant->getType().getCanonicalType();
QualType RHSCanonical = Other->getType().getCanonicalType();
if ((LHSCanonical->isIntegralOrEnumerationType() || LHSCanonical->isFloatingType()) &&
    S.Context.hasSameType(LHSCanonical, RHSCanonical)) {
  return false;
}

This will silence a lot of warnings and a total 5 test case fails.

Can you share some examples of what test cases start failing with that approach? What you have above matches what I think @cor3ntin was asking for and does seem like a pretty reasonable way to silence false positives.

xgupta updated this revision to Diff 544622.Jul 26 2023, 11:40 PM

Update as per comment of @cor3ntin

xgupta added a comment.Jul 26 2023, 11:41 PM
In D155457#4535941, @aaron.ballman wrote:
In D155457#4526168, @xgupta wrote:
In D155457#4523388, @cor3ntin wrote:

I'm not sure I understand the motivation for this change. Sure, people do that but they also might do the same thing for ssize_t, intmax_t, or to compare int to int32_t.
I think a better heuristic would be to not emit a warning for any integral (and floating point?) type that have the same canonical types (but we probably still want one if their non-canonical type if the same)

I am not sure but are you expecting these changes -

// Don't warn if the comparison involves integral or floating-point types with the same canonical types.
QualType LHSCanonical = Constant->getType().getCanonicalType();
QualType RHSCanonical = Other->getType().getCanonicalType();
if ((LHSCanonical->isIntegralOrEnumerationType() || LHSCanonical->isFloatingType()) &&
    S.Context.hasSameType(LHSCanonical, RHSCanonical)) {
  return false;
}

This will silence a lot of warnings and a total 5 test case fails.

Can you share some examples of what test cases start failing with that approach? What you have above matches what I think @cor3ntin was asking for and does seem like a pretty reasonable way to silence false positives.

Sure, updated three test cases in the patch and list the other two here -

FAIL: Clang :: Sema/tautological-constant-compare.c (865 of 18988)
******************** TEST 'Clang :: Sema/tautological-constant-compare.c' FAILED ********************

error: 'warning' diagnostics expected but not seen: 
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 560 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:568): comparison of 3-bit signed value < 4 is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 574 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:585): comparison of 8-bit unsigned value < 0 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 593 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:595): comparison of 2-bit unsigned value > 3 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 603: result of comparison 'int' > 2147483647 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 608 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:610): comparison of 15-bit unsigned value > 32767 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 614 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:616): comparison of 6-bit signed value > 31 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 621 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:637): comparison of 4-bit signed value < -8 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 623 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:638): comparison of 4-bit signed value > 7 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 628 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:639): comparison of 5-bit signed value < -16 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 629 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:640): comparison of 5-bit signed value > 15 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 632 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:641): comparison of 4-bit signed value > 7 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 633 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:642): comparison of 4-bit signed value < -8 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 635 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:643): comparison of 5-bit signed value < -16 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 648 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:659): comparison of 3-bit signed value > 3 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 652 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:660): comparison of 3-bit signed value < -4 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 656 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:661): comparison of 2-bit unsigned value > 3 is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c Line 657 (directive at /home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:662): comparison of 2-bit unsigned value < 0 is always false
17 errors generated.
FAIL: Clang :: Sema/bool-compare.c (10880 of 18988)
******************** TEST 'Clang :: Sema/bool-compare.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/shivam/.llvm/llvm-project/build/bin/clang -cc1 -internal-isystem /home/shivam/.llvm/llvm-project/build/lib/clang/17/include -nostdsysteminc -fsyntax-only -verify /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c -Wno-logical-not-parentheses
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics expected but not seen: 
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 8: comparison of constant 1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 33: comparison of constant 1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 34: comparison of constant 2 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 37: comparison of constant -1 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 39: comparison of constant 0 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 41: comparison of constant 2 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 44: comparison of constant -1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 46: comparison of constant 0 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 48: comparison of constant 2 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 51: comparison of constant -1 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 54: comparison of constant 1 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 55: comparison of constant 2 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 58: comparison of constant -1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 61: comparison of constant 1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 62: comparison of constant 4 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 63: comparison of constant -1 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 66: comparison of constant 1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 67: comparison of constant 4 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 70: comparison of constant 1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 71: comparison of constant 4 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 73: comparison of constant -1 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 77: comparison of constant 2 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 79: comparison of constant -1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 83: comparison of constant 2 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 85: comparison of constant -1 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 97: comparison of constant 0 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 99: comparison of constant 2 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 101: comparison of constant -1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 104: comparison of constant 1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 105: comparison of constant 2 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 107: comparison of constant -1 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 110: comparison of constant 1 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 111: comparison of constant 2 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 113: comparison of constant -1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 115: comparison of constant 0 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 117: comparison of constant 2 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 119: comparison of constant -1 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 121: comparison of constant 0 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 123: comparison of constant 4 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 125: comparison of constant 0 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 127: comparison of constant 4 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 129: comparison of constant 0 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 131: comparison of constant 4 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 133: comparison of constant -1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 137: comparison of constant 2 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 139: comparison of constant -1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 143: comparison of constant 2 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 145: comparison of constant -1 with boolean expression is always true
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 158: comparison of constant -1 with boolean expression is always false
  File /home/shivam/.llvm/llvm-project/clang/test/Sema/bool-compare.c Line 159: comparison of constant 2 with boolean expression is always true
Harbormaster completed remote builds in B248448: Diff 544622.Jul 27 2023, 12:40 AM
cor3ntin added inline comments.Jul 27 2023, 1:26 AM
clang/lib/Sema/SemaChecking.cpp
13366–13374

This is what I had in mind. I haven't tested it though

xgupta updated this revision to Diff 544728.Jul 27 2023, 5:49 AM

updated code as per the comment of @cor3ntin

xgupta updated this revision to Diff 544732.Jul 27 2023, 5:59 AM

minor update

Harbormaster completed remote builds in B248535: Diff 544732.Jul 27 2023, 6:52 AM
xgupta abandoned this revision.Mon, Dec 4, 11:44 PM

Moved to GitHub - https://github.com/llvm/llvm-project/pull/74427.

Revision Contents

PathSize
clang/
lib/
Sema/
7 lines

Diff 540991

clang/lib/Sema/SemaChecking.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 13,357 Lines • ▼ Show 20 Linesstatic bool CheckTautologicalComparison(Sema &S, BinaryOperator *E,
// //
// some_long_value <= INT_MAX // some_long_value <= INT_MAX
// //
// when sizeof(int) == sizeof(long). // when sizeof(int) == sizeof(long).
bool InRange = Cmp & PromotedRange::InRangeFlag; bool InRange = Cmp & PromotedRange::InRangeFlag;
if (InRange && IsEnumConstOrFromMacro(S, Constant)) if (InRange && IsEnumConstOrFromMacro(S, Constant))
return false; return false;
// Don't warn if the comparison involves the 'size_t' type.
QualType SizeT = S.Context.getSizeType();
if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
return false;
}
// A comparison of an unsigned bit-field against 0 is really a type problem, // A comparison of an unsigned bit-field against 0 is really a type problem,
// even though at the type level the bit-field might promote to 'signed int'. // even though at the type level the bit-field might promote to 'signed int'.
cor3ntinUnsubmitted
Not Done
ReplyInline Actions
return false;
- // Don't warn if the comparison involves integral or floating-point types with
+ // Don't warn if the comparison involves integral or floating-point types with
// the same canonical types.
QualType LHSCanonical = Constant->getType().getCanonicalType();
QualType RHSCanonical = Other->getType().getCanonicalType();
- if ((LHSCanonical->isIntegralOrEnumerationType() ||
+ if (TautologicalTypeCompare && (LHSCanonical->isIntegralOrEnumerationType() ||
LHSCanonical->isFloatingType()) &&
- S.Context.hasSameType(LHSCanonical, RHSCanonical)) {
+ S.Context.hasSameType(LHSCanonical, RHSCanonical) && !S.Context.hasSameType(Constant->getType(), Other->getType())) {
return false;
}
// A comparison of an unsigned bit-field against 0 is really a type problem,

This is what I had in mind. I haven't tested it though

cor3ntin: This is what I had in mind. I haven't tested it though
if (Other->refersToBitField() && InRange && Value == 0 && if (Other->refersToBitField() && InRange && Value == 0 &&
Other->getType()->isUnsignedIntegerOrEnumerationType()) Other->getType()->isUnsignedIntegerOrEnumerationType())
TautologicalTypeCompare = true; TautologicalTypeCompare = true;
// If this is a comparison to an enum constant, include that // If this is a comparison to an enum constant, include that
// constant in the diagnostic. // constant in the diagnostic.
const EnumConstantDecl *ED = nullptr; const EnumConstantDecl *ED = nullptr;
if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Constant)) if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Constant))
▲ Show 20 Lines • Show All 5,349 Lines • Show Last 20 Lines