[Bug middle-end/89337] Bogus "exceeds maximum object size" on unreachable code

2019-02-18 Thread msebor at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337

--- Comment #12 from Martin Sebor  ---
One of the approaches we have been discussing is replacing these invalid calls
with __builtin_trap or __builtin_unreachable, perhaps optionally preceded by
__builtin_warning ("specified size exceeds maximum object size") which would be
issued if the code wasn't eliminated.  Command line options controlling the
choices would be provided.  This would work in some cases but probably not
those where the invalid value ends up constant-propagated after the new
statement has been introduced.  It's something to investigate/prototype for GCC
10.

[Bug middle-end/89337] Bogus "exceeds maximum object size" on unreachable code

2019-02-18 Thread rguenth at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337

--- Comment #11 from Richard Biener  ---
(In reply to Rafael Avila de Espindola from comment #10)
> Maybe we should have a general flag that disables all warnings where gcc
> cannot prove that there is a path from a function entry to the broken
> statement?

But what about proving there is a path from program entry to the function?

And now reason why those two are different.

[Bug middle-end/89337] Bogus "exceeds maximum object size" on unreachable code

2019-02-15 Thread rafael at espindo dot la
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337

--- Comment #10 from Rafael Avila de Espindola  ---
(In reply to Martin Sebor from comment #9)
> The warning is very simple: it just looks for excessive sizes in calls
> emitted in the optimized IL.  When the call is there (either because it's in
> the source code as is or because it's been synthesized by GCC based on the
> invariants it can infer from the code) it triggers.  It runs after all
> high-level optimizations, including DCE, and assumes that if the statement
> is in the IL it is reachable.  Compiling the test case with
> -fdump-tree-optimized=/dev/stdout shows the GIMPLE the warning works with:
> 
>[local count: 233860936]:
>   # iftmp.6_113 = PHI 
>   memset (iftmp.6_113, 0, 18446744073709551613);
> 
> I think the issue can essentially be reduced to the following:
> 
> $ cat z.c && gcc -O2 -S -fdump-tree-optimized=/dev/stdout z.c
> void f (char *d, const char *s)
> {
>   if (__builtin_strstr (s, "ABC"))
> {
>   __SIZE_TYPE__ n = __builtin_strlen (s) - 3;
> 
>   if (n > __builtin_strlen (s))   // cannot be true
> __builtin_memset (d, 0, n - __builtin_strlen (s));
>   }
> }
> 
> ;; Function f (f, funcdef_no=0, decl_uid=1907, cgraph_uid=1, symbol_order=0)
> 
> Removing basic block 6
> Removing basic block 7
> f (char * d, const char * s)
> {
>   char * _1;
>   long unsigned int _2;
> 
>[local count: 1073741824]:
>   _1 = __builtin_strstr (s_5(D), "ABC");
>   if (_1 != 0B)
> goto ; [70.00%]
>   else
> goto ; [30.00%]
> 
>[local count: 751619278]:
>   _2 = __builtin_strlen (s_5(D));
>   if (_2 <= 2)
> goto ; [33.00%]
>   else
> goto ; [67.00%]
> 
>[local count: 248034361]:
>   __builtin_memset (d_6(D), 0, 18446744073709551613); [tail call]
> 
>[local count: 1073741824]:
>   return;
> 
> }
> 
> 
> z.c: In function ‘f’:
> z.c:8:9: warning: ‘__builtin_memset’ specified size 18446744073709551613
> exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]
> 8 | __builtin_memset (d, 0, n - __builtin_strlen (s));
>   | ^
> 
> GCC doesn't have the smarts to figure out that when s contains a substring
> of length 3 then strlen(s) must be at least 3.  As a result, it doesn't
> eliminate the memset call in the function and the warning triggers.  Suppose
> we teach GCC how to figure this out from the strstr call (which might be a
> useful optimization) and then someone comes along with a test case that uses
> strspn() instead of strstr().  We can also teach GCC about strstr() but then
> someone else might use regcomp() or some user-defined function that we won't
> have access to.  At some point we'll have to call it good enough.

Absolutely, this is the halting problem after all.

The question is not to give up or not, it is whether to warn when we do. If we
do, we get potential warnings every time gcc gives up on solving the halting
problem.

The uninitialized variable warning was at least split in two, one for the cases
where gcc is not sure.

Maybe we should have a general flag that disables all warnings where gcc cannot
prove that there is a path from a function entry to the broken statement?

[Bug middle-end/89337] Bogus "exceeds maximum object size" on unreachable code

2019-02-14 Thread msebor at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337

--- Comment #9 from Martin Sebor  ---
The warning is very simple: it just looks for excessive sizes in calls emitted
in the optimized IL.  When the call is there (either because it's in the source
code as is or because it's been synthesized by GCC based on the invariants it
can infer from the code) it triggers.  It runs after all high-level
optimizations, including DCE, and assumes that if the statement is in the IL it
is reachable.  Compiling the test case with -fdump-tree-optimized=/dev/stdout
shows the GIMPLE the warning works with:

   [local count: 233860936]:
  # iftmp.6_113 = PHI 
  memset (iftmp.6_113, 0, 18446744073709551613);

I think the issue can essentially be reduced to the following:

$ cat z.c && gcc -O2 -S -fdump-tree-optimized=/dev/stdout z.c
void f (char *d, const char *s)
{
  if (__builtin_strstr (s, "ABC"))
{
  __SIZE_TYPE__ n = __builtin_strlen (s) - 3;

  if (n > __builtin_strlen (s))   // cannot be true
__builtin_memset (d, 0, n - __builtin_strlen (s));
  }
}

;; Function f (f, funcdef_no=0, decl_uid=1907, cgraph_uid=1, symbol_order=0)

Removing basic block 6
Removing basic block 7
f (char * d, const char * s)
{
  char * _1;
  long unsigned int _2;

   [local count: 1073741824]:
  _1 = __builtin_strstr (s_5(D), "ABC");
  if (_1 != 0B)
goto ; [70.00%]
  else
goto ; [30.00%]

   [local count: 751619278]:
  _2 = __builtin_strlen (s_5(D));
  if (_2 <= 2)
goto ; [33.00%]
  else
goto ; [67.00%]

   [local count: 248034361]:
  __builtin_memset (d_6(D), 0, 18446744073709551613); [tail call]

   [local count: 1073741824]:
  return;

}


z.c: In function ‘f’:
z.c:8:9: warning: ‘__builtin_memset’ specified size 18446744073709551613
exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]
8 | __builtin_memset (d, 0, n - __builtin_strlen (s));
  | ^

GCC doesn't have the smarts to figure out that when s contains a substring of
length 3 then strlen(s) must be at least 3.  As a result, it doesn't eliminate
the memset call in the function and the warning triggers.  Suppose we teach GCC
how to figure this out from the strstr call (which might be a useful
optimization) and then someone comes along with a test case that uses strspn()
instead of strstr().  We can also teach GCC about strstr() but then someone
else might use regcomp() or some user-defined function that we won't have
access to.  At some point we'll have to call it good enough.

It's helpful to keep in mind that no control flow or data flow-based warning is
free of false positives (or negatives), either in GCC or in any static
analyzer.  Analyzing only provably reachable code would mean not diagnosing
bugs in translation units without main because we cannot prove that they're
ever called.  Similarly, at the function level, it would mean not diagnosing
bugs in conditional statements unless the conditions could be proven to be true
at compile time.  That would make not only the false positive rate nearly zero,
it would also make the true positive rate nearly zero. In effect, it would make
the diagnostics virtually useless, able to detect only bugs in the simplest
code.  If you find a non-zero rate of false positives unacceptable then my
suggestion is to turn warnings off.  Not just this one but all others as most
have some false positives, including the front-ends ones that have no notion of
reachability.  Otherwise, if you accept that some false positives are
unavoidable and worth the checking GCC does, then until the strstr optimization
above is implemented, I would recommend to make use of __builtin_unreachable():

  void trim_asterisk(seastar::sstring ) {
if (name.find("ABC") != seastar::sstring::npos) {
  if (name.length () < 3)
__builtin_unreachable ();
  name.resize(name.length() - 3);
}
  }

Chances are that besides avoiding the warning it will also improve the quality
of the object code.  When hidden behind a well-named macro it might also
improve readability.

[Bug middle-end/89337] Bogus "exceeds maximum object size" on unreachable code

2019-02-14 Thread rafael at espindo dot la
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337

Rafael Avila de Espindola  changed:

   What|Removed |Added

 Status|RESOLVED|UNCONFIRMED
 Resolution|DUPLICATE   |---

--- Comment #8 from Rafael Avila de Espindola  ---
The previous testcase passes on trunk, but the original doesn't.

The original has a "if (boost::algorithm::ends_with(name, "%2A"))" instead of a
direct "if(name.size() > 3)".

The reduced testcase uses a find to avoid boost::algorithm and keep the
testcase size sane.

In the end it seems that gcc is doing an heroic effort to understand the code
and issue an warning if it can't prove that the bogus memset is not reached. I
don't think that is workable in practice (for sure not in theory): There is
always something that is obvious to the developer but not to gcc.

IMHO gcc should be issuing warnings only when it shows that a statement can be
reached, not when it fails to show the statement cannot be reached.

[Bug middle-end/89337] Bogus "exceeds maximum object size" on unreachable code

2019-02-14 Thread rafael at espindo dot la
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337

Rafael Avila de Espindola  changed:

   What|Removed |Added

  Attachment #45710|0   |1
is obsolete||

--- Comment #7 from Rafael Avila de Espindola  ---
Created attachment 45726
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=45726=edit
testcase that fails on trunk

[Bug middle-end/89337] Bogus "exceeds maximum object size" on unreachable code

2019-02-13 Thread msebor at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337

Martin Sebor  changed:

   What|Removed |Added

 Status|UNCONFIRMED |RESOLVED
 Resolution|--- |DUPLICATE

--- Comment #6 from Martin Sebor  ---
Thank you for the updated test case.  The original looked similar to bug 86153
to me at first but then I decided it was different.  The new test case is very
close to the one in that bug, and is not diagnosed with the top of trunk
anymore thanks to the fix in r267252.  I think it's safe to resolve this as a
duplicate of that bug.  Let me add your test case to the test suite.  (If the
warning persists in your code even with the latest GCC 9.0 please reopen the
bug with an updated test case.)

*** This bug has been marked as a duplicate of bug 86153 ***

[Bug middle-end/89337] Bogus "exceeds maximum object size" on unreachable code

2019-02-13 Thread rafael at espindo dot la
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337

Rafael Avila de Espindola  changed:

   What|Removed |Added

 Status|RESOLVED|UNCONFIRMED
 Resolution|INVALID |---

--- Comment #5 from Rafael Avila de Espindola  ---
Reopening with an expanded testcase.

[Bug middle-end/89337] Bogus "exceeds maximum object size" on unreachable code

2019-02-13 Thread rafael at espindo dot la
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337

Rafael Avila de Espindola  changed:

   What|Removed |Added

  Attachment #45704|0   |1
is obsolete||

--- Comment #4 from Rafael Avila de Espindola  ---
Created attachment 45710
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=45710=edit
testcase where the string size should still be visible to the compiler

[Bug middle-end/89337] Bogus "exceeds maximum object size" on unreachable code

2019-02-13 Thread rafael at espindo dot la
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337

--- Comment #3 from Rafael Avila de Espindola  ---

> GCC can't see that drop3() cannot be called with name.size() < 3, and in
> resize, the condition (n > size()) can only be true only when name.size() <
> 3 so n - size() is unavoidably too large.

That is why gcc should not warn about it :-)

The original testcase had

  if (boost::algorithm::ends_with(name, "%2A")) {
  name.resize(name.length() - 3);

I think it is fair to say that:

* The code is valid, if something ends with "%2A" the size is >= 3.
* GCC should not be required to figure out the above implication.
* The developer should not be required to add a __builtin_unreachable() to
avoid a warning in the above code.

I will re reduce the testcase keeping the ends_with and attach.

[Bug middle-end/89337] Bogus "exceeds maximum object size" on unreachable code

2019-02-13 Thread msebor at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337

Martin Sebor  changed:

   What|Removed |Added

   Keywords||diagnostic
 Status|UNCONFIRMED |RESOLVED
 CC||msebor at gcc dot gnu.org
 Blocks||88443
 Resolution|--- |INVALID

--- Comment #2 from Martin Sebor  ---
In the test case from attachment 45704:

  struct sstring {
size_t _size;
sstring(size_t size) : _size(size) { memset(begin(), '\n', size); }
size_t size() const noexcept { return _size; }
void resize(size_t n) {
  if (n > size()) {
sstring x(n - size());
  }
}
char *begin();
  };
  void drop3(sstring ) { name.resize(name.size() - 3); }

GCC can't see that drop3() cannot be called with name.size() < 3, and in
resize, the condition (n > size()) can only be true only when name.size() < 3
so n - size() is unavoidably too large.

To avoid the warning make the precondition explicit:

  void drop3(sstring )
  {
if (name.size () < 3)
__builtin_unreachable ();

name.resize(name.size() - 3);
  }


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88443
[Bug 88443] [meta-bug] bogus/missing -Wstringop-overflow warnings

[Bug middle-end/89337] Bogus "exceeds maximum object size" on unreachable code

2019-02-13 Thread rafael at espindo dot la
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337

--- Comment #1 from Rafael Avila de Espindola  ---
The original testcase is from https://github.com/scylladb/seastar/issues/598