[Bug c/103881] New: Wconversion false positive when using |= and &= with two rvalues in binary op

[Bug c/103881] New: Wconversion false positive when using |= and &= with two rvalues in binary op

[thomas at habets dot se]

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

            Bug ID: 103881
           Summary: Wconversion false positive when using |= and &= with
                    two rvalues in binary op
           Product: gcc
           Version: 11.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: thomas at habets dot se
  Target Milestone: ---

Got:

Warnings of conversion from int when doing "x |= f() | f()", with all types set
to uint8_t.

No warning when using "=" instead of "|=".

Per https://gcc.gnu.org/bugzilla/show_bug.cgi?id=40752, this makes me wonder if
"|=" creates as implicit cast to int (but why?).
But if that's the case then why does "x |= f()" and "x |= a & a" not warn?
And if "|=" implicitly casts to int, that should mean there's no way at all to
not trigger this warning for |= into an uint8_t?

Or does binary "&" implicitly cast to int (again, why?), then why does "x |= a
& a" and "x = f() & f()" not warn?

Reproduce program:
#include<inttypes.h>
uint8_t f();
void foo()
{
  uint8_t t = 0;
  t |= f();       // one rvalue: no warning
  t |= f() & f(); // two rvalues: warn
  t &= f() & f(); // same with &=
  t = f() & f();  // no warning with assign

  uint8_t a = f(); // no warning
  t |= a & a;      // no warning with variables.
  t |= f() & a;    // warning with one rvalue
  t |= a & f();    // no matter position.
}

Output
$ gcc -v -save-temps -Wconversion -c bug2.c
Using built-in specs.
COLLECT_GCC=gcc
OFFLOAD_TARGET_NAMES=nvptx-none:amdgcn-amdhsa
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 11.2.0-10'
--with-bugurl=file:///usr/share/doc/gcc-11/README.Bugs
--enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++,m2 --prefix=/usr
--with-gcc-major-version-only --program-suffix=-11
--program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id
--libexecdir=/usr/lib --without-included-gettext --enable-threads=posix
--libdir=/usr/lib --enable-nls --enable-bootstrap --enable-clocale=gnu
--enable-libstdcxx-debug --enable-libstdcxx-time=yes
--with-default-libstdcxx-abi=new --enable-gnu-unique-object
--disable-vtable-verify --enable-plugin --enable-default-pie --with-system-zlib
--enable-libphobos-checking=release --with-target-system-zlib=auto
--enable-objc-gc=auto --enable-multiarch --disable-werror --enable-cet
--with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32
--enable-multilib --with-tune=generic
--enable-offload-targets=nvptx-none=/build/gcc-11-R0liEm/gcc-11-11.2.0/debian/tmp-nvptx/usr,amdgcn-amdhsa=/build/gcc-11-R0liEm/gcc-11-11.2.0/debian/tmp-gcn/usr
--without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu
--host=x86_64-linux-gnu --target=x86_64-linux-gnu
--with-build-config=bootstrap-lto-lean --enable-link-serialization=8
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 11.2.0 (Debian 11.2.0-10) 
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-Wconversion' '-c' '-mtune=generic'
'-march=x86-64'
 /usr/lib/gcc/x86_64-linux-gnu/11/cc1 -E -quiet -v -imultiarch x86_64-linux-gnu
bug2.c -mtune=generic -march=x86-64 -Wconversion -fpch-preprocess
-fasynchronous-unwind-tables -o bug2.i
ignoring nonexistent directory "/usr/local/include/x86_64-linux-gnu"
ignoring nonexistent directory "/usr/lib/gcc/x86_64-linux-gnu/11/include-fixed"
ignoring nonexistent directory
"/usr/lib/gcc/x86_64-linux-gnu/11/../../../../x86_64-linux-gnu/include"
#include "..." search starts here:
#include <...> search starts here:
 /usr/lib/gcc/x86_64-linux-gnu/11/include
 /usr/local/include
 /usr/include/x86_64-linux-gnu
 /usr/include
End of search list.
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-Wconversion' '-c' '-mtune=generic'
'-march=x86-64'
 /usr/lib/gcc/x86_64-linux-gnu/11/cc1 -fpreprocessed bug2.i -quiet -dumpbase
bug2.c -dumpbase-ext .c -mtune=generic -march=x86-64 -Wconversion -version
-fasynchronous-unwind-tables -o bug2.s
GNU C17 (Debian 11.2.0-10) version 11.2.0 (x86_64-linux-gnu)
        compiled by GNU C version 11.2.0, GMP version 6.2.1, MPFR version
4.1.0, MPC version 1.2.1, isl version isl-0.24-GMP

GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
GNU C17 (Debian 11.2.0-10) version 11.2.0 (x86_64-linux-gnu)
        compiled by GNU C version 11.2.0, GMP version 6.2.1, MPFR version
4.1.0, MPC version 1.2.1, isl version isl-0.24-GMP

GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
Compiler executable checksum: 010de8893e7b0de1cd6903c6a1890e7a
bug2.c: In function ‘foo’:
bug2.c:7:8: warning: conversion from ‘int’ to ‘uint8_t’ {aka ‘unsigned char’}
may change value [-Wconversion]
    7 |   t |= f() & f(); // two rvalues: warn
      |        ^
bug2.c:8:8: warning: conversion from ‘int’ to ‘uint8_t’ {aka ‘unsigned char’}
may change value [-Wconversion]
    8 |   t &= f() & f(); // same with &=
      |        ^
bug2.c:13:8: warning: conversion from ‘int’ to ‘uint8_t’ {aka ‘unsigned char’}
may change value [-Wconversion]
   13 |   t |= f() & a;    // warning with one rvalue
      |        ^
bug2.c:14:8: warning: conversion from ‘int’ to ‘uint8_t’ {aka ‘unsigned char’}
may change value [-Wconversion]
   14 |   t |= a & f();    // no matter position.
      |        ^
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-Wconversion' '-c' '-mtune=generic'
'-march=x86-64'
 as -v --64 -o bug2.o bug2.s
GNU assembler version 2.37 (x86_64-linux-gnu) using BFD version (GNU Binutils
for Debian) 2.37
COMPILER_PATH=/usr/lib/gcc/x86_64-linux-gnu/11/:/usr/lib/gcc/x86_64-linux-gnu/11/:/usr/lib/gcc/x86_64-linux-gnu/:/usr/lib/gcc/x86_64-linux-gnu/11/:/usr/lib/gcc/x86_64-linux-gnu/
LIBRARY_PATH=/usr/lib/gcc/x86_64-linux-gnu/11/:/usr/lib/gcc/x86_64-linux-gnu/11/../../../x86_64-linux-gnu/:/usr/lib/gcc/x86_64-linux-gnu/11/../../../../lib/:/lib/x86_64-linux-gnu/:/lib/../lib/:/usr/lib/x86_64-linux-gnu/:/usr/lib/../lib/:/usr/lib/gcc/x86_64-linux-gnu/11/../../../:/lib/:/usr/lib/
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-Wconversion' '-c' '-mtune=generic'
'-march=x86-64'

[Bug c/103881] Wconversion false positive when using |= and &= with two rvalues in binary op

[thomas at habets dot se]

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

--- Comment #1 from thomas at habets dot se ---
I just reproduced this from git HEAD, as seen on github (commit
e3cbb8c66c930ba738674b0fcf29848dc3ecfef2), with latest commit today,
2021-12-31.

[Bug c/103881] Wconversion false positive when using |= and &= with two rvalues in binary op

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
See C99 6.3.1.1/2 and 6.3.1.8, yes, in all cases of |=, &=, |, & the uint8_t
operands are promoted to int.
And the reason why we don't warn for the simple binary operations is that those
common cases have special cases for the warnings, while if you have more
complex expressions you get warnings.  This is a FE warning, value range
propagation etc. doesn't happen at that point...

[Bug c/103881] Wconversion false positive when using |= and &= with two rvalues in binary op

[thomas at habets dot se]

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

--- Comment #3 from thomas at habets dot se ---
Interesting.

So the difference between "x |= a & a" and "x |= f() & f()" is that the latter
has passed a somewhat arbitrary level of complexity after which GCC is not able
to prove that it's safe, and therefore warns as it being potentially losing
precision?

It's understandable, but unfortunate. It means that I have no hope of having
real world programs be free of false positives for conversion warnings.

[Bug c/103881] Wconversion false positive when using |= and &= with two rvalues in binary op

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:20e4a5e573e76f4379b353cc736215a5f10cdb84

commit r12-6486-g20e4a5e573e76f4379b353cc736215a5f10cdb84
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Jan 11 19:11:51 2022 +0100

    c-family: Fix up -W*conversion on bitwise &/|/^ [PR101537]

    The following testcases emit a bogus -Wconversion warning.  This is because
    conversion_warning function doesn't handle BIT_*_EXPR (only
unsafe_conversion_p
    that is called during the default: case, and that one doesn't handle
    SAVE_EXPRs added because the unsigned char & or | operands promoted to int
    have side-effects and =| or =& is used.

    The patch handles BIT_IOR_EXPR/BIT_XOR_EXPR like the last 2 operands of
    COND_EXPR by recursing on the two operands, if either of them doesn't fit
    into the narrower type, complain.  BIT_AND_EXPR too, but first it needs to
    handle some special cases that unsafe_conversion_p does, namely when one
    of the two operands is a constant.

    This fixes completely the pr101537.c test and for C also pr103881.c
    and doesn't regress anything in the testsuite, for C++ pr103881.c still
    emits the bogus warnings.
    This is because while the C FE emits in that case a SAVE_EXPR that
    conversion_warning can handle already, C++ FE emits
    TARGET_EXPR <D.whatever, ...>, something | D.whatever
    etc. and conversion_warning handles COMPOUND_EXPR by "recursing" on the
    rhs.  To handle that case, we'd need for TARGET_EXPR on the lhs remember
    in some hash map the mapping from D.whatever to the TARGET_EXPR and when
    we see D.whatever, use corresponding TARGET_EXPR initializer instead.

    2022-01-11  Jakub Jelinek  <jakub@redhat.com>

            PR c/101537
            PR c/103881
    gcc/c-family/
            * c-warn.c (conversion_warning): Handle BIT_AND_EXPR, BIT_IOR_EXPR
            and BIT_XOR_EXPR.
    gcc/testsuite/
            * c-c++-common/pr101537.c: New test.
            * c-c++-common/pr103881.c: New test.

[Bug c/103881] Wconversion false positive when using |= and &= with two rvalues in binary op

[egallager at gcc dot gnu.org]

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

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=40752,
                   |                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=12411
                 CC|                            |egallager at gcc dot gnu.org

--- Comment #5 from Eric Gallager <egallager at gcc dot gnu.org> ---
(In reply to thomas from comment #3)
> Interesting.
> 
> So the difference between "x |= a & a" and "x |= f() & f()" is that the
> latter has passed a somewhat arbitrary level of complexity after which GCC
> is not able to prove that it's safe, and therefore warns as it being
> potentially losing precision?
> 
> It's understandable, but unfortunate. It means that I have no hope of having
> real world programs be free of false positives for conversion warnings.

The latter looks like something that ought to get a -Wsequence-point warning
anyways, at least per bug 12411... but then again that one was closed as
WONTFIX, so never mind...

[Bug c/103881] Wconversion false positive when using |= and &= with two rvalues in binary op

[thomas at habets dot se]

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

--- Comment #6 from thomas at habets dot se ---
The sequence point is a bit off topic, but I can't imagine that it's the
intention to warn about `f() & f()`.

Per gcc docs it's supposed to "Warn about code that may have undefined
semantics because of violations of sequence point rules in the C and C++
standards […] Programs whose behavior depends on this have undefined behavior".

I'm not a language lawyer, but I can't imagine that it's the intention to warn
about e.g. `printf("called with %f: %s", sqrt(2), strerror(errno));`. Nor does
it sound helpful to do so.

Nor `x = get_value() & admin_bits();`. There's just no wrong but technically
conforming way to miscompile that.

[Bug c/103881] Wconversion false positive when using |= and &= with two rvalues in binary op

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:777b73e45983b11e010bd5192185ad01af444de4

commit r11-9500-g777b73e45983b11e010bd5192185ad01af444de4
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Jan 11 19:11:51 2022 +0100

    c-family: Fix up -W*conversion on bitwise &/|/^ [PR101537]

    The following testcases emit a bogus -Wconversion warning.  This is because
    conversion_warning function doesn't handle BIT_*_EXPR (only
unsafe_conversion_p
    that is called during the default: case, and that one doesn't handle
    SAVE_EXPRs added because the unsigned char & or | operands promoted to int
    have side-effects and =| or =& is used.

    The patch handles BIT_IOR_EXPR/BIT_XOR_EXPR like the last 2 operands of
    COND_EXPR by recursing on the two operands, if either of them doesn't fit
    into the narrower type, complain.  BIT_AND_EXPR too, but first it needs to
    handle some special cases that unsafe_conversion_p does, namely when one
    of the two operands is a constant.

    This fixes completely the pr101537.c test and for C also pr103881.c
    and doesn't regress anything in the testsuite, for C++ pr103881.c still
    emits the bogus warnings.
    This is because while the C FE emits in that case a SAVE_EXPR that
    conversion_warning can handle already, C++ FE emits
    TARGET_EXPR <D.whatever, ...>, something | D.whatever
    etc. and conversion_warning handles COMPOUND_EXPR by "recursing" on the
    rhs.  To handle that case, we'd need for TARGET_EXPR on the lhs remember
    in some hash map the mapping from D.whatever to the TARGET_EXPR and when
    we see D.whatever, use corresponding TARGET_EXPR initializer instead.

    2022-01-11  Jakub Jelinek  <jakub@redhat.com>

            PR c/101537
            PR c/103881
    gcc/c-family/
            * c-warn.c (conversion_warning): Handle BIT_AND_EXPR, BIT_IOR_EXPR
            and BIT_XOR_EXPR.
    gcc/testsuite/
            * c-c++-common/pr101537.c: New test.
            * c-c++-common/pr103881.c: New test.

    (cherry picked from commit 20e4a5e573e76f4379b353cc736215a5f10cdb84)

[Bug c/103881] Wconversion false positive when using |= and &= with two rvalues in binary op

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:4e8c32ecfb87e723229dc6c08df8ea602e947f1e

commit r10-10666-g4e8c32ecfb87e723229dc6c08df8ea602e947f1e
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Jan 11 19:11:51 2022 +0100

    c-family: Fix up -W*conversion on bitwise &/|/^ [PR101537]

    The following testcases emit a bogus -Wconversion warning.  This is because
    conversion_warning function doesn't handle BIT_*_EXPR (only
unsafe_conversion_p
    that is called during the default: case, and that one doesn't handle
    SAVE_EXPRs added because the unsigned char & or | operands promoted to int
    have side-effects and =| or =& is used.

    The patch handles BIT_IOR_EXPR/BIT_XOR_EXPR like the last 2 operands of
    COND_EXPR by recursing on the two operands, if either of them doesn't fit
    into the narrower type, complain.  BIT_AND_EXPR too, but first it needs to
    handle some special cases that unsafe_conversion_p does, namely when one
    of the two operands is a constant.

    This fixes completely the pr101537.c test and for C also pr103881.c
    and doesn't regress anything in the testsuite, for C++ pr103881.c still
    emits the bogus warnings.
    This is because while the C FE emits in that case a SAVE_EXPR that
    conversion_warning can handle already, C++ FE emits
    TARGET_EXPR <D.whatever, ...>, something | D.whatever
    etc. and conversion_warning handles COMPOUND_EXPR by "recursing" on the
    rhs.  To handle that case, we'd need for TARGET_EXPR on the lhs remember
    in some hash map the mapping from D.whatever to the TARGET_EXPR and when
    we see D.whatever, use corresponding TARGET_EXPR initializer instead.

    2022-01-11  Jakub Jelinek  <jakub@redhat.com>

            PR c/101537
            PR c/103881
    gcc/c-family/
            * c-warn.c (conversion_warning): Handle BIT_AND_EXPR, BIT_IOR_EXPR
            and BIT_XOR_EXPR.
    gcc/testsuite/
            * c-c++-common/pr101537.c: New test.
            * c-c++-common/pr103881.c: New test.

    (cherry picked from commit 20e4a5e573e76f4379b353cc736215a5f10cdb84)

[Bug c/103881] Wconversion false positive when using |= and &= with two rvalues in binary op

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 10.4 as well, not going to backport it further.