[Bug tree-optimization/113630] New: -fno-strict-aliasing introduces out-of-bounds memory access

[Bug tree-optimization/113630] New: -fno-strict-aliasing introduces out-of-bounds memory access

[kristerw at gcc dot gnu.org]

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

            Bug ID: 113630
           Summary: -fno-strict-aliasing introduces out-of-bounds memory
                    access
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: kristerw at gcc dot gnu.org
  Target Milestone: ---

The test gcc.dg/torture/pr110799.c crashes because of an out of bounds memory
access when compiled with "-O2 -fno-strict-aliasing".

What is happening is that the pre pass has changed

struct S {
    int a;
};
struct M {
    int a, b;
};

__attribute__((noipa, noinline, noclone, no_icf))
int f (struct S * p, int c, int d)
{
  int r;

  <bb 2>:
  if (c_2(D) != 0)
    goto <bb 3>;
  else
    goto <bb 6>;

  <bb 3>:
  if (d_6(D) != 0)
    goto <bb 4>;
  else
    goto <bb 5>;

  <bb 4>
  r_8 = p_4(D)->a;
  goto <bb 7>;

  <bb 5>
  r_7 = MEM[(struct M *)p_4(D)].a;
  goto <bb 7>;

  <bb 6>
  r_5 = MEM[(struct M *)p_4(D)].b;

  <bb 7>
  # r_1 = PHI <r_7(5), r_5(6), r_8(4)>
  return r_1;
}


by combining  bb 4 and bb 5 and doing all accesses as struct M:


__attribute__((noipa, noinline, noclone, no_icf))
int f (struct S * p, int c, int d)
{
  int r;
  int pretmp_9;

  <bb 2>:
  if (c_2(D) != 0)
    goto <bb 3>; [50.00%]
  else
    goto <bb 4>; [50.00%]

  <bb 3>:
  pretmp_9 = MEM[(struct M *)p_4(D)].a;
  goto <bb 5>;

  <bb 4>:
  r_5 = MEM[(struct M *)p_4(D)].b;

  <bb 5>:
  # r_1 = PHI <pretmp_9(3), r_5(4)>
  return r_1;
}


This in turn allows later passes to hoist the two loads


__attribute__((noipa, noinline, noclone, no_icf))
int f (struct S * p, int c, int d)
{
  int r;
  int pretmp_9;

  <bb 2>:
  pretmp_9 = MEM[(struct M *)p_4(D)].a;
  r_5 = MEM[(struct M *)p_4(D)].b;
  if (c_2(D) != 0)
    goto <bb 3>;
  else
    goto <bb 4>;

  <bb 3>:

  <bb 4>:
  # r_1 = PHI <pretmp_9(3), r_5(2)>
  return r_1;
}


which now reads out of bounds when we pass a struct S as f(&s, 1, 1).

[Bug tree-optimization/113630] -fno-strict-aliasing introduces out-of-bounds memory access

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I suspect pre us doing the right thing. It is phi-opt code that hoists is doing
the wrong thing for non strict aliasing.

[Bug tree-optimization/113630] [11/12/13/14 Regression] -fno-strict-aliasing introduces out-of-bounds memory access

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.5
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=110799
      Known to fail|                            |10.1.0, 14.0, 7.1.0
      Known to work|                            |5.1.0, 6.1.0, 6.4.0
           Keywords|                            |needs-bisection, wrong-code
            Summary|-fno-strict-aliasing        |[11/12/13/14 Regression]
                   |introduces out-of-bounds    |-fno-strict-aliasing
                   |memory access               |introduces out-of-bounds
                   |                            |memory access

[Bug tree-optimization/113630] [11/12/13/14 Regression] -fno-strict-aliasing introduces out-of-bounds memory access

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2024-01-27
     Ever confirmed|0                           |1
      Known to work|5.1.0, 6.1.0, 6.4.0         |

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Confirmed.

I really think what PRE does is correct here since we have an aliasing set of 0
for both. Now what is incorrect is hoist_adjacent_loads which cannot do either
of any of the aliasing sets are 0 ...



I think even the function below is valid for non-strict aliasing:
```
int __attribute__((noipa,noinline))
f(struct S *p, int c, int d)
{
  int r;
  if (c)
    {
        r = ((struct M*)p)->a;
    }
  else
    r = ((struct M*)p)->b;
  return r;
}
```

That is hoist_adjacent_loads is broken for non-strict-aliasing in general and
has been since 4.8.0 when it was added (r0-117275-g372a6eb8d991eb).

[Bug tree-optimization/113630] [11/12/13/14 Regression] -fno-strict-aliasing introduces out-of-bounds memory access

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Note LLVM produces decent code here by only using one load:
```
        xor     eax, eax
        test    esi, esi
        sete    al
        mov     eax, dword ptr [rdi + 4*rax]
```

Maybe GCC could do the same ...

[Bug tree-optimization/113630] [11/12/13/14 Regression] -fno-strict-aliasing introduces out-of-bounds memory access

[rguenth at gcc dot gnu.org]

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #3)
> Note LLVM produces decent code here by only using one load:
> ```
>         xor     eax, eax
>         test    esi, esi
>         sete    al
>         mov     eax, dword ptr [rdi + 4*rax]
> ```
> 
> Maybe GCC could do the same ...

IIRC there's duplicate bugs about this - phiprop does kind-of the reverse.
The sink pass can now sink two exactly same stores but doesn't try sinking
a "compatible" store by introducing a PHI for the address.

              /* ??? We could handle differing SSA uses in the LHS by inserting
                 PHIs for them.  */
              else if (! operand_equal_p (gimple_assign_lhs (first_store),
                                          gimple_assign_lhs (def), 0)
                       || (gimple_clobber_p (first_store)
                           != gimple_clobber_p (def)))

[Bug tree-optimization/113630] [11/12/13/14 Regression] -fno-strict-aliasing introduces out-of-bounds memory access

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #2)
> Confirmed.
> 
> I really think what PRE does is correct here since we have an aliasing set
> of 0 for both. Now what is incorrect is hoist_adjacent_loads which cannot do
> either of any of the aliasing sets are 0 ...
> 
> 
> 
> I think even the function below is valid for non-strict aliasing:
> ```
> int __attribute__((noipa,noinline))
> f(struct S *p, int c, int d)
> {
>   int r;
>   if (c)
>     {
>         r = ((struct M*)p)->a;
>     }
>   else
>     r = ((struct M*)p)->b;
>   return r;
> }
> ```
> 
> That is hoist_adjacent_loads is broken for non-strict-aliasing in general
> and has been since 4.8.0 when it was added (r0-117275-g372a6eb8d991eb).

It looks it relies on

      /* The zeroth operand of the two component references must be
         identical.  It is not sufficient to compare get_base_address of
         the two references, because this could allow for different
         elements of the same array in the two trees.  It is not safe to
         assume that the existence of one array element implies the
         existence of a different one.  */
      if (!operand_equal_p (TREE_OPERAND (ref1, 0), TREE_OPERAND (ref2, 0), 0))
        continue;

for the correctness test.  Note the MEM accesses are of size sizeof (struct M).

With -fno-strict-aliasing we're not wiping that detail so I think it _is_
a bug in PRE that it merges the two accesses.

I'll have a more detailed look.

[Bug tree-optimization/113630] [11/12/13/14 Regression] -fno-strict-aliasing introduces out-of-bounds memory access

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:724b64304ff5c8ac08a913509afd6fde38d7b767

commit r14-8653-g724b64304ff5c8ac08a913509afd6fde38d7b767
Author: Richard Biener <rguenther@suse.de>
Date:   Wed Jan 31 11:28:50 2024 +0100

    tree-optimization/113630 - invalid code hoisting

    The following avoids code hoisting (but also PRE insertion) of
    expressions that got value-numbered to another one that are not
    a valid replacement (but still compute the same value).  This time
    because the access path ends in a structure with different size,
    meaning we consider a related access as not trapping because of the
    size of the base of the access.

            PR tree-optimization/113630
            * tree-ssa-pre.cc (compute_avail): Avoid registering a
            reference with a representation with not matching base
            access size.

            * gcc.dg/torture/pr113630.c: New testcase.

[Bug tree-optimization/113630] [11/12/13 Regression] -fno-strict-aliasing introduces out-of-bounds memory access

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
            Summary|[11/12/13/14 Regression]    |[11/12/13 Regression]
                   |-fno-strict-aliasing        |-fno-strict-aliasing
                   |introduces out-of-bounds    |introduces out-of-bounds
                   |memory access               |memory access
      Known to work|                            |14.0

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed on trunk sofar.

[Bug tree-optimization/113630] [11/12/13 Regression] -fno-strict-aliasing introduces out-of-bounds memory access

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:47cd06042237bf2d4f05b8355362bc038f6fa445

commit r13-8693-g47cd06042237bf2d4f05b8355362bc038f6fa445
Author: Richard Biener <rguenther@suse.de>
Date:   Wed Jan 31 11:28:50 2024 +0100

    tree-optimization/113630 - invalid code hoisting

    The following avoids code hoisting (but also PRE insertion) of
    expressions that got value-numbered to another one that are not
    a valid replacement (but still compute the same value).  This time
    because the access path ends in a structure with different size,
    meaning we consider a related access as not trapping because of the
    size of the base of the access.

            PR tree-optimization/113630
            * tree-ssa-pre.cc (compute_avail): Avoid registering a
            reference with a representation with not matching base
            access size.

            * gcc.dg/torture/pr113630.c: New testcase.

    (cherry picked from commit 724b64304ff5c8ac08a913509afd6fde38d7b767)

[Bug tree-optimization/113630] [11/12 Regression] -fno-strict-aliasing introduces out-of-bounds memory access

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org
      Known to fail|                            |13.2.0
             Status|ASSIGNED                    |NEW
      Known to work|                            |13.2.1
            Summary|[11/12/13 Regression]       |[11/12 Regression]
                   |-fno-strict-aliasing        |-fno-strict-aliasing
                   |introduces out-of-bounds    |introduces out-of-bounds
                   |memory access               |memory access
           Assignee|rguenth at gcc dot gnu.org         |unassigned at gcc dot gnu.org

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
Backported as far as I want.