Ugh, -Wstringop-overflow is enabled by default even without -Wall?  That is just wrong.  When it solely warns about __*_chk builtins known to overflow that is fine, but the other newly added warnings in this area should be only in -Wall or -Wextra or not enabled by default at all.
This is yet another case showing why very late warnings after path isolation etc. are harmful.  Sure, if foo doesn't grow the vector there will be UB in the resize, but there is nothing in the testcase that would show that is expected.

The warning is reproducible with the test case below.  The memset call is inserted by the loop distribution pass.  The pass should avoid inserting memcpy/memmove/memset calls with excessively large arguments (> SIZE_MAX / 2) because they are certainly incorrect and, if executed, will lead to buffer overflow and memory corruption.  I'm testing a patch that inserts a trap instead.

FWIW, the warning is working as I hoped it would: it's helping find bugs.  In this case, the problem is exposed by the loop to memset transformation.  The warning isn't necessarily useful to the user because the code is inserted by GCC itself but it has exposed an opportunity to both make the generated code safer (avoid the memory corruption) and more efficient (it can help GCC generate code based on the assumption that the overflow is unlikely to happen).

$ cat t.ii && gcc -O3 -S -Wall -Wextra -Wpedantic -xc++ -fdump-tree-optimized=/dev/stdout t.ii 
typedef long unsigned int size_t;

inline void
fill (int *p, size_t n, int)
{
  while (n--)
    *p++ = 0;
}

struct B
{
  int* p0, *p1, *p2;

  size_t size () const {
    return size_t (p1 - p0);
  }

  void resize (size_t n) {
    if (n > size())
      append (n - size());
  }

  void append (size_t n)
  {
    if (size_t (p2 - p1) >= n) 	 {
      fill (p1, n, 0);
    }
  }
};

void foo (B &b)
{
  b.resize (b.size () - 1);
}

;; Function void foo(B&) (_Z3fooR1B, funcdef_no=4, decl_uid=2299, cgraph_uid=4, symbol_order=4)

Removing basic block 6
Removing basic block 7
void foo(B&) (struct B & b)
{
  int * _4;
  int * _5;
  int * _6;
  long int _7;
  long int _8;
  long int _13;
  long int _14;

  <bb 2> [4.91%]:
  _4 = MEM[(int * *)b_3(D)];
  _5 = MEM[(int * *)b_3(D) + 8B];
  _13 = (long int) _5;
  _14 = (long int) _4;
  if (_13 == _14)
    goto <bb 3>; [54.00%]
  else
    goto <bb 5>; [46.00%]

  <bb 3> [2.65%]:
  _6 = MEM[(int * *)b_3(D) + 16B];
  _7 = (long int) _6;
  _8 = _7 - _13;
  if (_8 == -4)
    goto <bb 4>; [37.68%]
  else
    goto <bb 5>; [62.32%]

  <bb 4> [1.00%]:
  __builtin_memset (_5, 0, 18446744073709551612); [tail call]

  <bb 5> [4.91%]:
  return;

}


In function ‘void foo(B&)’:
cc1plus: warning: ‘void* __builtin_memset(void*, int, long unsigned int)’: specified size 18446744073709551612 exceeds maximum object size 9223372036854775807 -Wstringop-overflow=]

Patch posted for review:
https://gcc.gnu.org/ml/gcc-patches/2017-01/msg01152.html

So the simplified tests are interesting, but ultimately too simplified.  The VRP prototype which works on the simplified testcases doesn't work on the real testcase for this BZ.

The key sequences look like:

 _13 = (long unsigned int) _12;
  _121 = ADD_OVERFLOW (_13, 18446744073709551615);
  _1 = REALPART_EXPR <_121>;
  _39 = IMAGPART_EXPR <_121>;
  if (_1 > _13)
    goto <bb 4>; [29.56%]
  else
    goto <bb 18>; [70.44%]


Which defeat the simplistic prototype for VRP.  It's essentially still the same construct, but we'd have to extract stuff out of the COMPLEX_EXPR and recognize the ADD_OVERFLOW.

(In reply to Jeffrey A. Law from comment #4)
> So the simplified tests are interesting, but ultimately too simplified.  The
> VRP prototype which works on the simplified testcases doesn't work on the
> real testcase for this BZ.
> 
> The key sequences look like:
> 
>  _13 = (long unsigned int) _12;
>   _121 = ADD_OVERFLOW (_13, 18446744073709551615);
>   _1 = REALPART_EXPR <_121>;
>   _39 = IMAGPART_EXPR <_121>;
>   if (_1 > _13)
>     goto <bb 4>; [29.56%]
>   else
>     goto <bb 18>; [70.44%]
> 
> 
> Which defeat the simplistic prototype for VRP.  It's essentially still the
> same construct, but we'd have to extract stuff out of the COMPLEX_EXPR and
> recognize the ADD_OVERFLOW.

I thought ADD_OVERFLOW would appear in the widening_mul pass, i.e. after all the VRP passes are done? On the other hand, teaching VRP about *_OVERFLOW sounds like a good idea.

(In reply to Marc Glisse from comment #5)
> I thought ADD_OVERFLOW would appear in the widening_mul pass, i.e. after all
> the VRP passes are done? On the other hand, teaching VRP about *_OVERFLOW
> sounds like a good idea.

VRP already handles that, see   /* Handle extraction of the two results (result of arithmetics and
     a flag whether arithmetics overflowed) from {ADD,SUB,MUL}_OVERFLOW
     internal function.  */
  else if (is_gimple_assign (stmt)
           && (gimple_assign_rhs_code (stmt) == REALPART_EXPR
               || gimple_assign_rhs_code (stmt) == IMAGPART_EXPR)
           && INTEGRAL_TYPE_P (type))
guarded block in extract_range_basic and 
  if (is_gimple_call (stmt) && gimple_call_internal_p (stmt))
    switch (gimple_call_internal_fn (stmt))
      {
in vrp_visit_stmt.
IFN_*_OVERFLOW can appear already during gimplification if __builtin_*_overflow{,_p} is used in the source.

(In reply to Jakub Jelinek from comment #6)
> VRP already handles that,

Nice :-) Thanks and sorry for not checking myself.

> IFN_*_OVERFLOW can appear already during gimplification if
> __builtin_*_overflow{,_p} is used in the source.

Yes, I meant that in the original testcase of this PR, widening_mul is the first dump that contains OVERFLOW. Otherwise, indeed the builtins, and match.pd seems to have one transform that can produce IFN_MUL_OVERFLOW.

(In reply to Jeffrey A. Law from comment #4)
> So the simplified tests are interesting, but ultimately too simplified.

You're right, the test case in comment #2 is oversimplified.  The one in the thread on gcc-patches should be closer (it includes the ADD_OVERFLOW):

$ cat t.C && gcc -O2 -S -Wall -fdump-tree-optimized=/dev/stdout t.C
typedef __SIZE_TYPE__ size_t;

struct S {
  int *p0, *p1, *p2;

  size_t size () const { return p1 - p0; }

  void f (size_t n) {
    if (n > size ())       // can't happen because
      foo (n - size ());   //   n is in [1, MIN(size() - 1, 3)]
    else if (n < size ())
      bar (p0 + n);
  }

  void foo (size_t n)
  {
    size_t left = (size_t)(p2 - p1);
    if (left >= n)
      __builtin_memset (p2, 0, n * sizeof *p2);
  }

  void bar (int*);
};

void f (S &s)
{
  size_t n = s.size ();
  if (n > 1 && n < 5)
    s.f (n - 1);
}

;; Function void f(S&) (_Z1fR1S, funcdef_no=3, decl_uid=2296, cgraph_uid=3, symbol_order=3)

Removing basic block 9
Removing basic block 10
Removing basic block 11
void f(S&) (struct S & s)
{
  long unsigned int _1;
  long unsigned int _2;
  int * _7;
  int * _8;
  long int _9;
  long int _10;
  long int _11;
  long int _12;
  long unsigned int _13;
  long unsigned int _17;
  int * _22;
  long int _23;
  long int _24;
  __complex__ long unsigned int _25;
  long unsigned int _28;
  int * _29;

  <bb 2> [100.00%]:
  _7 = MEM[(int * *)s_5(D)];
  _8 = MEM[(int * *)s_5(D) + 8B];
  _9 = (long int) _8;
  _10 = (long int) _7;
  _11 = _9 - _10;
  _12 = _11 /[ex] 4;
  _13 = (long unsigned int) _12;
  _1 = _13 + 18446744073709551614;
  if (_1 <= 2)
    goto <bb 3>; [36.64%]
  else
    goto <bb 8>; [63.36%]

  <bb 3> [36.64%]:
  _25 = ADD_OVERFLOW (_13, 18446744073709551615);
  _2 = REALPART_EXPR <_25>;
  _17 = IMAGPART_EXPR <_25>;
  if (_2 > _13)
    goto <bb 4>; [50.00%]
  else
    goto <bb 6>; [50.00%]

  <bb 4> [18.32%]:
  _22 = MEM[(int * *)s_5(D) + 16B];
  _23 = (long int) _22;
  _24 = _23 - _9;
  if (_24 == -4)
    goto <bb 5>; [33.00%]
  else
    goto <bb 8>; [67.00%]

  <bb 5> [6.05%]:
  __builtin_memset (_22, 0, 18446744073709551612); [tail call]
  goto <bb 8>; [100.00%]

  <bb 6> [18.32%]:
  if (_17 != 0)
    goto <bb 7>; [36.64%]
  else
    goto <bb 8>; [63.36%]

  <bb 7> [6.71%]:
  _28 = _2 * 4;
  _29 = _7 + _28;
  S::bar (s_5(D), _29); [tail call]

  <bb 8> [100.00%]:
  return;

}


In function ‘void S::foo(size_t)’,
    inlined from ‘void S::f(size_t)’ at t.C:10:24,
    inlined from ‘void f(S&)’ at t.C:29:16:
t.C:19:47: warning: ‘void* __builtin_memset(void*, int, long unsigned int)’: specified size 18446744073709551612 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]
       __builtin_memset (p2, 0, n * sizeof *p2);
                                               ^

Sigh.  I was looking at the wrong dump.  We don't need to deal with ADD_OVERFLOW.

Looking at the real test for this BZ, the guarding block has the right form:
<bb 3> [100.00%]:
_7 = MEM[(unsigned int * *)&v];
_8 = MEM[(unsigned int * *)&v + 8B];
_9 = (long int) _8;
_10 = (long int) _7;
_11 = _9 - _10;
_12 = _11 /[ex] 4;
_13 = (long unsigned int) _12;
_1 = _13 + 18446744073709551615;
if (_1 > _13)
  goto <bb 4>; [29.56%]
else
  goto <bb 28>; [70.44%]

We can easily see the condition can be rewritten as (_13 == 0)

The problem is we don't have enough information about _13 to evaluate that at compile time so my VRP hack doesn't optimize it in vrp_evaluate_conditional.

But I would probably claim that simplification from a 2-operand relational to a zero/not-zero test is in and of itself a good thing irrespective of the single_use issues and thus we should go ahead and turn it into an equality test.   If I manually transform that test, 2 of the 3 warnings go away.

While looking through the dumps I saw another conditional that has the form above, *but* it's obfuscated by ASSERT_EXPRs.  I believe if we transform that one as well, there's a good chance the last warning will be eliminated.

So when I look at the whole, I think there's really two changes that need to be made.  First, a special cased match.pd pattern to detect these overflow tests that collapse into a zero/not-zero comparison and apply them regardless of the single_use property.  That should detect both cases in the original testcase and hopefully remove all 3 of the warnings.

Second, vrp_evaluate_conditional ought detect when there's a more general simplified form (which wasn't simplified due to the single_use property) and use that simplified form when trying to determine if a conditional is compile-time determinable.  I've got a prototype for this.  THe question is whether or not it applies often enough to matter in practice.  I'll have to do more instrumentation.

So with a prototype patch to simplify the overflow checks to zero/not zero, we still get the one warning.   The more I look at the code, the more I think the warning is justified. 

Ponder the case where v.size () returns 0 in the test.  That'll result in calling v.resize (-1).  

In the resize code we have:


     resize(size_type __new_size)
      {
        if (__new_size > size())
          _M_default_append(__new_size - size());
        else if (__new_size < size())
          _M_erase_at_end(this->_M_impl._M_start + __new_size);
      }
Remembering that we're working with size_t (ie, unsigned) objects, we'll go into the TRUE clause, calling:

  std::vector<unsigned int>::_M_default_append (&v, 18446744073709551615);

At which point I think we've lost....  And ISTM that GCC gave a valid warning for the given inputs.


Thoughts Martin?

I agree that calling v.resize(v.size() - 0) when v.size() is zero is a bug in the program and diagnosing it should be a good thing (I think that corresponds to the test case I pasted in comment #2 and what I based my patch with the trap on.)

If your patch avoids the warning when the if guard in the test case in comment #0 is uncommented that would seem like a fix.

As a separate change (in GCC 8) I do think it would be worth considering the idea of inserting the trap, and also having tree-loop-distribution.c issue a better warning when it detects an out of bounds loop like this one.

With the v.size() > 0 guard enabled and my VRP tweaks I get no warnings.  Plan is to either polish up the VRP tweaks or turn them into a match.pd pattern.  Not sure which is going to be better yet.

Author: law
Date: Tue Feb 14 15:54:09 2017
New Revision: 245434

URL: https://gcc.gnu.org/viewcvs?rev=245434&root=gcc&view=rev
Log:
	PR tree-optimization/79095
	* tree-vrp.c (extract_range_from_binary_expr_1): For EXACT_DIV_EXPR,
	if the numerator has the range ~[0,0] make the resultant range ~[0,0].
	(extract_range_from_binary_expr): For MINUS_EXPR with no derived range,
	if the operands are known to be not equal, then the resulting range
	is ~[0,0].
	(intersect_ranges): If the new range is ~[0,0] and the old range is
	wide, then prefer ~[0,0].
	* tree-vrp.c (overflow_comparison_p_1): New function.
	(overflow_comparison_p): New function.
	* tree-vrp.c (register_edge_assert_for_2): Register additional asserts
	if NAME is used in an overflow test.
	(vrp_evaluate_conditional_warnv_with_ops): If the ops represent an
	overflow check that can be expressed as an equality test, then adjust
	ops to be that equality test.

	PR tree-optimization/79095
	* g++.dg/pr79095-1.C: New test
	* g++.dg/pr79095-2.C: New test
	* g++.dg/pr79095-3.C: New test
	* g++.dg/pr79095-4.C: New test
	* g++.dg/pr79095-5.C: New test
	* gcc.c-torture/execute/arith-1.c: Update with more cases.
	* gcc.dg/tree-ssa/pr79095-1.c: New test.

Added:
    trunk/gcc/testsuite/g++.dg/pr79095-1.C
    trunk/gcc/testsuite/g++.dg/pr79095-2.C
    trunk/gcc/testsuite/g++.dg/pr79095-3.C
    trunk/gcc/testsuite/g++.dg/pr79095-4.C
    trunk/gcc/testsuite/g++.dg/pr79095-5.C
    trunk/gcc/testsuite/gcc.dg/tree-ssa/pr79095.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.c-torture/execute/arith-1.c
    trunk/gcc/tree-vrp.c

Bogus warnings fixed on the trunk.