-Wtautological-constant-compare issues

Hi all (CC some people who have been involved in this discussion already and Hans for clang 6 release discussion),

-Wtautological-constant-compare was introduced in r315614 to diagnose tautological comparisons against a type’s maximum and minimum bounds. However, this warning can fire spuriously when a type’s size is platform-dependent. For example, libc++ has some generic code for which the warning fires on platforms where int and long have the same size; see https://reviews.llvm.org/D39149 for my initial attempt to work around the warning, and then https://reviews.llvm.org/D41368, which just silences the warning where it’s problematic.

Unfortunately, this appears to be a pretty widespread problem. For example, Petr Hosek reported in https://reviews.llvm.org/D39462 that he had to globally disable the warning when they rolled out a newer clang. Roman attempted to address this problem by having the warning take type size differences into account in https://reviews.llvm.org/D39462, but that’s tricky to implement and hit some implementation roadblocks.

I think shipping the warning in its current state in clang 6 is going to be problematic, because there are going to be many instances of generic code running into spurious warnings. At the very least, I think the warning as-is shouldn’t be part of -Wall. What are other people’s thoughts on this issue?

Thanks,

Shoaib

Hi,

Hi all (CC some people who have been involved in this discussion already and Hans for clang 6 release discussion),

-Wtautological-constant-compare was introduced in r315614 to diagnose tautological comparisons against a type's maximum and minimum bounds. However, this warning can fire spuriously when a type's size is platform-dependent. For example, libc++ has some generic code for which the warning fires on platforms where int and long have the same size;

I think our kernel team is planning on disabling -Wtautological-constant-compare for this exact reason. I've CC'd Alex, who may know more about any issues with this diagnostic w.r.t Apple projects. If the false positive really is widespread, I'd be supportive of disabling it by default or in -Wall.

vedant

If there's significant community concern about this, and to me that standard has been met, then I think the only reasonable solution is to take it out of -Wall until we feel more confident in it.

John.

I agree. I think we currently have -Wtautological-out-of-range-constant-compare (or similar? not near a computer right now…) as a subgroup; that should remain in -Wall as it predates this new warning, and we should make sure we have a dedicated flag for just the “in the range of the type” portion of the warning.

I concur.

Yesterday I received the following bug report against libc++:
   Bug 35698 - include/istream causes test failures on x86 due to
-Werror,-Wtautological-constant-compare
   https://bugs.llvm.org/show_bug.cgi?id=35698

-- Marshall

+Roman, the patch author

+1 for disabling this, most of these have been false positives.

Hi all!

First of all, i'm sorry for creating this issue, it was not my intention :slight_smile:

I have created https://reviews.llvm.org/D41512
Does it address all the concerns without completely killing the diagnostic?

While there, i have a question about line 8870 of semachecking.cpp:
https://github.com/llvm-mirror/clang/blob/c98d5adb4e64815effa4855a3804cc1a9a7d2958/lib/Sema/SemaChecking.cpp#L8870

if (InRange && IsEnumConstOrFromMacro(S, Constant))
  return false;

This completely prevents the diagnostic from firing on cases like:

  unsigned char x = ...
  assert(x <= 255);

Is this *really* the intention here?
Perhaps it was meant to silence in in the cases like:

  #define val_is_in_bounds(x) ((x) <= 255)
  ...
  unsigned char x = ...
  assert(val_is_in_bounds(x));

?

Roman.

Hi all!

First of all, i'm sorry for creating this issue, it was not my intention :slight_smile:

I have created https://reviews.llvm.org/D41512
Does it address all the concerns without completely killing the diagnostic?

I think we should be more cautious and just treat it as an experimental warning like any other: that is, it should not be in any standard warning groups, even -Wextra.

That is, for now, unless someone specifically passes -Wtautological-constant-compare (or -Weverything), they should just get the behavior prior to the implementation of this warning. As Richard pointed out, this should only affect the new warning, not the existing -Wtautological-constant-out-of-range-compare.

Don't feel bad about this; I think anyone who's done a significant amount of warning work has gone through the exact same thing. I certainly know I have. I think this is a very good diagnostic in principle, we just need to get it right.

John.

Hi all!

First of all, i'm sorry for creating this issue, it was not my intention :slight_smile:

I have created https://reviews.llvm.org/D41512
Does it address all the concerns without completely killing the diagnostic?

I think we should be more cautious and just treat it as an experimental warning like any other: that is, it should not be in any standard warning groups, even -Wextra.

That is, for now, unless someone specifically passes -Wtautological-constant-compare (or -Weverything), they should just get the behavior prior to the implementation of this warning. As Richard pointed out, this should only affect the new warning, not the existing -Wtautological-constant-out-of-range-compare.

Don't feel bad about this; I think anyone who's done a significant amount of warning work has gone through the exact same thing. I certainly know I have. I think this is a very good diagnostic in principle, we just need to get it right.

Apologies for coming late to the thread.

What's the status here? Did everything land, and should we merge r321691 to 6.0?

Thanks,
Hans

It landed and I think we should merge r321691 to 6.0.

~Aaron

Hi all!

First of all, i'm sorry for creating this issue, it was not my intention :slight_smile:

I have created https://reviews.llvm.org/D41512
Does it address all the concerns without completely killing the diagnostic?

I think we should be more cautious and just treat it as an experimental warning like any other: that is, it should not be in any standard warning groups, even -Wextra.

That is, for now, unless someone specifically passes -Wtautological-constant-compare (or -Weverything), they should just get the behavior prior to the implementation of this warning. As Richard pointed out, this should only affect the new warning, not the existing -Wtautological-constant-out-of-range-compare.

Don't feel bad about this; I think anyone who's done a significant amount of warning work has gone through the exact same thing. I certainly know I have. I think this is a very good diagnostic in principle, we just need to get it right.

Apologies for coming late to the thread.

No problem.

What's the status here? Did everything land, and should we merge r321691 to 6.0?

It landed before branching. I see r321691 in release_60 branch.
So nothing do be done here.

Thanks,
Hans

Roman.

Hi all!

First of all, i'm sorry for creating this issue, it was not my intention :slight_smile:

I have created https://reviews.llvm.org/D41512
Does it address all the concerns without completely killing the diagnostic?

I think we should be more cautious and just treat it as an experimental warning like any other: that is, it should not be in any standard warning groups, even -Wextra.

That is, for now, unless someone specifically passes -Wtautological-constant-compare (or -Weverything), they should just get the behavior prior to the implementation of this warning. As Richard pointed out, this should only affect the new warning, not the existing -Wtautological-constant-out-of-range-compare.

Don't feel bad about this; I think anyone who's done a significant amount of warning work has gone through the exact same thing. I certainly know I have. I think this is a very good diagnostic in principle, we just need to get it right.

Apologies for coming late to the thread.

No problem.

What's the status here? Did everything land, and should we merge r321691 to 6.0?

It landed before branching. I see r321691 in release_60 branch.
So nothing do be done here.

Ah, right. Thanks for checking! Sorry for the noise.

From: cfe-dev [mailto:cfe-dev-bounces@lists.llvm.org] On Behalf Of Hans
Wennborg via cfe-dev
Sent: Tuesday, January 16, 2018 8:01 AM
To: Roman Lebedev <lebedev.ri@gmail.com>
Cc: Marshall Clow <mclow.lists@gmail.com>; Richard Smith <richard-
llvm@metafoo.co.uk>; cfe-dev@lists.llvm.org; John McCall
<rjmccall@gmail.com>
Subject: Re: [cfe-dev] -Wtautological-constant-compare issues

...

>>
>> Apologies for coming late to the thread.
> No problem.
>
>> What's the status here? Did everything land, and should we merge
r321691 to 6.0?
> It landed before branching. I see r321691 in release_60 branch.
> So nothing do be done here.

Ah, right. Thanks for checking! Sorry for the noise.

Can we consider https://reviews.llvm.org/D41727 for inclusion in master and possibly release_60?

-Brian

Wait, wasn’t the consensus here to leave the warning out of -Wextra too? Looks like r321691 got that wrong?

Yes, I’d prefer to take this warning out of -Wextra at least for Clang 6. Hopefully we can find good heuristics to suppress the false positives for Clang 7 and then re-enable it.

Wait, wasn't the consensus here to leave the warning out of -Wextra too?
Looks like r321691 got that wrong?

Hm, consensus? Since it was pretty quite clear (?) from the diff and commit
message that it was only moved to -Wextra, i guess this was somehow
missed by me and all the reviewers...

Yes, I'd prefer to take this warning out of -Wextra at least for Clang 6.

What exactly do you have in mind?
Only from release_60 branch, or from trunk+release_60?

Hopefully we can find good heuristics to suppress the false positives for
Clang 7 and then re-enable it.

John McCall did post a short summary of what *seemingly* needs to be
done in ⚙ D39462 [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent
To be honest i'm not quite sure where to start on that.

Roman.

> Wait, wasn't the consensus here to leave the warning out of -Wextra too?
> Looks like r321691 got that wrong?
Hm, consensus? Since it was pretty quite clear (?) from the diff and commit
message that it was only moved to -Wextra, i guess this was somehow
missed by me and all the reviewers...

> Yes, I'd prefer to take this warning out of -Wextra at least for Clang 6.
What exactly do you have in mind?
Only from release_60 branch, or from trunk+release_60?

trunk+release_60; we do trunk-based development.

Roman, do you want to remove the warning from -Wextra, or do you want me to do it?

Roman, do you want to remove the warning from -Wextra

No, i do not want to remove the warning from -Wextra.
I have specified that quite clearly in all previous communications.

Next time, please do actually review such differentials, if you care.
Much better than having this disscussion this late after branching.

or do you want me to do it?

So be it. *Please* do spell all these details in the commit message.

Roman.

I just received https://bugs.llvm.org/show_bug.cgi?id=35997, which was
reported by a user building with '-Wall', '-Wextra', '-Werror'.

-- Marshall