[Sema] -Wtautological-compare: handle comparison of unsigned with 0S.
ClosedPublic

Authored by lebedev.ri on Sep 7 2017, 5:48 AM.

Details

Summary

This is a first half(?) of a fix for the following bug:
https://bugs.llvm.org/show_bug.cgi?id=34147 (gcc -Wtype-limits)

GCC's -Wtype-limits does warn on comparison of unsigned value
with signed zero (as in, with 0), but clang only warns if the
zero is unsigned (i.e. 0U).

Also, be careful not to double-warn, or falsely warn on
comparison of signed/fp variable and signed 0.

Yes, all these testcases are needed.

Testing: $ ninja check-clang-sema check-clang-semacxx
Also, no new warnings for clang stage-2 build.

Diff Detail

Repository
rL LLVM
lebedev.ri created this revision.Sep 7 2017, 5:48 AM
lebedev.ri updated this revision to Diff 114172.Sep 7 2017, 6:43 AM

Added ReleaseNotes.rst note

rjmccall added inline comments.Sep 7 2017, 10:05 AM
lib/Sema/SemaChecking.cpp
8592 ↗(On Diff #114172)

Please extract a function which computes this for an Expr* and then call it as part of the conditions below, e.g.:

if (op == BO_LT && isNonBooleanUnsignedValue(E->getLHS()) && IsZero(S, E->getRHS()))
8879 ↗(On Diff #114172)

Part of the purpose of checking for signed comparisons up here is to avoid unnecessarily computing ranges for the operands when we aren't going to use them. That's still something we want to avoid. I think you just need to call CheckTrivialUnsignedComparison in this fallback case, at least when the comparison is not constant.

You should also rename CheckTrivialUnsignedComparison to something like CheckTautologicalComparison.

lebedev.ri marked an inline comment as done.

Address review notes.

lib/Sema/SemaChecking.cpp
8592 ↗(On Diff #114172)

Yes, that is indeed a better way :)

8879 ↗(On Diff #114172)

Ok, i must admit that i understand the idea to have minimal overhead. But i don't really follow the code in here :)
This seems to work, and check-clang-sema+check-clang-semacxx+stage2 still pass.
Is this obviously-wrong / some testcase is missing?

rjmccall added inline comments.Sep 7 2017, 11:44 AM
lib/Sema/SemaChecking.cpp
8589 ↗(On Diff #114221)

Do you still need these? I'm always antsy about code that ignores implicit casts, especially before evaluation.

8879 ↗(On Diff #114172)

There's a lot of code here doing very similar checks, and we just want to ensure that we aren't doing too much redundant work. I think the way you've structured it now is fine.

lebedev.ri added inline comments.Sep 7 2017, 11:49 AM
lib/Sema/SemaChecking.cpp
8589 ↗(On Diff #114221)

Yes, absolutely. I did check without them, and they are needed. Else:

$ ninja check-clang-sema check-clang-semacxx
[27/29] Running lit suite /build/clang/test/Sema
lit.py: /build/clang/test/lit.cfg:200: note: using clang: '/build/llvm-build-Clang-release/./bin/clang'
FAIL: Clang :: Sema/compare.c (195 of 548)
******************** TEST 'Clang :: Sema/compare.c' FAILED ********************
Script:
--
/build/llvm-build-Clang-release/./bin/clang -cc1 -internal-isystem /build/llvm-build-Clang-release/lib/clang/6.0.0/include -nostdsysteminc -triple x86_64-apple-darwin -fsyntax-only -pedantic -verify -Wsign-compare /build/clang/test/Sema/compare.c -Wno-unreachable-code
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics seen but not expected: 
  File /build/clang/test/Sema/compare.c Line 80: comparison of unsigned expression < 0 is always false
  File /build/clang/test/Sema/compare.c Line 88: comparison of unsigned expression < 0 is always false
  File /build/clang/test/Sema/compare.c Line 89: comparison of unsigned expression < 0 is always false
3 errors generated.

--

********************
FAIL: Clang :: Sema/outof-range-constant-compare.c (357 of 548)
******************** TEST 'Clang :: Sema/outof-range-constant-compare.c' FAILED ********************
Script:
--
/build/llvm-build-Clang-release/./bin/clang -cc1 -internal-isystem /build/llvm-build-Clang-release/lib/clang/6.0.0/include -nostdsysteminc -triple x86_64-apple-darwin -fsyntax-only -Wtautological-constant-out-of-range-compare -verify /build/clang/test/Sema/outof-range-constant-compare.c
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics expected but not seen: 
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 163: comparison of unsigned expression < 0 is always false
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 169: comparison of unsigned expression >= 0 is always true
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 178: comparison of 0 <= unsigned expression is always true
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 180: comparison of 0 > unsigned expression is always false
error: 'warning' diagnostics seen but not expected: 
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 40: comparison of unsigned expression < 0 is always false
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 46: comparison of unsigned expression >= 0 is always true
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 55: comparison of 0 <= unsigned expression is always true
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 57: comparison of 0 > unsigned expression is always false
8 errors generated.

--

********************
Testing Time: 2.05s
********************
Failing Tests (2):
    Clang :: Sema/compare.c
    Clang :: Sema/outof-range-constant-compare.c

  Expected Passes    : 546
  Unexpected Failures: 2
FAILED: tools/clang/test/CMakeFiles/check-clang-sema 
cd /build/llvm-build-Clang-release/tools/clang/test && /usr/bin/python2.7 /build/llvm/utils/lit/lit.py -sv --param clang_site_config=/build/llvm-build-Clang-release/tools/clang/test/lit.site.cfg /build/clang/test/Sema
ninja: build stopped: subcommand failed.
8879 ↗(On Diff #114172)

OK :)

rjmccall added inline comments.Sep 7 2017, 12:02 PM
lib/Sema/SemaChecking.cpp
8589 ↗(On Diff #114221)

What happens if you just move them into isNonBooleanUnsignedValue?

test/Sema/outof-range-constant-compare.c
41 ↗(On Diff #114221)

Hmm. I think this should probably still warn under -Wtautological-compare, shouldn't it? The fact that it also warns under -Wsign-compare is something we should try to suppress, but it's definitely still a tautological comparison because of the promotion to an unsigned type.

lebedev.ri planned changes to this revision.Sep 7 2017, 12:11 PM
lebedev.ri added inline comments.
test/Sema/outof-range-constant-compare.c
41 ↗(On Diff #114221)

Hmm, actually, i guess so?

int value(void);
int main()
{
    int a = value();
    if (a <= 0x0000000000000000UL)
        return 0;	
}
|-BinaryOperator <line:5:9, col:14> '_Bool' '<='
| |-ImplicitCastExpr <col:9> 'unsigned long' <IntegralCast>
| | `-ImplicitCastExpr <col:9> 'int' <LValueToRValue>
| |   `-DeclRefExpr <col:9> 'int' lvalue Var 0x7028450 'a' 'int'
| `-IntegerLiteral <col:14> 'unsigned long' 0

Here, clang and gcc both only warn about different sign comparison.

lebedev.ri updated this revision to Diff 114249.Sep 7 2017, 1:40 PM
lebedev.ri marked 4 inline comments as done.

Address review notes.

Great, thanks. Just a few tweaks.

docs/ReleaseNotes.rst
76 ↗(On Diff #114249)

"now warns when comparing an unsigned integer and 0 regardless of whether the constant is signed or unsigned."

79 ↗(On Diff #114249)

Probably better to be clear here: "now warns about comparing a signed integer and 0 when the signed integer is coerced to an unsigned type for the comparison."

83 ↗(On Diff #114249)

I'm not sure this level of detail is necessary in release notes.

lib/Sema/SemaChecking.cpp
8879 ↗(On Diff #114249)

Space after 'if', and I would suggest for the comment:

"If this is a tautological comparison, suppress -Wsign-compare."
lebedev.ri updated this revision to Diff 114260.Sep 7 2017, 2:42 PM
lebedev.ri marked 3 inline comments as done.

Address review notes.

rjmccall accepted this revision.Sep 7 2017, 3:08 PM

LGTM.

docs/ReleaseNotes.rst
79 ↗(On Diff #114249)

That's great, thanks.

This revision is now accepted and ready to land.Sep 7 2017, 3:08 PM
This revision was automatically updated to reflect the committed changes.