[Bug c++/113852] New: -Wsign-compare doesn't warn on unsigned result types

[Bug c++/113852] New: -Wsign-compare doesn't warn on unsigned result types

[admin at computerquip dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113852

            Bug ID: 113852
           Summary: -Wsign-compare doesn't warn on unsigned result types
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: admin at computerquip dot com
  Target Milestone: ---

I haven't quite figured out the pattern here so the title may not be great.
Some code may help explain better: https://godbolt.org/z/d8cqd1WqP

```
#include <iostream>
#include <limits>
#include <cinttypes>

int main()
{
    uint16_t a1 = std::numeric_limits<unsigned short>::max();
    uint16_t a2 = std::numeric_limits<unsigned short>::max();

    /* a1 * a2 should be 4294836225 in math terms */
    uint64_t a3 = 4294836225;

    /*
     * The result of (a1 * a2) is of type int and the result is negative.
     * (a1 * a2) ends up as some bogus high number because the common
     * type here ends up as uint64_t and sign-extension occurs.
     */
    if ((a1 * a2) > a3) {
        std::cout << "this will print\n";
    }
}
```

Some observations I've noticed:
* If either a1 or a2 are constexpr, the warning will occur.
* This used to warn up until 8.1.
* Clang also doesn't warn here but MSVC will with /W3 or higher.
* This seems like a slight variation of
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101645

[Bug c++/113852] -Wsign-compare doesn't warn on unsigned result types

[admin at computerquip dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113852

--- Comment #1 from Zachary L <admin at computerquip dot com> ---
Sorry, that should say "If *both* a1 or a2 are constexpr, the warning will
occur."

[Bug c++/113852] -Wsign-compare doesn't warn on unsigned result types

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113852

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2024-02-09

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
If a1 and a2 are const then GCC also notices the undefined overflow:

<source>: In function 'int main()':
<source>:18:13: warning: integer overflow in expression of type 'int' results
in '-131071' [-Woverflow]
   18 |     if ((a1 * a2) > a3) {
      |          ~~~^~~~
<source>:18:19: warning: comparison of integer expressions of different
signedness: 'int' and 'uint64_t' {aka 'long unsigned int'} [-Wsign-compare]
   18 |     if ((a1 * a2) > a3) {
      |         ~~~~~~~~~~^~~~

I don't know why the -Wsign-compare warning is only given when we've detected
the overflow. The types are always the same, whether the values are known to
overflow or not.

I assume what's happening is that GCC assumes integer promotion from uint16_t
to int is value preserving and so we get two positive values, and therefore
comparison with an unsigned value is fine - there are no negative values
involved and so it doesn't matter that we're comparing int with unsigned long.
But of course that's not true here. We have two positive ints but their product
overflows to produce a negative int. I guess we're also assuming no overflow
happens, because that would be undefined and we assume no UB.

When the values are constant we can tell the overflow happens, and no longer
assume it doesn't happen.

[Bug c++/113852] -Wsign-compare doesn't warn on unsigned result types

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113852

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #2)
> I assume what's happening is that GCC assumes integer promotion from
> uint16_t to int is value preserving and so we get two positive values, and
> therefore comparison with an unsigned value is fine - there are no negative
> values involved and so it doesn't matter that we're comparing int with
> unsigned long. But of course that's not true here. We have two positive ints
> but their product overflows to produce a negative int. I guess we're also
> assuming no overflow happens, because that would be undefined and we assume
> no UB.

Ah yes, if you add -fwrapv then you get the -Wsign-compare warning, because now
a negative product can occur without undefined overflow.

[Bug c++/113852] -Wsign-compare doesn't warn on unsigned result types

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113852

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Reduced:

int main()
{
    unsigned short a1 = (1u << 16) - 1;
    unsigned short a2 = a1;

    /* a1 * a2 should be 4294836225 in math terms */
    unsigned long long a3 = 4294836225;

    /*
     * The result of (a1 * a2) is of type int and the result is negative.
     * (a1 * a2) ends up as some bogus high number because the common
     * type here ends up as uint64_t and sign-extension occurs.
     */
    if ((a1 * a2) > a3) {
        __builtin_abort();
    }
}

[Bug c++/113852] -Wsign-compare doesn't warn on unsigned result types

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113852

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The behaviour changed with r247495

common.opt (fstrict-overflow): Alias negative to fwrapv.

2017-05-02  Richard Biener 

* common.opt (fstrict-overflow): Alias negative to fwrapv.
* doc/invoke.texi (fstrict-overflow): Remove all traces of
-fstrict-overflow documentation.
* tree.h (TYPE_OVERFLOW_UNDEFINED): Do not test flag_strict_overflow.
(POINTER_TYPE_OVERFLOW_UNDEFINED): Test !flag_wrapv instead of
flag_strict_overflow.
* ipa-inline.c (can_inline_edge_p): Do not test flag_strict_overflow.
* lto-opts.c (lto_write_options): Do not stream it.
* lto-wrapper.c (merge_and_complain): Do not handle it.
* opts.c (default_options_table): Do not set -fstrict-overflow.
(finish_options): Likewise do not clear it when sanitizing.
* simplify-rtx.c (simplify_const_relational_operation): Do not
test flag_strict_overflow.

[Bug c++/113852] -Wsign-compare doesn't warn on unsigned result types

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113852

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
Well, given athat a1 * a2 is carried out in 'int' you are invoking undefined
behavior if it overflows.  GCC assumes that doesn't happen so it's correct
to elide the diagnostic.  Unless you make overflow well-defined with -fwrapv.

I think that errors on the right side for the purpose of -Wsign-compare.

[Bug c++/113852] -Wsign-compare doesn't warn on unsigned result types

[admin at computerquip dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113852

--- Comment #7 from Zachary L <admin at computerquip dot com> ---
(In reply to Richard Biener from comment #6)
> Well, given athat a1 * a2 is carried out in 'int' you are invoking undefined
> behavior if it overflows.  GCC assumes that doesn't happen so it's correct
> to elide the diagnostic.  Unless you make overflow well-defined with -fwrapv.
> 
> I think that errors on the right side for the purpose of -Wsign-compare.

Removing a diagnostic because the program could be ill-formed seems backwards
to me, especially since it seems like there's already logic in performing this
diagnostic. Perhaps I've misunderstood the situation?

[Bug c++/113852] -Wsign-compare doesn't warn on unsigned result types

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113852

--- Comment #8 from rguenther at suse dot de <rguenther at suse dot de> ---
On Mon, 12 Feb 2024, admin at computerquip dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113852
> 
> --- Comment #7 from Zachary L <admin at computerquip dot com> ---
> (In reply to Richard Biener from comment #6)
> > Well, given athat a1 * a2 is carried out in 'int' you are invoking undefined
> > behavior if it overflows.  GCC assumes that doesn't happen so it's correct
> > to elide the diagnostic.  Unless you make overflow well-defined with -fwrapv.
> > 
> > I think that errors on the right side for the purpose of -Wsign-compare.
> 
> Removing a diagnostic because the program could be ill-formed seems backwards
> to me, especially since it seems like there's already logic in performing this
> diagnostic. Perhaps I've misunderstood the situation?

Well, we could, for each int * int diagnose a "may invoke undefined 
behavior if overflow happens at runtime" but that's hardly useful.
So we want to diagnose cases where it's _likely_ that there's an
issue.

For the case at hand it's not likely that ushort * ushort invokes
undefined behavior (because invoking undefined should be unlikely,
very much so).  So we choose to not diagnose it as "maybe".

Yes, with constexpr evaluation we actually _see_ we invoke undefined
behavior and diagnose that.  The subsequent diagnostic of
-Wsign-compare is then spurious though and IMO should be not given.
The error is the integer overflow invoking undefined behavior even
- if that didn't happen there would be no issue with comparing 
signed/unsigned values.