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

[Roman Lebedev via cfe-commits]

On Sat, Sep 9, 2017 at 12:56 AM, Aaron Ballman  wrote:
> On Fri, Sep 8, 2017 at 5:49 PM, Hans Wennborg via cfe-commits
>  wrote:
>> On Fri, Sep 8, 2017 at 2:26 PM, Friedman, Eli  
>> wrote:
>>> On 9/8/2017 2:18 PM, Hans Wennborg via cfe-commits wrote:

 On Fri, Sep 8, 2017 at 2:09 PM, Roman Lebedev 
 wrote:
>
>
> Interesting. My first thought was to explicitly specify enum as signed:
>
> enum MediaDeviceType : signed int {
> MEDIA_DEVICE_TYPE_AUDIO_INPUT = 0,
> MEDIA_DEVICE_TYPE_VIDEO_INPUT,
> MEDIA_DEVICE_TYPE_AUDIO_OUTPUT,
> NUM_MEDIA_DEVICE_TYPES,
> };
>
> inline bool IsValidMediaDeviceType(MediaDeviceType type) {
> return type >= 0U && type < NUM_MEDIA_DEVICE_TYPES;
> }
>
> But it still seem to warn, and *that* sounds like a bug.
> Please open a new bugreport.

 I'm reporting it here :-)

> As for different default signedness, i'm not sure what is there to do.
> Does
> not sound like a problem for this diagnostic to intentionally avoid to
> be honest.

 I think it is a problem for this warning. If a user sees the warning
 and removes the "type >= 0" check, thinking that it was unnecessary
 because the compiler told them so, they have now introduced a bug in
 the code.
>>>
>>>
>>> Even if you declare the enum type as signed, it still isn't allowed to
>>> contain a negative number; that's undefined behavior.  You can check for
>>> this using ubsan's -fsanitize=enum.
>>
>> Is it? I thought if you define it as signed, it falls under "For an
>> enumeration whose underlying type is fixed, the values of the
>> enumeration are the values of the underlying type." (c++11 7.2p7)
>>
>> My standards-fu is weak though, so I could be wrong.
>
> The enum values can be signed because of the fixed type, but the
> comparison was enum_val <= 0U, so I believe the usual arithmetic
> conversions will convert the enum value to an *unsigned* value, making
> this a tautological comparison. If the value was 0, then I think the
> comparison would not be tautological because there would be no
> conversion needed.

Once i re-committed D37629, test-clang-msc-x64-on-i686-linux-RA build
broke with that exact problem - the expected diagnostic about enum
was not emitted. So i fixed it up in r313747, so now such diagnostic
about tautological comparison of 0 and enum is properly emitted
regardless of the sign of the enum type.

>> But I think you're right about the case when the underlying type is
>> not specified. Even if it happens to be 'int' on Windows, negative
>> values aren't allowed (unless there are negative enumerators). That
>> doesn't mean it won't happen though, and lots of code isn't UB-clean
>> :-/
>
> Yes, but if the *optimizer* were ever clever enough to realize the
> value cannot be negative, then your code still has a pretty serious
> problem in the face of that same UB.
>
> ~Aaron

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

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

[Roman Lebedev via cfe-commits]

On Fri, Sep 8, 2017 at 7:10 PM, Roman Lebedev  wrote:
> On Fri, Sep 8, 2017 at 3:26 PM, Roman Lebedev  wrote:
>> On Fri, Sep 8, 2017 at 2:48 PM, Sam McCall  wrote:
>> Hi.
>>
>>> Nice fix!
>> Thank you!
>>
>>> It catches a lot of new cases on our codebase, all technically
>>> correct so far.
>>>
>>> A couple of issues though:
>>> A) Rollout - until we've completely cleaned up, we need to disable
>>> -Wtautological-compare entirely, which is a valuable check. I imagine anyone
>>> else using -Werror is in the same boat.
>>> What do you think about putting the new warnings behind a subcategory? (e.g.
>>> -Wtautological-compare-unsigned-zero, implied by -Wtautological-compare)
>>> It's an ugly artifact of the history here, but allows this fix to be rolled
>>> out in a controlled way.
>> https://reviews.llvm.org/D37620
> And landed.
>
>>> B) Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX)
>>> The warning strongly suggests that the enum < 0 check has no effect (for
>>> enums with nonnegative ranges).
>>> Clang doesn't seem to optimize such checks out though, and they seem likely
>>> to catch bugs in some cases. Yes, only if there's UB elsewhere, but I assume
>>> not optimizing out these checks indicates a deliberate decision to stay
>>> somewhat compatible with a technically-incorrect mental model.
>>> If this is the case, should we move these to a -Wtautological-compare-enum
>>> subcategory?
>> (Did not look at this yet)
> https://reviews.llvm.org/D37629 i hope that is what you meant.
And landed, too.

Next will try to work on the second part of the fix for
https://bugs.llvm.org/show_bug.cgi?id=34147

>> Roman.
> Roman.
Roman.

>>> On Fri, Sep 8, 2017 at 12:14 AM, Roman Lebedev via cfe-commits
>>>  wrote:

 Author: lebedevri
 Date: Thu Sep  7 15:14:25 2017
 New Revision: 312750

 URL: http://llvm.org/viewvc/llvm-project?rev=312750=rev
 Log:
 [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

 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.

 Reviewers: rjmccall, rsmith, aaron.ballman

 Reviewed By: rjmccall

 Subscribers: cfe-commits

 Tags: #clang

 Differential Revision: https://reviews.llvm.org/D37565

 Modified:
 cfe/trunk/docs/ReleaseNotes.rst
 cfe/trunk/lib/Sema/SemaChecking.cpp
 cfe/trunk/test/Sema/compare.c
 cfe/trunk/test/Sema/outof-range-constant-compare.c
 cfe/trunk/test/SemaCXX/compare.cpp

 Modified: cfe/trunk/docs/ReleaseNotes.rst
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=312750=312749=312750=diff

 ==
 --- cfe/trunk/docs/ReleaseNotes.rst (original)
 +++ cfe/trunk/docs/ReleaseNotes.rst Thu Sep  7 15:14:25 2017
 @@ -71,6 +71,13 @@ Improvements to Clang's diagnostics
errors/warnings, as the system frameworks might add a method with the
 same
selector which could make the message send to ``id`` ambiguous.

 +- ``-Wtautological-compare`` now warns when comparing an unsigned integer
 and 0
 +  regardless of whether the constant is signed or unsigned."
 +
 +- ``-Wtautological-compare`` now warns about comparing a signed integer
 and 0
 +  when the signed integer is coerced to an unsigned type for the
 comparison.
 +  ``-Wsign-compare`` was adjusted not to warn in this case.
 +
  Non-comprehensive list of changes in this release
  -


 Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=312750=312749=312750=diff

 ==
 --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
 +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Sep  7 15:14:25 2017
 @@ -8567,32 +8567,51 @@ bool HasEnumType(Expr *E) {
return E->getType()->isEnumeralType();
  }

 -void CheckTrivialUnsignedComparison(Sema , BinaryOperator *E) {
 +bool isNonBooleanUnsignedValue(Expr *E) {
 +  // We are checking that the expression is not known to have boolean
 value,
 +  // is an 

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

[Aaron Ballman via cfe-commits]

On Fri, Sep 8, 2017 at 5:49 PM, Hans Wennborg via cfe-commits
 wrote:
> On Fri, Sep 8, 2017 at 2:26 PM, Friedman, Eli  wrote:
>> On 9/8/2017 2:18 PM, Hans Wennborg via cfe-commits wrote:
>>>
>>> On Fri, Sep 8, 2017 at 2:09 PM, Roman Lebedev 
>>> wrote:


 Interesting. My first thought was to explicitly specify enum as signed:

 enum MediaDeviceType : signed int {
 MEDIA_DEVICE_TYPE_AUDIO_INPUT = 0,
 MEDIA_DEVICE_TYPE_VIDEO_INPUT,
 MEDIA_DEVICE_TYPE_AUDIO_OUTPUT,
 NUM_MEDIA_DEVICE_TYPES,
 };

 inline bool IsValidMediaDeviceType(MediaDeviceType type) {
 return type >= 0U && type < NUM_MEDIA_DEVICE_TYPES;
 }

 But it still seem to warn, and *that* sounds like a bug.
 Please open a new bugreport.
>>>
>>> I'm reporting it here :-)
>>>
 As for different default signedness, i'm not sure what is there to do.
 Does
 not sound like a problem for this diagnostic to intentionally avoid to
 be honest.
>>>
>>> I think it is a problem for this warning. If a user sees the warning
>>> and removes the "type >= 0" check, thinking that it was unnecessary
>>> because the compiler told them so, they have now introduced a bug in
>>> the code.
>>
>>
>> Even if you declare the enum type as signed, it still isn't allowed to
>> contain a negative number; that's undefined behavior.  You can check for
>> this using ubsan's -fsanitize=enum.
>
> Is it? I thought if you define it as signed, it falls under "For an
> enumeration whose underlying type is fixed, the values of the
> enumeration are the values of the underlying type." (c++11 7.2p7)
>
> My standards-fu is weak though, so I could be wrong.

The enum values can be signed because of the fixed type, but the
comparison was enum_val <= 0U, so I believe the usual arithmetic
conversions will convert the enum value to an *unsigned* value, making
this a tautological comparison. If the value was 0, then I think the
comparison would not be tautological because there would be no
conversion needed.

> But I think you're right about the case when the underlying type is
> not specified. Even if it happens to be 'int' on Windows, negative
> values aren't allowed (unless there are negative enumerators). That
> doesn't mean it won't happen though, and lots of code isn't UB-clean
> :-/

Yes, but if the *optimizer* were ever clever enough to realize the
value cannot be negative, then your code still has a pretty serious
problem in the face of that same UB.

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

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

[Roman Lebedev via cfe-commits]

On Sat, Sep 9, 2017 at 12:18 AM, Hans Wennborg  wrote:
> On Fri, Sep 8, 2017 at 2:09 PM, Roman Lebedev  wrote:
>> On Fri, Sep 8, 2017 at 11:45 PM, Hans Wennborg  wrote:
>>> On Fri, Sep 8, 2017 at 4:48 AM, Sam McCall via cfe-commits
>>>  wrote:
 Nice fix! It catches a lot of new cases on our codebase, all technically
 correct so far.

 A couple of issues though:
 A) Rollout - until we've completely cleaned up, we need to disable
 -Wtautological-compare entirely, which is a valuable check. I imagine 
 anyone
 else using -Werror is in the same boat.
 What do you think about putting the new warnings behind a subcategory? 
 (e.g.
 -Wtautological-compare-unsigned-zero, implied by -Wtautological-compare)
 It's an ugly artifact of the history here, but allows this fix to be rolled
 out in a controlled way.

 B) Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX)
 The warning strongly suggests that the enum < 0 check has no effect (for
 enums with nonnegative ranges).
 Clang doesn't seem to optimize such checks out though, and they seem likely
 to catch bugs in some cases. Yes, only if there's UB elsewhere, but I 
 assume
 not optimizing out these checks indicates a deliberate decision to stay
 somewhat compatible with a technically-incorrect mental model.
 If this is the case, should we move these to a -Wtautological-compare-enum
 subcategory?
>>>
>>> Just another data point: we're seeing this a lot in Chromium, e.g.
>>>
>>> enum MediaDeviceType {
>>>   MEDIA_DEVICE_TYPE_AUDIO_INPUT,
>>>   MEDIA_DEVICE_TYPE_VIDEO_INPUT,
>>>   MEDIA_DEVICE_TYPE_AUDIO_OUTPUT,
>>>   NUM_MEDIA_DEVICE_TYPES,
>>> };
>>>
>>> inline bool IsValidMediaDeviceType(MediaDeviceType type) {
>>>   return type >= 0 && type < NUM_MEDIA_DEVICE_TYPES;
>>> }
>>>
>>> ../../content/common/media/media_devices.h:53:15: warning: comparison
>>> of unsigned enum expression >= 0 is always true
>>> [-Wtautological-unsigned-zero-compare]
>>>   return type >= 0 && type < NUM_MEDIA_DEVICE_TYPES;
>>>   ^  ~
>>>
>>> The problem is that the signedness of the enum varies per platform.
>>> For example, when targeting Windows the underlying type is 'int' by
>>> default, and then the comparison isn't tautological.
>>
>> Interesting. My first thought was to explicitly specify enum as signed:
>>
>> enum MediaDeviceType : signed int {
>> MEDIA_DEVICE_TYPE_AUDIO_INPUT = 0,
>> MEDIA_DEVICE_TYPE_VIDEO_INPUT,
>> MEDIA_DEVICE_TYPE_AUDIO_OUTPUT,
>> NUM_MEDIA_DEVICE_TYPES,
>> };
>>
>> inline bool IsValidMediaDeviceType(MediaDeviceType type) {
>> return type >= 0U && type < NUM_MEDIA_DEVICE_TYPES;
>> }
>>
>> But it still seem to warn, and *that* sounds like a bug.
>> Please open a new bugreport.
>
> I'm reporting it here :-)
Here i might forget :)

But as soon as i get a LGTM on https://reviews.llvm.org/D37629
you will at least be able to silence that particular warning.

>> As for different default signedness, i'm not sure what is there to do. Does
>> not sound like a problem for this diagnostic to intentionally avoid to
>> be honest.
>
> I think it is a problem for this warning. If a user sees the warning
> and removes the "type >= 0" check, thinking that it was unnecessary
> because the compiler told them so, they have now introduced a bug in
> the code.
>
 On Fri, Sep 8, 2017 at 12:14 AM, Roman Lebedev via cfe-commits
  wrote:
>
> Author: lebedevri
> Date: Thu Sep  7 15:14:25 2017
> New Revision: 312750
>
> URL: http://llvm.org/viewvc/llvm-project?rev=312750=rev
> Log:
> [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.
>
> 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.
>
> Reviewers: rjmccall, rsmith, aaron.ballman
>
> Reviewed By: rjmccall
>
> Subscribers: cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D37565
>
> Modified:
> cfe/trunk/docs/ReleaseNotes.rst
> cfe/trunk/lib/Sema/SemaChecking.cpp
> cfe/trunk/test/Sema/compare.c
> cfe/trunk/test/Sema/outof-range-constant-compare.c
> cfe/trunk/test/SemaCXX/compare.cpp
>
> Modified: cfe/trunk/docs/ReleaseNotes.rst
> URL:
> 

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

[Hans Wennborg via cfe-commits]

On Fri, Sep 8, 2017 at 2:26 PM, Friedman, Eli  wrote:
> On 9/8/2017 2:18 PM, Hans Wennborg via cfe-commits wrote:
>>
>> On Fri, Sep 8, 2017 at 2:09 PM, Roman Lebedev 
>> wrote:
>>>
>>>
>>> Interesting. My first thought was to explicitly specify enum as signed:
>>>
>>> enum MediaDeviceType : signed int {
>>> MEDIA_DEVICE_TYPE_AUDIO_INPUT = 0,
>>> MEDIA_DEVICE_TYPE_VIDEO_INPUT,
>>> MEDIA_DEVICE_TYPE_AUDIO_OUTPUT,
>>> NUM_MEDIA_DEVICE_TYPES,
>>> };
>>>
>>> inline bool IsValidMediaDeviceType(MediaDeviceType type) {
>>> return type >= 0U && type < NUM_MEDIA_DEVICE_TYPES;
>>> }
>>>
>>> But it still seem to warn, and *that* sounds like a bug.
>>> Please open a new bugreport.
>>
>> I'm reporting it here :-)
>>
>>> As for different default signedness, i'm not sure what is there to do.
>>> Does
>>> not sound like a problem for this diagnostic to intentionally avoid to
>>> be honest.
>>
>> I think it is a problem for this warning. If a user sees the warning
>> and removes the "type >= 0" check, thinking that it was unnecessary
>> because the compiler told them so, they have now introduced a bug in
>> the code.
>
>
> Even if you declare the enum type as signed, it still isn't allowed to
> contain a negative number; that's undefined behavior.  You can check for
> this using ubsan's -fsanitize=enum.

Is it? I thought if you define it as signed, it falls under "For an
enumeration whose underlying type is fixed, the values of the
enumeration are the values of the underlying type." (c++11 7.2p7)

My standards-fu is weak though, so I could be wrong.


But I think you're right about the case when the underlying type is
not specified. Even if it happens to be 'int' on Windows, negative
values aren't allowed (unless there are negative enumerators). That
doesn't mean it won't happen though, and lots of code isn't UB-clean
:-/
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Friedman, Eli via cfe-commits]

On 9/8/2017 2:18 PM, Hans Wennborg via cfe-commits wrote:

On Fri, Sep 8, 2017 at 2:09 PM, Roman Lebedev  wrote:


Interesting. My first thought was to explicitly specify enum as signed:

enum MediaDeviceType : signed int {
MEDIA_DEVICE_TYPE_AUDIO_INPUT = 0,
MEDIA_DEVICE_TYPE_VIDEO_INPUT,
MEDIA_DEVICE_TYPE_AUDIO_OUTPUT,
NUM_MEDIA_DEVICE_TYPES,
};

inline bool IsValidMediaDeviceType(MediaDeviceType type) {
return type >= 0U && type < NUM_MEDIA_DEVICE_TYPES;
}

But it still seem to warn, and *that* sounds like a bug.
Please open a new bugreport.

I'm reporting it here :-)


As for different default signedness, i'm not sure what is there to do. Does
not sound like a problem for this diagnostic to intentionally avoid to
be honest.

I think it is a problem for this warning. If a user sees the warning
and removes the "type >= 0" check, thinking that it was unnecessary
because the compiler told them so, they have now introduced a bug in
the code.


Even if you declare the enum type as signed, it still isn't allowed to 
contain a negative number; that's undefined behavior.  You can check for 
this using ubsan's -fsanitize=enum.


-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

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

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

[Hans Wennborg via cfe-commits]

On Fri, Sep 8, 2017 at 2:09 PM, Roman Lebedev  wrote:
> On Fri, Sep 8, 2017 at 11:45 PM, Hans Wennborg  wrote:
>> On Fri, Sep 8, 2017 at 4:48 AM, Sam McCall via cfe-commits
>>  wrote:
>>> Nice fix! It catches a lot of new cases on our codebase, all technically
>>> correct so far.
>>>
>>> A couple of issues though:
>>> A) Rollout - until we've completely cleaned up, we need to disable
>>> -Wtautological-compare entirely, which is a valuable check. I imagine anyone
>>> else using -Werror is in the same boat.
>>> What do you think about putting the new warnings behind a subcategory? (e.g.
>>> -Wtautological-compare-unsigned-zero, implied by -Wtautological-compare)
>>> It's an ugly artifact of the history here, but allows this fix to be rolled
>>> out in a controlled way.
>>>
>>> B) Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX)
>>> The warning strongly suggests that the enum < 0 check has no effect (for
>>> enums with nonnegative ranges).
>>> Clang doesn't seem to optimize such checks out though, and they seem likely
>>> to catch bugs in some cases. Yes, only if there's UB elsewhere, but I assume
>>> not optimizing out these checks indicates a deliberate decision to stay
>>> somewhat compatible with a technically-incorrect mental model.
>>> If this is the case, should we move these to a -Wtautological-compare-enum
>>> subcategory?
>>
>> Just another data point: we're seeing this a lot in Chromium, e.g.
>>
>> enum MediaDeviceType {
>>   MEDIA_DEVICE_TYPE_AUDIO_INPUT,
>>   MEDIA_DEVICE_TYPE_VIDEO_INPUT,
>>   MEDIA_DEVICE_TYPE_AUDIO_OUTPUT,
>>   NUM_MEDIA_DEVICE_TYPES,
>> };
>>
>> inline bool IsValidMediaDeviceType(MediaDeviceType type) {
>>   return type >= 0 && type < NUM_MEDIA_DEVICE_TYPES;
>> }
>>
>> ../../content/common/media/media_devices.h:53:15: warning: comparison
>> of unsigned enum expression >= 0 is always true
>> [-Wtautological-unsigned-zero-compare]
>>   return type >= 0 && type < NUM_MEDIA_DEVICE_TYPES;
>>   ^  ~
>>
>> The problem is that the signedness of the enum varies per platform.
>> For example, when targeting Windows the underlying type is 'int' by
>> default, and then the comparison isn't tautological.
>
> Interesting. My first thought was to explicitly specify enum as signed:
>
> enum MediaDeviceType : signed int {
> MEDIA_DEVICE_TYPE_AUDIO_INPUT = 0,
> MEDIA_DEVICE_TYPE_VIDEO_INPUT,
> MEDIA_DEVICE_TYPE_AUDIO_OUTPUT,
> NUM_MEDIA_DEVICE_TYPES,
> };
>
> inline bool IsValidMediaDeviceType(MediaDeviceType type) {
> return type >= 0U && type < NUM_MEDIA_DEVICE_TYPES;
> }
>
> But it still seem to warn, and *that* sounds like a bug.
> Please open a new bugreport.

I'm reporting it here :-)

> As for different default signedness, i'm not sure what is there to do. Does
> not sound like a problem for this diagnostic to intentionally avoid to
> be honest.

I think it is a problem for this warning. If a user sees the warning
and removes the "type >= 0" check, thinking that it was unnecessary
because the compiler told them so, they have now introduced a bug in
the code.

>>> On Fri, Sep 8, 2017 at 12:14 AM, Roman Lebedev via cfe-commits
>>>  wrote:

 Author: lebedevri
 Date: Thu Sep  7 15:14:25 2017
 New Revision: 312750

 URL: http://llvm.org/viewvc/llvm-project?rev=312750=rev
 Log:
 [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

 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.

 Reviewers: rjmccall, rsmith, aaron.ballman

 Reviewed By: rjmccall

 Subscribers: cfe-commits

 Tags: #clang

 Differential Revision: https://reviews.llvm.org/D37565

 Modified:
 cfe/trunk/docs/ReleaseNotes.rst
 cfe/trunk/lib/Sema/SemaChecking.cpp
 cfe/trunk/test/Sema/compare.c
 cfe/trunk/test/Sema/outof-range-constant-compare.c
 cfe/trunk/test/SemaCXX/compare.cpp

 Modified: cfe/trunk/docs/ReleaseNotes.rst
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=312750=312749=312750=diff

 ==
 --- cfe/trunk/docs/ReleaseNotes.rst (original)
 +++ cfe/trunk/docs/ReleaseNotes.rst Thu Sep  7 15:14:25 2017
 @@ -71,6 +71,13 @@ Improvements to Clang's diagnostics
errors/warnings, 

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

[Roman Lebedev via cfe-commits]

On Fri, Sep 8, 2017 at 11:45 PM, Hans Wennborg  wrote:
> On Fri, Sep 8, 2017 at 4:48 AM, Sam McCall via cfe-commits
>  wrote:
>> Nice fix! It catches a lot of new cases on our codebase, all technically
>> correct so far.
>>
>> A couple of issues though:
>> A) Rollout - until we've completely cleaned up, we need to disable
>> -Wtautological-compare entirely, which is a valuable check. I imagine anyone
>> else using -Werror is in the same boat.
>> What do you think about putting the new warnings behind a subcategory? (e.g.
>> -Wtautological-compare-unsigned-zero, implied by -Wtautological-compare)
>> It's an ugly artifact of the history here, but allows this fix to be rolled
>> out in a controlled way.
>>
>> B) Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX)
>> The warning strongly suggests that the enum < 0 check has no effect (for
>> enums with nonnegative ranges).
>> Clang doesn't seem to optimize such checks out though, and they seem likely
>> to catch bugs in some cases. Yes, only if there's UB elsewhere, but I assume
>> not optimizing out these checks indicates a deliberate decision to stay
>> somewhat compatible with a technically-incorrect mental model.
>> If this is the case, should we move these to a -Wtautological-compare-enum
>> subcategory?
>
> Just another data point: we're seeing this a lot in Chromium, e.g.
>
> enum MediaDeviceType {
>   MEDIA_DEVICE_TYPE_AUDIO_INPUT,
>   MEDIA_DEVICE_TYPE_VIDEO_INPUT,
>   MEDIA_DEVICE_TYPE_AUDIO_OUTPUT,
>   NUM_MEDIA_DEVICE_TYPES,
> };
>
> inline bool IsValidMediaDeviceType(MediaDeviceType type) {
>   return type >= 0 && type < NUM_MEDIA_DEVICE_TYPES;
> }
>
> ../../content/common/media/media_devices.h:53:15: warning: comparison
> of unsigned enum expression >= 0 is always true
> [-Wtautological-unsigned-zero-compare]
>   return type >= 0 && type < NUM_MEDIA_DEVICE_TYPES;
>   ^  ~
>
> The problem is that the signedness of the enum varies per platform.
> For example, when targeting Windows the underlying type is 'int' by
> default, and then the comparison isn't tautological.

Interesting. My first thought was to explicitly specify enum as signed:

enum MediaDeviceType : signed int {
MEDIA_DEVICE_TYPE_AUDIO_INPUT = 0,
MEDIA_DEVICE_TYPE_VIDEO_INPUT,
MEDIA_DEVICE_TYPE_AUDIO_OUTPUT,
NUM_MEDIA_DEVICE_TYPES,
};

inline bool IsValidMediaDeviceType(MediaDeviceType type) {
return type >= 0U && type < NUM_MEDIA_DEVICE_TYPES;
}

But it still seem to warn, and *that* sounds like a bug.
Please open a new bugreport.

As for different default signedness, i'm not sure what is there to do. Does
not sound like a problem for this diagnostic to intentionally avoid to
be honest.

>> On Fri, Sep 8, 2017 at 12:14 AM, Roman Lebedev via cfe-commits
>>  wrote:
>>>
>>> Author: lebedevri
>>> Date: Thu Sep  7 15:14:25 2017
>>> New Revision: 312750
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=312750=rev
>>> Log:
>>> [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.
>>>
>>> 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.
>>>
>>> Reviewers: rjmccall, rsmith, aaron.ballman
>>>
>>> Reviewed By: rjmccall
>>>
>>> Subscribers: cfe-commits
>>>
>>> Tags: #clang
>>>
>>> Differential Revision: https://reviews.llvm.org/D37565
>>>
>>> Modified:
>>> cfe/trunk/docs/ReleaseNotes.rst
>>> cfe/trunk/lib/Sema/SemaChecking.cpp
>>> cfe/trunk/test/Sema/compare.c
>>> cfe/trunk/test/Sema/outof-range-constant-compare.c
>>> cfe/trunk/test/SemaCXX/compare.cpp
>>>
>>> Modified: cfe/trunk/docs/ReleaseNotes.rst
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=312750=312749=312750=diff
>>>
>>> ==
>>> --- cfe/trunk/docs/ReleaseNotes.rst (original)
>>> +++ cfe/trunk/docs/ReleaseNotes.rst Thu Sep  7 15:14:25 2017
>>> @@ -71,6 +71,13 @@ Improvements to Clang's diagnostics
>>>errors/warnings, as the system frameworks might add a method with the
>>> same
>>>selector which could make the message send to ``id`` ambiguous.
>>>
>>> +- ``-Wtautological-compare`` now warns when comparing an unsigned integer
>>> and 0
>>> +  regardless of whether the constant is signed or unsigned."
>>> +
>>> +- ``-Wtautological-compare`` now warns about comparing a signed integer
>>> and 0
>>> +  when the signed integer is coerced to an unsigned type for the

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

[Hans Wennborg via cfe-commits]

On Fri, Sep 8, 2017 at 4:48 AM, Sam McCall via cfe-commits
 wrote:
> Nice fix! It catches a lot of new cases on our codebase, all technically
> correct so far.
>
> A couple of issues though:
> A) Rollout - until we've completely cleaned up, we need to disable
> -Wtautological-compare entirely, which is a valuable check. I imagine anyone
> else using -Werror is in the same boat.
> What do you think about putting the new warnings behind a subcategory? (e.g.
> -Wtautological-compare-unsigned-zero, implied by -Wtautological-compare)
> It's an ugly artifact of the history here, but allows this fix to be rolled
> out in a controlled way.
>
> B) Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX)
> The warning strongly suggests that the enum < 0 check has no effect (for
> enums with nonnegative ranges).
> Clang doesn't seem to optimize such checks out though, and they seem likely
> to catch bugs in some cases. Yes, only if there's UB elsewhere, but I assume
> not optimizing out these checks indicates a deliberate decision to stay
> somewhat compatible with a technically-incorrect mental model.
> If this is the case, should we move these to a -Wtautological-compare-enum
> subcategory?

Just another data point: we're seeing this a lot in Chromium, e.g.

enum MediaDeviceType {
  MEDIA_DEVICE_TYPE_AUDIO_INPUT,
  MEDIA_DEVICE_TYPE_VIDEO_INPUT,
  MEDIA_DEVICE_TYPE_AUDIO_OUTPUT,
  NUM_MEDIA_DEVICE_TYPES,
};

inline bool IsValidMediaDeviceType(MediaDeviceType type) {
  return type >= 0 && type < NUM_MEDIA_DEVICE_TYPES;
}

../../content/common/media/media_devices.h:53:15: warning: comparison
of unsigned enum expression >= 0 is always true
[-Wtautological-unsigned-zero-compare]
  return type >= 0 && type < NUM_MEDIA_DEVICE_TYPES;
  ^  ~

The problem is that the signedness of the enum varies per platform.
For example, when targeting Windows the underlying type is 'int' by
default, and then the comparison isn't tautological.



> On Fri, Sep 8, 2017 at 12:14 AM, Roman Lebedev via cfe-commits
>  wrote:
>>
>> Author: lebedevri
>> Date: Thu Sep  7 15:14:25 2017
>> New Revision: 312750
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=312750=rev
>> Log:
>> [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.
>>
>> 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.
>>
>> Reviewers: rjmccall, rsmith, aaron.ballman
>>
>> Reviewed By: rjmccall
>>
>> Subscribers: cfe-commits
>>
>> Tags: #clang
>>
>> Differential Revision: https://reviews.llvm.org/D37565
>>
>> Modified:
>> cfe/trunk/docs/ReleaseNotes.rst
>> cfe/trunk/lib/Sema/SemaChecking.cpp
>> cfe/trunk/test/Sema/compare.c
>> cfe/trunk/test/Sema/outof-range-constant-compare.c
>> cfe/trunk/test/SemaCXX/compare.cpp
>>
>> Modified: cfe/trunk/docs/ReleaseNotes.rst
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=312750=312749=312750=diff
>>
>> ==
>> --- cfe/trunk/docs/ReleaseNotes.rst (original)
>> +++ cfe/trunk/docs/ReleaseNotes.rst Thu Sep  7 15:14:25 2017
>> @@ -71,6 +71,13 @@ Improvements to Clang's diagnostics
>>errors/warnings, as the system frameworks might add a method with the
>> same
>>selector which could make the message send to ``id`` ambiguous.
>>
>> +- ``-Wtautological-compare`` now warns when comparing an unsigned integer
>> and 0
>> +  regardless of whether the constant is signed or unsigned."
>> +
>> +- ``-Wtautological-compare`` now warns about comparing a signed integer
>> and 0
>> +  when the signed integer is coerced to an unsigned type for the
>> comparison.
>> +  ``-Wsign-compare`` was adjusted not to warn in this case.
>> +
>>  Non-comprehensive list of changes in this release
>>  -
>>
>>
>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=312750=312749=312750=diff
>>
>> ==
>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Sep  7 15:14:25 2017
>> @@ -8567,32 +8567,51 @@ bool HasEnumType(Expr *E) {
>>return E->getType()->isEnumeralType();
>>  }
>>
>> -void CheckTrivialUnsignedComparison(Sema , BinaryOperator *E) {
>> +bool isNonBooleanUnsignedValue(Expr *E) {
>> +  

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

[Roman Lebedev via cfe-commits]

On Fri, Sep 8, 2017 at 3:26 PM, Roman Lebedev  wrote:
> On Fri, Sep 8, 2017 at 2:48 PM, Sam McCall  wrote:
> Hi.
>
>> Nice fix!
> Thank you!
>
>> It catches a lot of new cases on our codebase, all technically
>> correct so far.
>>
>> A couple of issues though:
>> A) Rollout - until we've completely cleaned up, we need to disable
>> -Wtautological-compare entirely, which is a valuable check. I imagine anyone
>> else using -Werror is in the same boat.
>> What do you think about putting the new warnings behind a subcategory? (e.g.
>> -Wtautological-compare-unsigned-zero, implied by -Wtautological-compare)
>> It's an ugly artifact of the history here, but allows this fix to be rolled
>> out in a controlled way.
> https://reviews.llvm.org/D37620
And landed.

>> B) Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX)
>> The warning strongly suggests that the enum < 0 check has no effect (for
>> enums with nonnegative ranges).
>> Clang doesn't seem to optimize such checks out though, and they seem likely
>> to catch bugs in some cases. Yes, only if there's UB elsewhere, but I assume
>> not optimizing out these checks indicates a deliberate decision to stay
>> somewhat compatible with a technically-incorrect mental model.
>> If this is the case, should we move these to a -Wtautological-compare-enum
>> subcategory?
> (Did not look at this yet)
https://reviews.llvm.org/D37629 i hope that is what you meant.

> Roman.
Roman.

>> On Fri, Sep 8, 2017 at 12:14 AM, Roman Lebedev via cfe-commits
>>  wrote:
>>>
>>> Author: lebedevri
>>> Date: Thu Sep  7 15:14:25 2017
>>> New Revision: 312750
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=312750=rev
>>> Log:
>>> [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.
>>>
>>> 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.
>>>
>>> Reviewers: rjmccall, rsmith, aaron.ballman
>>>
>>> Reviewed By: rjmccall
>>>
>>> Subscribers: cfe-commits
>>>
>>> Tags: #clang
>>>
>>> Differential Revision: https://reviews.llvm.org/D37565
>>>
>>> Modified:
>>> cfe/trunk/docs/ReleaseNotes.rst
>>> cfe/trunk/lib/Sema/SemaChecking.cpp
>>> cfe/trunk/test/Sema/compare.c
>>> cfe/trunk/test/Sema/outof-range-constant-compare.c
>>> cfe/trunk/test/SemaCXX/compare.cpp
>>>
>>> Modified: cfe/trunk/docs/ReleaseNotes.rst
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=312750=312749=312750=diff
>>>
>>> ==
>>> --- cfe/trunk/docs/ReleaseNotes.rst (original)
>>> +++ cfe/trunk/docs/ReleaseNotes.rst Thu Sep  7 15:14:25 2017
>>> @@ -71,6 +71,13 @@ Improvements to Clang's diagnostics
>>>errors/warnings, as the system frameworks might add a method with the
>>> same
>>>selector which could make the message send to ``id`` ambiguous.
>>>
>>> +- ``-Wtautological-compare`` now warns when comparing an unsigned integer
>>> and 0
>>> +  regardless of whether the constant is signed or unsigned."
>>> +
>>> +- ``-Wtautological-compare`` now warns about comparing a signed integer
>>> and 0
>>> +  when the signed integer is coerced to an unsigned type for the
>>> comparison.
>>> +  ``-Wsign-compare`` was adjusted not to warn in this case.
>>> +
>>>  Non-comprehensive list of changes in this release
>>>  -
>>>
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=312750=312749=312750=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Sep  7 15:14:25 2017
>>> @@ -8567,32 +8567,51 @@ bool HasEnumType(Expr *E) {
>>>return E->getType()->isEnumeralType();
>>>  }
>>>
>>> -void CheckTrivialUnsignedComparison(Sema , BinaryOperator *E) {
>>> +bool isNonBooleanUnsignedValue(Expr *E) {
>>> +  // We are checking that the expression is not known to have boolean
>>> value,
>>> +  // is an integer type; and is either unsigned after implicit casts,
>>> +  // or was unsigned before implicit casts.
>>> +  return !E->isKnownToHaveBooleanValue() && E->getType()->isIntegerType()
>>> &&
>>> + (!E->getType()->isSignedIntegerType() ||
>>> +  

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

[Roman Lebedev via cfe-commits]

On Fri, Sep 8, 2017 at 2:48 PM, Sam McCall  wrote:
Hi.

> Nice fix!
Thank you!

> It catches a lot of new cases on our codebase, all technically
> correct so far.
>
> A couple of issues though:
> A) Rollout - until we've completely cleaned up, we need to disable
> -Wtautological-compare entirely, which is a valuable check. I imagine anyone
> else using -Werror is in the same boat.
> What do you think about putting the new warnings behind a subcategory? (e.g.
> -Wtautological-compare-unsigned-zero, implied by -Wtautological-compare)
> It's an ugly artifact of the history here, but allows this fix to be rolled
> out in a controlled way.
https://reviews.llvm.org/D37620

> B) Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX)
> The warning strongly suggests that the enum < 0 check has no effect (for
> enums with nonnegative ranges).
> Clang doesn't seem to optimize such checks out though, and they seem likely
> to catch bugs in some cases. Yes, only if there's UB elsewhere, but I assume
> not optimizing out these checks indicates a deliberate decision to stay
> somewhat compatible with a technically-incorrect mental model.
> If this is the case, should we move these to a -Wtautological-compare-enum
> subcategory?
(Did not look at this yet)

Roman.

> On Fri, Sep 8, 2017 at 12:14 AM, Roman Lebedev via cfe-commits
>  wrote:
>>
>> Author: lebedevri
>> Date: Thu Sep  7 15:14:25 2017
>> New Revision: 312750
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=312750=rev
>> Log:
>> [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.
>>
>> 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.
>>
>> Reviewers: rjmccall, rsmith, aaron.ballman
>>
>> Reviewed By: rjmccall
>>
>> Subscribers: cfe-commits
>>
>> Tags: #clang
>>
>> Differential Revision: https://reviews.llvm.org/D37565
>>
>> Modified:
>> cfe/trunk/docs/ReleaseNotes.rst
>> cfe/trunk/lib/Sema/SemaChecking.cpp
>> cfe/trunk/test/Sema/compare.c
>> cfe/trunk/test/Sema/outof-range-constant-compare.c
>> cfe/trunk/test/SemaCXX/compare.cpp
>>
>> Modified: cfe/trunk/docs/ReleaseNotes.rst
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=312750=312749=312750=diff
>>
>> ==
>> --- cfe/trunk/docs/ReleaseNotes.rst (original)
>> +++ cfe/trunk/docs/ReleaseNotes.rst Thu Sep  7 15:14:25 2017
>> @@ -71,6 +71,13 @@ Improvements to Clang's diagnostics
>>errors/warnings, as the system frameworks might add a method with the
>> same
>>selector which could make the message send to ``id`` ambiguous.
>>
>> +- ``-Wtautological-compare`` now warns when comparing an unsigned integer
>> and 0
>> +  regardless of whether the constant is signed or unsigned."
>> +
>> +- ``-Wtautological-compare`` now warns about comparing a signed integer
>> and 0
>> +  when the signed integer is coerced to an unsigned type for the
>> comparison.
>> +  ``-Wsign-compare`` was adjusted not to warn in this case.
>> +
>>  Non-comprehensive list of changes in this release
>>  -
>>
>>
>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=312750=312749=312750=diff
>>
>> ==
>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Sep  7 15:14:25 2017
>> @@ -8567,32 +8567,51 @@ bool HasEnumType(Expr *E) {
>>return E->getType()->isEnumeralType();
>>  }
>>
>> -void CheckTrivialUnsignedComparison(Sema , BinaryOperator *E) {
>> +bool isNonBooleanUnsignedValue(Expr *E) {
>> +  // We are checking that the expression is not known to have boolean
>> value,
>> +  // is an integer type; and is either unsigned after implicit casts,
>> +  // or was unsigned before implicit casts.
>> +  return !E->isKnownToHaveBooleanValue() && E->getType()->isIntegerType()
>> &&
>> + (!E->getType()->isSignedIntegerType() ||
>> +  !E->IgnoreParenImpCasts()->getType()->isSignedIntegerType());
>> +}
>> +
>> +bool CheckTautologicalComparisonWithZero(Sema , BinaryOperator *E) {
>>// Disable warning in template instantiations.
>>if (S.inTemplateInstantiation())
>> -return;
>> +return false;
>> +
>> +  // bool values are handled by 

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

[Sam McCall via cfe-commits]

Nice fix! It catches a lot of new cases on our codebase, all technically
correct so far.

A couple of issues though:
A) Rollout - until we've completely cleaned up, we need to disable
-Wtautological-compare entirely, which is a valuable check. I imagine
anyone else using -Werror is in the same boat.
What do you think about putting the new warnings behind a subcategory?
(e.g. -Wtautological-compare-unsigned-zero, implied by
-Wtautological-compare)
It's an ugly artifact of the history here, but allows this fix to be rolled
out in a controlled way.

B) Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX)
The warning strongly suggests that the enum < 0 check has no effect (for
enums with nonnegative ranges).
Clang doesn't seem to optimize such checks out though, and they seem likely
to catch bugs in some cases. Yes, only if there's UB elsewhere, but I
assume not optimizing out these checks indicates a deliberate decision to
stay somewhat compatible with a technically-incorrect mental model.
If this is the case, should we move these to a -Wtautological-compare-enum
subcategory?

On Fri, Sep 8, 2017 at 12:14 AM, Roman Lebedev via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: lebedevri
> Date: Thu Sep  7 15:14:25 2017
> New Revision: 312750
>
> URL: http://llvm.org/viewvc/llvm-project?rev=312750=rev
> Log:
> [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.
>
> 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.
>
> Reviewers: rjmccall, rsmith, aaron.ballman
>
> Reviewed By: rjmccall
>
> Subscribers: cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D37565
>
> Modified:
> cfe/trunk/docs/ReleaseNotes.rst
> cfe/trunk/lib/Sema/SemaChecking.cpp
> cfe/trunk/test/Sema/compare.c
> cfe/trunk/test/Sema/outof-range-constant-compare.c
> cfe/trunk/test/SemaCXX/compare.cpp
>
> Modified: cfe/trunk/docs/ReleaseNotes.rst
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/
> ReleaseNotes.rst?rev=312750=312749=312750=diff
> 
> ==
> --- cfe/trunk/docs/ReleaseNotes.rst (original)
> +++ cfe/trunk/docs/ReleaseNotes.rst Thu Sep  7 15:14:25 2017
> @@ -71,6 +71,13 @@ Improvements to Clang's diagnostics
>errors/warnings, as the system frameworks might add a method with the
> same
>selector which could make the message send to ``id`` ambiguous.
>
> +- ``-Wtautological-compare`` now warns when comparing an unsigned integer
> and 0
> +  regardless of whether the constant is signed or unsigned."
> +
> +- ``-Wtautological-compare`` now warns about comparing a signed integer
> and 0
> +  when the signed integer is coerced to an unsigned type for the
> comparison.
> +  ``-Wsign-compare`` was adjusted not to warn in this case.
> +
>  Non-comprehensive list of changes in this release
>  -
>
>
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/
> SemaChecking.cpp?rev=312750=312749=312750=diff
> 
> ==
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Sep  7 15:14:25 2017
> @@ -8567,32 +8567,51 @@ bool HasEnumType(Expr *E) {
>return E->getType()->isEnumeralType();
>  }
>
> -void CheckTrivialUnsignedComparison(Sema , BinaryOperator *E) {
> +bool isNonBooleanUnsignedValue(Expr *E) {
> +  // We are checking that the expression is not known to have boolean
> value,
> +  // is an integer type; and is either unsigned after implicit casts,
> +  // or was unsigned before implicit casts.
> +  return !E->isKnownToHaveBooleanValue() &&
> E->getType()->isIntegerType() &&
> + (!E->getType()->isSignedIntegerType() ||
> +  !E->IgnoreParenImpCasts()->getType()->isSignedIntegerType());
> +}
> +
> +bool CheckTautologicalComparisonWithZero(Sema , BinaryOperator *E) {
>// Disable warning in template instantiations.
>if (S.inTemplateInstantiation())
> -return;
> +return false;
> +
> +  // bool values are handled by DiagnoseOutOfRangeComparison().
>
>BinaryOperatorKind op = E->getOpcode();
>if (E->isValueDependent())
> -return;
> +return false;
>
> -  if (op == BO_LT && IsZero(S, E->getRHS())) {
> +  Expr *LHS = E->getLHS();
> +  Expr *RHS = E->getRHS();
> +
> +  bool Match = true;
> +
> +  if (op == BO_LT &&