Bug 59892 - out of bounds array access is misoptimized
Summary: out of bounds array access is misoptimized
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: unknown
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-20 18:35 UTC by Alexei Starovoitov
Modified: 2014-01-21 17:35 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
test case (226 bytes, text/plain)
2014-01-20 18:35 UTC, Alexei Starovoitov
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexei Starovoitov 2014-01-20 18:35:17 UTC
Created attachment 31901 [details]
test case

attached test case was narrowed down from linux kernel
file drivers/scsi/isci/host.h

#define for_each_isci_host(id, ihost, pdev) \
        for (id = 0, ihost = to_pci_info(pdev)->hosts[id]; \
             id < ARRAY_SIZE(to_pci_info(pdev)->hosts) && ihost; \
             ihost = to_pci_info(pdev)->hosts[++id])

'hosts' array has fixed size of 2:
struct isci_pci_info {
        struct msix_entry msix_entries[SCI_MAX_MSIX_INT];
        struct isci_host *hosts[SCI_MAX_CONTROLLERS];
        struct isci_orom *orom;
};

and loop is accessing 3rd element of the array.
Behavior is undefined, but gcc 4.7 and older were ok,
whereas GCC 4.8 and the latest 4.9 are misoptimizing the code
by dropping 'id < 2' loop condition.

$ gcc -O0 array_out_of_bounds.c
$ ./a.out
(0 < 2) == 1
(1 < 2) == 1
$ gcc -O2 array_out_of_bounds.c
$ ./a.out
(0 < 2) == 1
(1 < 2) == 1
Segmentation fault (core dumped)

'cunrolli' pass is confused with such loop condition and produces wrong tree:
  <bb 3>:
  _19 = (int) _16;
  printf ("(%d < %d) == %d\n", 0, 2, _19);
  i_21 = 1;
  isci_host_22 = v.hosts[i_21];
  _6 = i_21 <= 1;
  _7 = isci_host_22 != 0B;
  _8 = _6 & _7;
  if (_8 != 0)
    goto <bb 4>;
  else
    goto <bb 5>;

  <bb 4>:
  _9 = (int) _6;
  printf ("(%d < %d) == %d\n", i_21, 2, _9);
  i_11 = i_21 + 1;
  __builtin_unreachable ();

if 'cunrolli' is disabled, the VRP pass is equally confused:
  <bb 3>:
  _9 = 1;
  printf ("(%d < %d) == %d\n", i_1, 2, 1);
  i_11 = i_1 + 1;
  isci_host_12 = v.hosts[i_11];

  <bb 4>:
  # i_1 = PHI <0(2), i_11(3)>
  # isci_host_2 = PHI <isci_host_5(2), isci_host_12(3)>
  _6 = 1;
  _7 = isci_host_2 != 0B;
  _8 = _7;
  if (_8 != 0)
    goto <bb 3>;
  else
    goto <bb 5>;

  <bb 5>:
  return 0;

and optimizes i<=1 condition into _6 = 1 above, whereas bb4 can
be executed for i=2

worst is that compiler doesn't warn on the problem.
-Wall and -Warray-bounds don't see it.

Though the code is erroneous by C standard definition, GCC should be smarter
and prevent misoptimization where it can. As a minimum it should warn about such cases.
Comment 1 Andrew Pinski 2014-01-20 18:43:39 UTC
The warning should be there already.
Comment 2 Markus Trippelsdorf 2014-01-20 18:47:05 UTC
struct isci_host;
struct isci_orom;

struct isci_pci_info {
  struct isci_host *hosts[2];
  struct isci_orom *orom;
} v = {{(struct isci_host *)1,(struct isci_host *)1}, 0};

int printf(const char *fmt, ...);

int isci_pci_probe()
{
  int i;
  struct isci_host *isci_host;

  for (i = 0, isci_host = v.hosts[i];
       i < 2 && isci_host;
       isci_host = v.hosts[++i]) {
    printf("(%d < %d) == %d\n", i, 2, (i < 2));
  }

  return 0;
}

int main()
{
  isci_pci_probe();
}

When v.hosts[0] or v.hosts[1] is NULL the loop is fine, so there is
no reason for a warning.
The testcase is obviously invalid.
Comment 3 Lukasz Dorau 2014-01-21 17:24:50 UTC
(In reply to Markus Trippelsdorf from comment #2)
> When v.hosts[0] or v.hosts[1] is NULL the loop is fine, so there is
> no reason for a warning.
> The testcase is obviously invalid.
>
But v.hosts[0] and v.hosts[1] do not have to be NULL. What's more, they are supposed not to be NULL. So why is it invalid?
I understand that the loop is erroneous, but I do not understand why misoptimizing the code by dropping 'id < 2' loop condition is a right thing to do? And even without any warning?
Comment 4 Markus Trippelsdorf 2014-01-21 17:31:56 UTC
(In reply to Lukasz Dorau from comment #3)
> (In reply to Markus Trippelsdorf from comment #2)
> > When v.hosts[0] or v.hosts[1] is NULL the loop is fine, so there is
> > no reason for a warning.
> > The testcase is obviously invalid.
> >
> But v.hosts[0] and v.hosts[1] do not have to be NULL. What's more, they are
> supposed not to be NULL. So why is it invalid?
> I understand that the loop is erroneous, but I do not understand why
> misoptimizing the code by dropping 'id < 2' loop condition is a right thing
> to do? And even without any warning?

Please note that you can use -fno-aggressive-loop-optimization if you 
dislike this behavior.

From http://gcc.gnu.org/bugs/:
 »if compiling with -fno-strict-aliasing -fwrapv -fno-aggressive-loop-optimizations makes a difference, your code probably is not correct«

And -Wno-aggressive-loop-optimizations doesn't handle early loop exits.
Comment 5 Lukasz Dorau 2014-01-21 17:35:55 UTC
I see. Now I understand. Thanks!