Bug 57199 - [4.8/4.9 Regression] Bogus warning: iteration NNNN invokes undefined behavior -Waggressive-loop-optimizations
Summary: [4.8/4.9 Regression] Bogus warning: iteration NNNN invokes undefined behavior...
Status: RESOLVED WONTFIX
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.9.0
: P3 normal
Target Milestone: 4.8.1
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-07 22:09 UTC by Paul Pluzhnikov
Modified: 2014-03-26 20:56 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2013-05-07 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Pluzhnikov 2013-05-07 22:09:29 UTC
Google ref: b/8856275

Test case:



typedef decltype(sizeof(0)) size_t;

struct Foo {
  ~Foo();     // comment out -> problem disappears
  int x[20];
};

template <typename T, int N>
struct InlinedVector {
  inline InlinedVector() : size_(0) { }
  inline size_t size() const {
    return size_;
  }
  T* mutable_array();
  void resize(size_t n) {
    size_t s = size();
    DestroyElements(mutable_array() + n, s - n);
  }
  static void DestroyElements(T* ptr, size_t num) {
    for (size_t i = 0; i < num; i++) {
      ptr[i].~T();
    }
  }

  size_t size_;
};

struct CP {
  void Add();
  InlinedVector<Foo,1> foo_;
};

void CP::Add() {
  foo_.resize(foo_.size() + 1);
}



Using trunk r198689:

g++ -std=c++11 -O2 -c tt.cc
tt.cc: In member function ‘void CP::Add()’:
tt.cc:21:7: warning: iteration 230584300921369396ul invokes undefined behavior [-Waggressive-loop-optimizations]
       ptr[i].~T();
       ^
tt.cc:20:5: note: containing loop
     for (size_t i = 0; i < num; i++) {
     ^
Comment 1 Richard Biener 2013-05-08 08:57:45 UTC
Note that there seems to be an error in the source:

    void resize(size_t n) {
        size_t s = size();
        DestroyElements(mutable_array() + n, s - n);

for the call with n == size () + 1 the function calls DestroyElements
with -1U elements which we figure out and optimize the caller to

  <bb 3>:
  _13 = i_12 * 80;
  _14 = _11 + _13;
  Foo::~Foo (_14);
  i_15 = i_12 + 1;

  <bb 4>:
  # i_12 = PHI <0(2), i_15(3)>
  if (i_12 != 18446744073709551615)
    goto <bb 3>;
  else
    goto <bb 5>;

which then causes the addition to overflow and invoke undefined behavior
and we warn.

Thus, the warning, while not 100% helpful, points at a real problem.
Comment 2 Paul Pluzhnikov 2013-05-08 12:55:31 UTC
> Thus, the warning, while not 100% helpful, points at a real problem.

There is no real problem in the original source, which reads:

  size_t s = size();
  if (n < s)
    DestroyElements(mutable_array() + n, s - n);
  else ...

and produces the same warning.

Here is updated test with above condition; confirmed to still show the warning with trunk @r198709.



typedef decltype(sizeof(0)) size_t;

struct Foo {
  ~Foo();      // comment out -> problem disappears
  int x[20];
};

template <typename T, int N>
struct InlinedVector {
  inline InlinedVector() : size_(0) { }
  inline size_t size() const {
    return size_;
  }
  T* mutable_array();
  void resize(size_t n) {
    size_t s = size();
    if (n < s)
      DestroyElements(mutable_array() + n, s - n);
  }
  static void DestroyElements(T* ptr, size_t num) {
    for (size_t i = 0; i < num; i++) {
      ptr[i].~T();
    }
  }

  size_t size_;
};

struct CP {
  void Add();
  InlinedVector<Foo,1> foo_;
};

void CP::Add() {
  foo_.resize(foo_.size() + 1);
}
Comment 3 Jakub Jelinek 2013-05-20 08:12:49 UTC
I don't think the warning is bogus.
At phiprop we have:
  long unsigned int _3;
  size_t _6;
  size_t _7;
  long unsigned int _8;

  <bb 2>:
  _6 = MEM[(const struct InlinedVector *)this_1(D)].size_;
  _3 = _6 + 1;
...
  _7 = MEM[(const struct InlinedVector *)this_1(D)].size_;
  if (_3 < _7)
    goto <bb 3>;
  else
    goto <bb 6>;

  <bb 3>:
  _8 = _7 - _3;

and fre2 turns this into:

  _6 = MEM[(const struct InlinedVector *)this_1(D)].size_;
  _3 = _6 + 1;
...
  _7 = _6;
  if (_3 < _7)
    goto <bb 3>;
  else
    goto <bb 6>;

  <bb 3>:
  _8 = 18446744073709551615;

i.e. it can't prove the following loop that uses _8 as upper bound is dead, but the loop has at that point constant bounds and triggers undefined behavior in it if executed.  The loop can be only executed if foo_.size() is (size_t) -1, but if you call it with this value, you'll surely hit the undefined behavior the warning is complaining about.
Comment 4 Paul Pluzhnikov 2013-05-20 14:10:28 UTC
(In reply to Jakub Jelinek from comment #3)
> it can't prove the following loop that uses _8 as upper bound is dead, ...

Do we need a separate "may invoke undefined behavior" warning?

In our codebase of 100+ million lines of C++, we've found ~20 instances of real "invokes undefined behavior" (so we don't want to turn the -Waggressive-loop-optimizations off).

But we also have ~5 instances where the warning is bogus, and there is no easy way to turn them off, short of sprinkling -Wno-aggresive-loop-optimizations over our BUILD files.
Comment 5 Jakub Jelinek 2013-05-20 14:26:22 UTC
But this isn't any form of the may invoke, the loop certainly unconditionally invokes undefined behavior, just the whole loop is very unlikely to be ever executed (in this case if size is supposed to represent the length of an array with elements bigger than 1, then already the size would need to be invalid, but that is something the compiler can't understand, for it the size_t field is likely any other field, and there is no guarantee it won't be -1).

It is in principle no different from say:

void
foo (size_t x)
{
  if (x == (size_t) -1)
    {
      unsigned int a[128];
      int i;

      for (i = 0; i < 128; ++i)     /* { dg-message "note: containing loop" } */
        a[i] = i * 0x02000001;      /* { dg-warning "invokes undefined behavior" } */
      bar (a);
    }
}

where you know you are never going to call foo with (size_t) -1, but the compiler doesn't know.  How is the above different from say:
void
bar (void)
{
  unsigned int a[128];
  int i;

  for (i = 0; i < 128; ++i)     /* { dg-message "note: containing loop" } */
    a[i] = i * 0x02000001;      /* { dg-warning "invokes undefined behavior" } */
  bar (a);
}
...
/* in another CU */
void
baz (size_t x)
{
  if (x == (size_t) -1)
    bar ();
}

In your original testcase, you wouldn't get the warning if size was a signed integer instead of unsigned one, then the compiler would know it is undefined behavior if the size wraps and would just optimize the loop away altogether.  Or perhaps some __builtin_unreachable assert that size isn't (size_t) -1?
Comment 6 Paul Pluzhnikov 2013-05-20 15:19:39 UTC
> perhaps some __builtin_unreachable assert that size isn't (size_t) -1?

That works. Thanks for the suggestion.
Comment 7 Mikhail Veltishchev 2014-03-26 20:39:46 UTC
(In reply to Paul Pluzhnikov from comment #6)
> > perhaps some __builtin_unreachable assert that size isn't (size_t) -1?
> 
> That works. Thanks for the suggestion.
Please, can you explain how you fixed this? We have almost the same problem.
Comment 8 Paul Pluzhnikov 2014-03-26 20:56:16 UTC
(In reply to Mikhail Veltishchev from comment #7)

> Please, can you explain how you fixed this? We have almost the same problem.

Here is the fix we deployed (test case from comment#2):

$ diff -u pr57199.cc pr57199a.cc
--- pr57199.cc  2014-03-26 13:50:25.682351842 -0700
+++ pr57199a.cc 2014-03-26 13:54:12.880279926 -0700
@@ -18,6 +18,9 @@
       DestroyElements(mutable_array() + n, s - n);
   }
   static void DestroyElements(T* ptr, size_t num) {
+    if (__builtin_expect (num == (size_t) -1, 0)) {
+      __builtin_unreachable();
+    }
     for (size_t i = 0; i < num; i++) {
       ptr[i].~T();
     }