[Bug c++/97474] New: Regression: optimization produces wrong code

[Bug c++/97474] New: Regression: optimization produces wrong code

[sfranzen85 at hotmail dot com]

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

            Bug ID: 97474
           Summary: Regression: optimization produces wrong code
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: sfranzen85 at hotmail dot com
  Target Milestone: ---

While searching for something different I found the following example code
demonstrating a possible gcc bug:
---
#include <iostream>
using std::cout;

struct A {
    int a;
    int& b;

    A(int x) : a(x), b(a) {}
    A(const A& other) : a(other.a), b(a) {}
    A() : a(0), b(a) {}
};

int foo(A a) {
    a.a *= a.b;
    return a.b;
}


int main() {
    A a(3);

    cout << foo(a) << '\n';
    return 0;
}
---
(Source:
https://stackoverflow.com/questions/62853805/why-does-modifying-a-field-that-is-referenced-by-another-variable-lead-to-unexpe)

I was unable to find a related bug report, hence this one. Wrong output (3
instead of 9) is produced with -O1 or higher, since gcc version 6.4, as
mentioned in a comment. Indeed gcc trunk on godbolt still generates faulty
code: https://godbolt.org/z/TcedxE.

[Bug c++/97474] Regression: optimization produces wrong code

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This feels like an use after something goes out of scope.

[Bug tree-optimization/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-10-17
     Ever confirmed|0                           |1
            Summary|Regression: optimization    |[8/9/10/11 Regression]
                   |produces wrong code         |produces wrong code with
                   |                            |references to another field
          Component|c++                         |tree-optimization
           Keywords|                            |alias
   Target Milestone|---                         |8.5
             Status|UNCONFIRMED                 |NEW

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #1)
> This feels like an use after something goes out of scope.

It is not, there is some weird aliasing issue.

The IR looks like:
int foo(A) (struct A & restrict a)
{
  int _1;
  int & _2;
  int _3;
  int _4;
  int & _5;
  int _9;

  <bb 2> [0.00%]:
  _1 = *a_7(D).a;
  _2 = *a_7(D).b;
  _3 = *_2;
  _4 = _1 * _3;
  *a_7(D).a = _4;
  _5 = *a_7(D).b;
  _9 = *_5;
  return _9;

}

For some reason we think _5 cannot alias (*a_7(D)).a.  Most likely due to the
restict in the IR.  The restrict is there because a was passed by value rather
than reference but the way the ABI works, it is passed by a temp struct and we
expose this in the IR.  All things go down hill from there.

[Bug tree-optimization/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with r7-70-g8270b82dd6523937110a2488194d8692a09299f5

[Bug tree-optimization/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[rguenth at gcc dot gnu.org]

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

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
           Priority|P3                          |P2

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
I will have a look.

[Bug tree-optimization/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[rguenth at gcc dot gnu.org]

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

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
OK, so the issue is that the C++ frontend marks 'this' as restrict which in
turn means that no other pointer can alias it.  For foo() GCC computes

  <bb 2> :
  _1 = MEM[(struct A &)a_7(D) clique 1 base 1].a;
  # PT = nonlocal escaped null 
  _2 = MEM[(struct A &)a_7(D) clique 1 base 1].b;
  _3 = MEM[(int &)_2 clique 1 base 0]; 
  _4 = _1 * _3; 
  MEM[(struct A &)a_7(D) clique 1 base 1].a = _4;
  # PT = nonlocal escaped null  
  _5 = MEM[(struct A &)a_7(D) clique 1 base 1].b;
  _9 = MEM[(int &)_5 clique 1 base 0]; 
  return _9;

in particular

a_7(D), points-to NULL, points-to vars: { D.2410 } (nonlocal, restrict)
_5, points-to non-local, points-to escaped, points-to NULL, points-to vars: { }

and thus the load of _9 via _5 does not alias the store via a_7.  Basically,
GCC does not consider storage pointed to by a restrict pointer to point to
itself (what the CTOR does - make b refer to its own a).

It's not difficult to fix this - simply amend the constraints:

a = &PARM_NOALIAS(9)
PARM_NOALIAS(9) = &NONLOCAL

also have

PARM_NOALIAS(9) = &PARM_NOALIAS(9)

the question is whether this generally applies to restrict qualified pointers.
In some sense for

 A * restrict p;
 int *q = p->b;

q is derived from p.

[Bug tree-optimization/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jsm28 at gcc dot gnu.org

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
So the C standard isn't explicit here but if I read "based on" as "a pointer
q is based on p if adjusting p changes q" then it's based on.  If the
change to q is supposed to be the same as the change to p then this case
wouldn't be covered.  Joseph?  C testcase:

struct S { int a; int *p; };
int foo (struct S * restrict p)
{
  p->a = 1;
  p->p->b = 2;
  return p->a;
}

int main()
{
  struct S s;
  s.p = &s.a;
  if (foo (&s) != 2)
    abort ();
}

so can S->p point to S->a in foo()?

[Bug tree-optimization/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Created attachment 49397
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49397&action=edit
fix in points-to analysis

So this fix would allow this case (and fix the C++ FEs using of restrict)

[Bug tree-optimization/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[joseph at codesourcery dot com]

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

--- Comment #8 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
p and p->p are two different pointer objects, the first restricted, so 
it's not valid to use p->p to access an object that's also accessed via p 
(and modified).

This is different from the case where there is one pointer object but 
multiple expressions, indirecting through multiple other pointer objects, 
that refer to it.  For that case, of multiple expressions that all end up 
referring to the same pointer object, see bug 14192 comment 8 where I 
discuss the rule about "restrict" when pointers to pointers are involved.

[Bug c++/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org
             Status|ASSIGNED                    |NEW
          Component|tree-optimization           |c++
           Assignee|rguenth at gcc dot gnu.org         |unassigned at gcc dot gnu.org

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
Thanks Joseph.  This means that while the proposed patch is conservative
and fixes the issue at hand it's really the C++ frontend that is wrong
in marking the parameter restrict.

Now, it might be useful to introduce a less strict definition of restrict
for this particular usecase in the frontend.  PTA for example implicitely
adds 'restrict' to the 'this' parameter of C++ CTORs (DECL_CXX_CONSTRUCTOR_P).

Leaving to C++ folks to decide.

Note that I'm not sure keying the C++ restrict adding on pointer fields in
the type works since PTA also tracks pointers through integers and thus the
following is miscompiled as well:

struct A {
    int a;
    unsigned long b;
    A(int x) : a(x), b((unsigned long)&a) {}
    A(const A& other) : a(other.a), b((unsigned long)&a) {}
    A() : a(0), b((unsigned long)&a) {}
};

int foo(A a) 
{   
    a.a *= *(int *)a.b;
    return *(int *)a.b;
}

int main() 
{   
    A a(3);
    if (foo(a) != 9)
      __builtin_abort ();
    return 0;
}

IMHO this tracking of pointers through integers is a valid restrict
optimization since extracting the "other" pointer from an integer
isn't any better to make it valid.

So if we want an "alternate restrict mode" then we have to carve another
bit out of tree_type_common (there are quite a few spare bits left).

Thoughts?

[Bug c++/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu.org,
                   |                            |mpolacek at gcc dot gnu.org,
                   |                            |ppalka at gcc dot gnu.org

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think the place in the C++ FE that adds restrict here is:
tree
type_passed_as (tree type)
{
  /* Pass classes with copy ctors by invisible reference.  */
  if (TREE_ADDRESSABLE (type))
    {
      type = build_reference_type (type);
      /* There are no other pointers to this temporary.  */
      type = cp_build_qualified_type (type, TYPE_QUAL_RESTRICT);
    }

So, the options are either not to do it for types that contain any
pointers/references among its members, or do some extra analysis as to whether
even if it contains some pointers/references if those are guaranteed not to
point to anything within the structure (e.g. because of TBAA rules), or using
that analysis decide if it can use normal restrict or needs to use a weaker
restrict, etc.

[Bug c++/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[jakub at gcc dot gnu.org]

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #10)
> I think the place in the C++ FE that adds restrict here is:
> tree
> type_passed_as (tree type)
> {
>   /* Pass classes with copy ctors by invisible reference.  */
>   if (TREE_ADDRESSABLE (type))
>     {
>       type = build_reference_type (type);
>       /* There are no other pointers to this temporary.  */
>       type = cp_build_qualified_type (type, TYPE_QUAL_RESTRICT);
>     }
> 
> So, the options are either not to do it for types that contain any
> pointers/references among its members, or do some extra analysis as to
> whether even if it contains some pointers/references if those are guaranteed
> not to point to anything within the structure (e.g. because of TBAA rules),
> or using that analysis decide if it can use normal restrict or needs to use
> a weaker restrict, etc.

E.g. the vtable pointer is guaranteed not to point into the structure, so that
one can be special cased in the checking.

[Bug c++/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[jason at gcc dot gnu.org]

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

--- Comment #12 from Jason Merrill <jason at gcc dot gnu.org> ---
Yeah, adding restrict there is just wrong; the constructor is called outside
the function, and could e.g. stash a pointer to the object in a global
variable.  What we actually want is to treat this reference parameter like a
value parameter.  Is that information actually useful to the optimizers?

[Bug c++/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[rguenther at suse dot de]

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

--- Comment #13 from rguenther at suse dot de <rguenther at suse dot de> ---
On Mon, 25 Jan 2021, jason at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474
> 
> --- Comment #12 from Jason Merrill <jason at gcc dot gnu.org> ---
> Yeah, adding restrict there is just wrong; the constructor is called outside
> the function, and could e.g. stash a pointer to the object in a global
> variable.  What we actually want is to treat this reference parameter like a
> value parameter.  Is that information actually useful to the optimizers?

It's a layout hint PTA can use but without restricting who can point
to said object it's not going to affect precision much (in fact I'd
have to double check that giving hinting a wrong object size by
passing the base "value" of a derived class object won't cause issues).

ISTR we've explored keying on REFERENCE_TYPE only (for said layout hint)
in the past.

[Bug c++/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

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

--- Comment #14 from Jason Merrill <jason at gcc dot gnu.org> ---
(In reply to rguenther@suse.de from comment #13)

Indeed, some useful information is that the parameter is a reference to the
most-derived object, not a base class; that should be useful to
devirtualization, particularly.

Looks like we already have DECL_BY_REFERENCE to indicate this situation, and
ipa-polymorphic-call already looks at it, so I'm going to just remove the
restrict.

DECL_BY_REFERENCE and the restrict were added in the same patch,
r0-60100-gd8472c75e8f6cdeabb60e2d743e58fb7ab46fef6, for PR16115.

[Bug c++/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[cvs-commit at gcc dot gnu.org]

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

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:96253f069ead0736536de803b06a8053a85039a6

commit r11-6919-g96253f069ead0736536de803b06a8053a85039a6
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Jan 26 16:04:24 2021 -0500

    c++: Invisible refs are not restrict [PR97474]

    In this testcase, we refer to the a parameter through a reference in its
own
    member, which we asserted couldn't happen by marking the parameter as
    'restrict'.  This assumption could also be broken if the address escapes
    from the constructor.

    gcc/cp/ChangeLog:

            PR c++/97474
            * call.c (type_passed_as): Don't mark invisiref restrict.

    gcc/testsuite/ChangeLog:

            PR c++/97474
            * g++.dg/torture/pr97474.C: New test.

[Bug c++/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[rguenther at suse dot de]

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

--- Comment #16 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 26 Jan 2021, jason at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474
> 
> Jason Merrill <jason at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>              Status|NEW                         |ASSIGNED
>            Assignee|unassigned at gcc dot gnu.org      |jason at gcc dot gnu.org
> 
> --- Comment #14 from Jason Merrill <jason at gcc dot gnu.org> ---
> (In reply to rguenther@suse.de from comment #13)
> 
> Indeed, some useful information is that the parameter is a reference to the
> most-derived object, not a base class; that should be useful to
> devirtualization, particularly.
> 
> Looks like we already have DECL_BY_REFERENCE to indicate this situation, and
> ipa-polymorphic-call already looks at it, so I'm going to just remove the
> restrict.
> 
> DECL_BY_REFERENCE and the restrict were added in the same patch,
> r0-60100-gd8472c75e8f6cdeabb60e2d743e58fb7ab46fef6, for PR16115.

Thanks - I'll see to deal with any PTA fallout if it arises.

[Bug c++/97474] [8/9/10/11 Regression] produces wrong code with references to another field

[cvs-commit at gcc dot gnu.org]

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

--- Comment #17 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jason Merrill
<jason@gcc.gnu.org>:

https://gcc.gnu.org/g:85908100051cb63b2fd2c039f3a166d45386b9d9

commit r10-9313-g85908100051cb63b2fd2c039f3a166d45386b9d9
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Jan 26 16:04:24 2021 -0500

    c++: Invisible refs are not restrict [PR97474]

    In this testcase, we refer to the a parameter through a reference in its
own
    member, which we asserted couldn't happen by marking the parameter as
    'restrict'.  This assumption could also be broken if the address escapes
    from the constructor.

    gcc/cp/ChangeLog:

            PR c++/97474
            * call.c (type_passed_as): Don't mark invisiref restrict.

    gcc/testsuite/ChangeLog:

            PR c++/97474
            * g++.dg/torture/pr97474.C: New test.

[Bug c++/97474] [8/9 Regression] produces wrong code with references to another field

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[8/9/10/11 Regression]      |[8/9 Regression] produces
                   |produces wrong code with    |wrong code with references
                   |references to another field |to another field

--- Comment #18 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 10.3+/11+.

[Bug c++/97474] [9 Regression] produces wrong code with references to another field

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|8.5                         |9.4

--- Comment #19 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 8 branch is being closed.

[Bug c++/97474] [9 Regression] produces wrong code with references to another field

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.4                         |9.5

--- Comment #20 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 9.4 is being released, retargeting bugs to GCC 9.5.

[Bug c++/97474] [9 Regression] produces wrong code with references to another field

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #21 from Jason Merrill <jason at gcc dot gnu.org> ---
Not backporting to 9.

[Bug c++/97474] [9 Regression] produces wrong code with references to another field

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.5                         |10.3