[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

In D85778#2251173 , @rsmith wrote:

> In D85778#2251160 , @zequanwu wrote:
>
>> Hi, this change seems like hits a false positive case in chromium build: 
>> https://bugs.chromium.org/p/chromium/issues/detail?id=1124085
>
> That's not a false positive. The code is (simplified):
>
>   int RoundDown(int a, long b) { return a & -b; }
>
> ... which is implicitly converting an expression of type `long` to `int`, 
> losing precision. For example, `RoundDown(-1, 0x1'')` is -4294967296 
> prior to the implicit conversion from `long` to `int`. Given that the 
> truncation is presumably intentional (0 at least seems like the least-bad 
> answer for rounding down 0 to a multiple of 2^32 as a 32-bit integer), you 
> can suppress the warning with a cast.

Sorry, I didn't notice that the type of second parameter is based on `__LP64__` 
(didn't scroll up...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85778/new/

https://reviews.llvm.org/D85778

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D85778#2251160 , @zequanwu wrote:

> Hi, this change seems like hits a false positive case in chromium build: 
> https://bugs.chromium.org/p/chromium/issues/detail?id=1124085

That's not a false positive. The code is (simplified):

  int RoundDown(int a, long b) { return a & -b; }

... which is implicitly converting an expression of type `long` to `int`, 
losing precision. For example, `RoundDown(-1, 0x1'')` is -4294967296 
prior to the implicit conversion from `long` to `int`. Given that the 
truncation is presumably intentional (0 at least seems like the least-bad 
answer for rounding down 0 to a multiple of 2^32 as a 32-bit integer), you can 
suppress the warning with a cast.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85778/new/

https://reviews.llvm.org/D85778

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

Hi, this change hits a false positive case in chromium build: 
https://bugs.chromium.org/p/chromium/issues/detail?id=1124085


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85778/new/

https://reviews.llvm.org/D85778

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D85778#2248714 , @nikic wrote:

> Just for the record, the additional analysis has a measurable compile-time 
> impact (0.3% at O0): 
> https://llvm-compile-time-tracker.com/compare.php?from=e7f53044e7263cdbbb4fed9abf086b88ba486bba=cff6dda604cb0548bef5e5951dd1e74536110588=instructions

Thanks, it seems very likely that this is caused by `CheckImplicitConversion` 
now calling `CheckExprRange` twice. It looks like that's not only avoidable, 
but there's a (pre-existing) bug here that we can squash at the same time :) 
Should be fixed in rG0ffbbce78de60f4f4d03d6ef97fe2f3bb4275e08 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85778/new/

https://reviews.llvm.org/D85778

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Just for the record, the additional analysis has a measurable compile-time 
impact (0.3% at O0): 
https://llvm-compile-time-tracker.com/compare.php?from=e7f53044e7263cdbbb4fed9abf086b88ba486bba=cff6dda604cb0548bef5e5951dd1e74536110588=instructions


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85778/new/

https://reviews.llvm.org/D85778

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcff6dda604cb: More accurately compute the ranges of possible 
values for +, -, *, , %. (authored by rsmith).

Changed prior to commit:
  https://reviews.llvm.org/D85778?vs=284883=289079#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85778/new/

https://reviews.llvm.org/D85778

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/tautological-constant-compare.c

Index: clang/test/Sema/tautological-constant-compare.c
===
--- clang/test/Sema/tautological-constant-compare.c
+++ clang/test/Sema/tautological-constant-compare.c
@@ -4,12 +4,16 @@
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-type-limit-compare -DTEST -verify -x c++ %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtype-limits -DTEST -verify %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtype-limits -DTEST -verify -x c++ %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify -x c++ %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify -x c++ %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify=silent %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify=silent -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify=silent %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify=silent -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify=silent %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify=silent -x c++ %s
+
+#ifndef TEST
+// silent-no-diagnostics
+#endif
 
 int value(void);
 
@@ -184,7 +188,6 @@
   if (-32768L >= s)
   return 0;
 #else
-  // expected-no-diagnostics
   if (s == 32767)
 return 0;
   if (s != 32767)
@@ -373,7 +376,6 @@
   if (65535UL >= us) // expected-warning {{comparison 65535 >= 'unsigned short' is always true}}
   return 0;
 #else
-  // expected-no-diagnostics
   if (us == 65535)
   return 0;
   if (us != 65535)
@@ -571,20 +573,100 @@
 
   if ((s & 0xff) < 0) {} // #valuerange1
   if ((s & 0xff) < 1) {}
-  if ((s & -3) < -4) {} // #valuerange2
+  if ((s & -3) < -4) {}
   if ((s & -3) < -3) {}
-  if ((s & -3) < 4u) {} // (true if s non-negative)
-  if ((s & -3) > 4u) {} // (true if s negative)
-  if ((s & -3) == 4u) {} // #valuerange3 (never true)
-  if ((s & -3) == 3u) {}
-  if ((s & -3) == -5u) {} // #valuerange4
+  if ((s & -3) < 4u) {}
+  if ((s & -3) > 4u) {}
+  if ((s & -3) == 4u) {}
+  if ((s & -3) == 3u) {} // FIXME: Impossible.
+  if ((s & -3) == -5u) {}
   if ((s & -3) == -4u) {}
 #if TEST == 2
   // expected-warning@#valuerange1 {{comparison of 8-bit unsigned value < 0 is always false}}
-  // expected-warning@#valuerange2 {{comparison of 3-bit signed value < -4 is always false}}
-  // expected-warning@#valuerange3 {{comparison of 3-bit signed value == 4 is always false}}
-  // expected-warning@#valuerange4 {{comparison of 3-bit signed value == 4294967291 is always false}}
 #endif
 
+  // FIXME: Our bit-level width tracking comes unstuck here: the second of the
+  // conditions below is also tautological, but we can't tell that because we
+  // don't track the actual range, only the bit-width.
+  if ((s ? 1 : 0) + (us ? 1 : 0) > 1) {}
+  if ((s ? 1 : 0) + (us ? 1 : 0) > 2) {}
+  if ((s ? 1 : 0) + (us ? 1 : 0) > 3) {} // #addrange1
+#if TEST == 2
+  // expected-warning@#addrange1 {{comparison of 2-bit unsigned value > 3 is always false}}
+#endif
+
+  // FIXME: The second and third comparisons are also tautological; 0x4000
+  // is the greatest value that multiplying two int16s can produce.
+  if (s * s > 0x3fff) {}
+  if (s * s > 0x4000) {}
+  if (s * s > 0x7ffe) {}
+  if (s * s > 0x7fff) {} // expected-warning {{result of comparison 'int' > 2147483647 is always false}}
+
+  if ((s & 0x3ff) * (s & 0x1f) > 0x7be0) {}
+  if ((s & 0x3ff) * (s & 0x1f) > 0x7be1) {} // FIXME
+  if ((s & 0x3ff) * (s & 0x1f) > 0x7ffe) {} // FIXME
+  if ((s & 0x3ff) * (s & 0x1f) > 0x7fff) {} // #mulrange1
+#if TEST == 2
+  // expected-warning@#mulrange1 {{comparison of 15-bit unsigned value > 32767 is always false}}
+#endif
+
+  if (a.a * a.b > 21) {} // FIXME
+  if (a.a * a.b > 31) {} // #mulrange2
+#if TEST == 2
+  // expected-warning@#mulrange2 {{comparison of 6-bit signed value > 31 is always false}}
+#endif
+
+  if (a.a - (s & 1) < 

[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

[Richard Trieu via Phabricator via cfe-commits]

rtrieu accepted this revision.
rtrieu added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/lib/Sema/SemaChecking.cpp:10166
+  /// The number of bits active in the int. Note that this includes exactly one
+  /// sign bit if !NoNegative.
   unsigned Width;

NonNegative


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85778/new/

https://reviews.llvm.org/D85778

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

[Stephan Bergmann via Phabricator via cfe-commits]

sberg added a comment.

Fixes all the false positives it had reported for LibreOffice (which had all 
involved expressions containing either ~ or +).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85778/new/

https://reviews.llvm.org/D85778

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith created this revision.
rsmith added reviewers: sberg, rtrieu.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
rsmith requested review of this revision.

Continue to heuristically pick the wider of the two operands for
narrowing conversion warnings so that some_char + 1 isn't treated as
being wider than a char, but use the more accurate computation for
tautological comparison warnings.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85778

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/tautological-constant-compare.c

Index: clang/test/Sema/tautological-constant-compare.c
===
--- clang/test/Sema/tautological-constant-compare.c
+++ clang/test/Sema/tautological-constant-compare.c
@@ -4,12 +4,16 @@
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-type-limit-compare -DTEST -verify -x c++ %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtype-limits -DTEST -verify %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtype-limits -DTEST -verify -x c++ %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify -x c++ %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify -x c++ %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify=silent %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify=silent -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify=silent %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify=silent -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify=silent %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify=silent -x c++ %s
+
+#ifndef TEST
+// silent-no-diagnostics
+#endif
 
 int value(void);
 
@@ -184,7 +188,6 @@
   if (-32768L >= s)
   return 0;
 #else
-  // expected-no-diagnostics
   if (s == 32767)
 return 0;
   if (s != 32767)
@@ -373,7 +376,6 @@
   if (65535UL >= us) // expected-warning {{comparison 65535 >= 'unsigned short' is always true}}
   return 0;
 #else
-  // expected-no-diagnostics
   if (us == 65535)
   return 0;
   if (us != 65535)
@@ -571,19 +573,93 @@
 
   if ((s & 0xff) < 0) {} // #valuerange1
   if ((s & 0xff) < 1) {}
-  if ((s & -3) < -4) {} // #valuerange2
+  if ((s & -3) < -4) {}
   if ((s & -3) < -3) {}
-  if ((s & -3) < 4u) {} // (true if s non-negative)
-  if ((s & -3) > 4u) {} // (true if s negative)
-  if ((s & -3) == 4u) {} // #valuerange3 (never true)
-  if ((s & -3) == 3u) {}
-  if ((s & -3) == -5u) {} // #valuerange4
+  if ((s & -3) < 4u) {}
+  if ((s & -3) > 4u) {}
+  if ((s & -3) == 4u) {}
+  if ((s & -3) == 3u) {} // FIXME: Impossible.
+  if ((s & -3) == -5u) {}
   if ((s & -3) == -4u) {}
 #if TEST == 2
   // expected-warning@#valuerange1 {{comparison of 8-bit unsigned value < 0 is always false}}
-  // expected-warning@#valuerange2 {{comparison of 3-bit signed value < -4 is always false}}
-  // expected-warning@#valuerange3 {{comparison of 3-bit signed value == 4 is always false}}
-  // expected-warning@#valuerange4 {{comparison of 3-bit signed value == 4294967291 is always false}}
+#endif
+
+  // FIXME: Our bit-level width tracking comes unstuck here: the second of the
+  // conditions below is also tautological, but we can't tell that because we
+  // don't track the actual range, only the bit-width.
+  if ((s ? 1 : 0) + (us ? 1 : 0) > 1) {}
+  if ((s ? 1 : 0) + (us ? 1 : 0) > 2) {}
+  if ((s ? 1 : 0) + (us ? 1 : 0) > 3) {} // #addrange1
+#if TEST == 2
+  // expected-warning@#addrange1 {{comparison of 2-bit unsigned value > 3 is always false}}
+#endif
+
+  // FIXME: The second and third comparisons are also tautological; 0x4000
+  // is the greatest value that multiplying two int16s can produce.
+  if (s * s > 0x3fff) {}
+  if (s * s > 0x4000) {}
+  if (s * s > 0x7ffe) {}
+  if (s * s > 0x7fff) {} // expected-warning {{result of comparison 'int' > 2147483647 is always false}}
+
+  if ((s & 0x3ff) * (s & 0x1f) > 0x7be0) {}
+  if ((s & 0x3ff) * (s & 0x1f) > 0x7be1) {} // FIXME
+  if ((s & 0x3ff) * (s & 0x1f) > 0x7ffe) {} // FIXME
+  if ((s & 0x3ff) * (s & 0x1f) > 0x7fff) {} // #mulrange1
+#if TEST == 2
+  // expected-warning@#mulrange1 {{comparison of 15-bit unsigned value > 32767 is always false}}
+#endif
+
+  if (a.a * a.b > 21) {} // FIXME
+  if (a.a * a.b > 31) {} // #mulrange2
+#if TEST == 2
+  // expected-warning@#mulrange2 {{comparison of 6-bit signed value > 31 is always false}}
+#endif
+
+  if (a.a -