[Bug c/104680] New: identical inner condition not detected

[Bug c/104680] New: identical inner condition not detected

[dcb314 at hotmail dot com]

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

            Bug ID: 104680
           Summary: identical inner condition not detected
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dcb314 at hotmail dot com
  Target Milestone: ---

For this C++ code, with a identical inner condition:

extern void g( int);

void f( int n)
{
        if (n > 31)
        {
                if (n > 31)
                {
                        g( 2 * n);
                }
        }
        else
                g( n);
}

g++ currently has nothing to say:

$ /home/dcb/gcc/results/bin/g++ -c -g -O2 -Wall -Wextra -Wduplicated-branches
feb24e.cc
$ 

cppcheck managed to find the problem, usually a result of
sloppy cut'n'paste:

feb24e.cc:8:9: warning: Identical inner 'if' condition is always true.
[identicalInnerCondition]
  if (n > 31)
        ^
feb24e.cc:6:8: note: outer condition: n>31
 if (n > 31)
       ^
feb24e.cc:8:9: note: identical inner condition: n>31
  if (n > 31)
        ^
Here is a list of all the places in the gcc source code where this
problem occurs:

trunk.git/gcc/config/avr/avr.cc:8674:22: warning: Identical inner 'if'
condition is always true. [identicalInnerCondition]
trunk.git/gcc/config/mn10300/mn10300.cc:888:8: warning: Identical inner 'if'
condition is always true. [identicalInnerCondition]
trunk.git/gcc/d/expr.cc:689:17: warning: Identical inner 'if' condition is
always true. [identicalInnerCondition]
trunk.git/libffi/src/m32r/ffi.c:66:15: warning: Identical inner 'if' condition
is always true. [identicalInnerCondition]
trunk.git/liboffloadmic/runtime/offload_engine.cpp:113:13: warning: Identical
inner 'if' condition is always true. [identicalInnerCondition]
trunk.git/zlib/contrib/minizip/zip.c:1212:26: warning: Identical inner 'if'
condition is always true. [identicalInnerCondition]

There are also 67 cases of this in the current linux kernel,
so it looks like it might be a real problem.

[Bug c/104680] identical inner condition not detected

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement
           Keywords|                            |diagnostic

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Wduplicated-branches only currently works for the a && a case and not the if
(a) if (a) case.

Maybe this could be done by the analyzer instead.

[Bug analyzer/104680] identical inner condition not detected

[dmalcolm at gcc dot gnu.org]

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

--- Comment #2 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
> trunk.git/gcc/config/avr/avr.cc:8674:22: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]

In avr_out_fract:

8665   │           /* We need to consider to-be-discarded bits
8666   │              if the value is negative.  */
8667   │           if (sn < s0)
8668   │             {
8669   │               avr_asm_len ("tst %0" CR_TAB
8670   │                            "brpl 0f",
8671   │                            &all_regs_rtx[src.regno_msb], plen, 2);
8672   │               /* Test to-be-discarded bytes for any nozero bits.
8673   │                  ??? Could use OR or SBIW to test two registers at
once.  */
8674   │               if (sn < s0)
8675   │                 avr_asm_len ("cp %0,__zero_reg__", &all_regs_rtx[sn],
plen, 1);
8676   │ 

sn < s0 is checked at line 8667, then again at line 8675.

[Bug analyzer/104680] identical inner condition not detected

[dmalcolm at gcc dot gnu.org]

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

--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
> trunk.git/gcc/config/mn10300/mn10300.cc:888:8: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]

In mn10300_expand_prologue:

 877   │       /* Consider alternative save_a0_merge only if we don't need a
 878   │      frame pointer, size is nonzero and the user hasn't
 879   │      changed the calling conventions of a0.  */
 880   │       if (! frame_pointer_needed && size
 881   │       && call_used_regs[FIRST_ADDRESS_REGNUM]
 882   │       && ! fixed_regs[FIRST_ADDRESS_REGNUM])
 883   │     {
 884   │       /* Insn: add -(size + 4 * num_regs_to_save), sp.  */
 885   │       this_strategy_size = SIZE_ADD_SP (-(size + 4 *
num_regs_to_save));
 886   │       /* Insn: mov sp, a0.  */
 887   │       this_strategy_size++;
 888   │       if (size)
 889   │         {
 890   │           /* Insn: add size, a0.  */
 891   │           this_strategy_size += SIZE_ADD_AX (size);
 892   │         }

"size" is checked at line 880, then again at line 888.

[Bug analyzer/104680] identical inner condition not detected

[dmalcolm at gcc dot gnu.org]

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

--- Comment #4 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
> trunk.git/gcc/d/expr.cc:689:17: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]

In 'void visit (CatExp *e)':
 682   │     if (e->e1->op == EXP::concatenate)
 683   │       {
 684   │     /* Flatten multiple concatenations to an array.
 685   │        So the expression ((a ~ b) ~ c) becomes [a, b, c]  */
 686   │     int ndims = 2;
 687   │ 
 688   │     for (Expression *ex = e->e1; ex->op == EXP::concatenate;)
 689   │       {
 690   │         if (ex->op == EXP::concatenate)
 691   │           {
 692   │         ex = ex->isCatExp ()->e1;
 693   │         ndims++;
 694   │           }
 695   │       }

Looks like the ex->op == EXP::concatenate in line 690 is indeed checked by the
loop guard at line 688, so this code does look suspicious to me.

[Bug analyzer/104680] identical inner condition not detected

[dmalcolm at gcc dot gnu.org]

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

--- Comment #5 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
> trunk.git/libffi/src/m32r/ffi.c:66:15: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]

In ffi_prep_args:

  56   │   for (i = ecif->cif->nargs, p_arg = ecif->cif->arg_types;
  57   │        (i != 0) && (avn != 0);
  58   │        i--, p_arg++)
  59   │     {
  60   │       size_t z;
  61   │ 
  62   │       /* Align if necessary.  */
  63   │       if (((*p_arg)->alignment - 1) & (unsigned) argp)
  64   │     argp = (char *) FFI_ALIGN (argp, (*p_arg)->alignment);
  65   │ 
  66   │       if (avn != 0) 
  67   │     {

avn != 0 checked at both line 66 and in the loop guard at line 57.

[Bug analyzer/104680] identical inner condition not detected

[dmalcolm at gcc dot gnu.org]

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

--- Comment #6 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
> trunk.git/liboffloadmic/runtime/offload_engine.cpp:113:13: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]

 108   │ void Engine::init(void)
 109   │ {
 110   │     if (!m_ready) {
 111   │         mutex_locker_t locker(m_lock);
 112   │ 
 113   │         if (!m_ready) {

Possible false positive? (multithreaded code?)

[Bug analyzer/104680] identical inner condition not detected

[dmalcolm at gcc dot gnu.org]

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

--- Comment #7 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
> trunk.git/zlib/contrib/minizip/zip.c:1212:26: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]

In zipOpenNewFileInZip4_64:

1206   │ #ifdef HAVE_BZIP2
1207   │     if ((err==ZIP_OK) && (zi->ci.method == Z_DEFLATED || zi->ci.method
== Z_BZIP2ED) && (!zi->ci.raw))
1208   │ #else
1209   │     if ((err==ZIP_OK) && (zi->ci.method == Z_DEFLATED) &&
(!zi->ci.raw))
1210   │ #endif
1211   │     {
1212   │         if(zi->ci.method == Z_DEFLATED)
1213   │         {

Arguably a false positive: it's going to be a repeated test if the preprocessor
uses line 1209, but not if it uses line 1207.

[Bug c/104680] identical inner condition not detected

[dmalcolm at gcc dot gnu.org]

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

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|analyzer                    |c
           Assignee|dmalcolm at gcc dot gnu.org        |unassigned at gcc dot gnu.org

--- Comment #8 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
There are two aspects to this bug:

(a) the code flagged by cppcheck's implementation of "identicalInnerCondition"
warnings from cppcheck when run on GCC itself.  I've posted comments above
giving some details on each of these.

(b) the fact that GCC's -Wduplicated-branches doesn't flag these issues, when
arguably it should (though note the possible false positives above).


Re (a), should these be opened as bugs against individual subsystems?

Re (b) and in reply to Andrew Pinski from comment #1):
> Wduplicated-branches only currently works for the a && a case and not the if
> (a) if (a) case.
> 
> Maybe this could be done by the analyzer instead.

...I don't think this is a good fit for the analyzer; it seems much more
appropriate for the frontends to me; reassigning component to "c".

Hope this is constructive.

[Bug c/104680] identical inner condition not detected

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
At least right now the analyzer might be too late for this though, no?  There
could be various optimizations before it that make it hard to figure out what
was actually user code and what is something else.
On the other side, doing this in the FEs would handle only the most trivial
cases, any time there would be some function call or inline asm or whatever
else could have possibly changed any values in the expressions, we'd need to
punt.
So, at least having a SSA form so that we can find out what is the same value
and what is (possibly) different would be helpful, likely with at least a
forwprop.

[Bug c/104680] identical inner condition not detected

[egallager at gcc dot gnu.org]

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

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

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

--- Comment #10 from Eric Gallager <egallager at gcc dot gnu.org> ---
This seems like basically the opposite of bug 82100


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89863
[Bug 89863] [meta-bug] Issues in gcc that other static analyzers (cppcheck,
clang-static-analyzer, PVS-studio) find that gcc misses