[Bug middle-end/97336] New: False positive -Wstring-compare warning for strncmp()

[Bug middle-end/97336] New: False positive -Wstring-compare warning for strncmp()

[franke at computer dot org]

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

            Bug ID: 97336
           Summary: False positive -Wstring-compare warning for strncmp()
           Product: gcc
           Version: 10.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: franke at computer dot org
  Target Milestone: ---

Testcase:

$ uname -srvmo
CYGWIN_NT-10.0 3.1.7(0.340/5/3) 2020-08-22 17:48 x86_64 Cygwin

$ cygcheck -f /usr/bin/gcc
gcc-core-10.2.0-1

$ gcc --version
gcc (GCC) 10.2.0
...

$ cat test.c
int f(const char * p, int n)
{
  char buf[10] = {0, };
  int i;
  for (i = 0; i < (int)sizeof(buf)-1 && i < n; i++)
    buf[i] = p[i];
  if (!__builtin_strncmp(buf, "12345", 5)
      && (i == 5 || buf[5] == ' '))
    return 1;
  return 0;
}

$ gcc -Wstring-compare -O2 -c test.c
test.c: In function ‘f’:
test.c:7:8: warning: ‘__builtin_strncmp’ of strings of length 0 and 5 and bound
of 5 evaluates to nonzero [-Wstring-compare]
    7 |   if (!__builtin_strncmp(buf, "12345", 5)
      |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Apparently it is assumed that the copy loop is never executed.

Any of the following single line changes removes the warning:

 {
-  char buf[10] = {0, };
+  char buf[10]; __builtin_memset(buf, 0, sizeof(buf));
   int i;


   if (!__builtin_strncmp(buf, "12345", 5)
-      && (i == 5 || buf[5] == ' '))
+      && (buf[5] == ' ' || i == 5))
     return 1;


   if (!__builtin_strncmp(buf, "12345", 5)
-      && (i == 5 || buf[5] == ' '))
+      && (!buf[5] || buf[5] == ' '))
     return 1;

[Bug middle-end/97336] False positive -Wstring-compare warning for strncmp()

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

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

--- Comment #1 from Martin Sebor <msebor at gcc dot gnu.org> ---
The warning works as intended: unless n > 4, the strncmp call will return
nonzero because the length of buf is less than 5.  GCC partially unrolls the
loop and the first iteration of it is what triggers the warning.  It disappears
when the buffer is cleared with memset only because GCC (as a limitation) loses
track of the length of the string in buf after that.  Handling the case when n
is zero or less (e.g., via if (n <= 0) return 0;) does as well.

The IL that leads up to the warning can be seen in the output of the
-fdump-tree-dom4 option (below).

f (const char * p, int n)
{
  sizetype ivtmp.6;
  int i;
  char buf[10];
  char _3;
  _Bool _4;
  _Bool _5;
  _Bool _6;
  int _7;
  int _10;
  char prephitmp_20;
  int _21;
  unsigned int _25;
  char pretmp_33;

  <bb 2> [local count: 118111600]:
  buf = "";                                     <<< strlen(buf) == 0
  if (n_15(D) > 0)
    goto <bb 11>; [89.00%]
  else
    goto <bb 4>; [11.00%]

  ...
  <bb 4> [local count: 12992276]:               <<< n == 0
  _21 = __builtin_strncmp (&buf, "12345", 5);   <<< warning here
  if (_21 == 0)                                 <<< folded to false
    goto <bb 5>; [50.00%]
  else
    goto <bb 6>; [50.00%]

  <bb 5> [local count: 6496138]:
  # prephitmp_20 = PHI <0(4)>

  <bb 6> [local count: 95940523]:
  goto <bb 10>; [100.00%]

  <bb 7> [local count: 105119324]:
  _7 = __builtin_strncmp (&buf, "12345", 5);    <<< second strncmp call
  if (_7 == 0)
    goto <bb 8>; [50.00%]
  else
    goto <bb 6>; [50.00%]

  <bb 8> [local count: 52559662]:
  if (i_19 == 5)
    goto <bb 10>; [23.93%]
  else
    goto <bb 9>; [76.07%]

  <bb 9> [local count: 40175661]:
  pretmp_33 = buf[5];
  if (pretmp_33 == 32)
    goto <bb 10>; [25.01%]
  else
    goto <bb 6>; [74.99%]

  <bb 10> [local count: 118111600]:
  # _10 = PHI <1(9), 0(6), 1(8)>
  buf ={v} {CLOBBER};
  return _10;

}


Rewriting the code in a way that avoids the loop (e.g., by using memcpy
instead) also avoids the warning.

[Bug middle-end/97336] False positive -Wstring-compare warning for strncmp()

[franke at computer dot org]

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

--- Comment #2 from Christian Franke <franke at computer dot org> ---
Sorry, but I disagree that this report is INVALID.

Unlike for example -Wstringop-overflow, warnings like -Wstring-compare should
IMO only occur if the warning condition holds for *all* function calls
generated by loop unrolling.

BTW, if the partial loop unrolling is done manually, the warning should occur,
but does not:

int f(const char * p, int n)
{
  char buf[10] = {0, };
  int i = 0;
  if (n <= 0) {
    if (!__builtin_strncmp(buf, "12345", 5) // <== no warning
        && (i == 5 || buf[5] == ' ')) // <== warning if removed
      return 1;
  }
  else {
    do {
      buf[i] = p[i]; i++;
    } while (i < (int)sizeof(buf)-1 && i < n);
    if (!__builtin_strncmp(buf, "12345", 5)
        && (i == 5 || buf[5] == ' '))
      return 1;
  }
  return 0;
}

With the above code, the warning occurs if the condition "&& (i == 5 || buf[5]
== ' ')" is removed. If this is done in the original testcase, the warning does
no longer occur.

[Bug middle-end/97336] False positive -Wstring-compare warning for strncmp()

[msebor at gcc dot gnu.org]

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

--- Comment #3 from Martin Sebor <msebor at gcc dot gnu.org> ---
For better or worse, in the internal representation there's no way to
differentiate a call that was synthesized during loop unrolling (or any other
similar transformation) from the original call in the source code, or even tell
whether or not loop unrolling or any other such transformation took place.

It also helps to keep in mind that the documented purpose of warnings in GCC
"is to report constructions that are not inherently erroneous but that are
risky or suggest there may have been an error."  If the test case in comment #0
is representative of real code then even if it doesn't indicate a bug I would
suggest to view the warning as an indication that the code can be improved. 
(This is often true for all of these flow-based warnings.) 

In the test case in comment #3 the body of the true branch of the if statement
is eliminated early on because the condition (i == 5 || buf[5] == ' ') is known
to be false.