[Bug c++/100468] New: set_up_extended_ref_temp via extend_ref_init_temps_1 drops TREE_ADDRESSABLE

[Bug c++/100468] New: set_up_extended_ref_temp via extend_ref_init_temps_1 drops TREE_ADDRESSABLE

[rguenth at gcc dot gnu.org]

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

            Bug ID: 100468
           Summary: set_up_extended_ref_temp via extend_ref_init_temps_1
                    drops TREE_ADDRESSABLE
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
  Target Milestone: ---

The g++.dg/cpp0x/constexpr-ice12.C shows that when we resolve the initializer
from

(int &) &TARGET_EXPR <D.2084, NON_LVALUE_EXPR <0>>

to

(int &) &_ZGRN1A1iE_

we fail to mark _ZGRN1A1iE_ TREE_ADDRESSABLE even though both the
TARGET_EXPR and its var were.  The following resolves this, but I'm
not sure if it is the correct place to fix this.  It looks like
'expr' will always be a TARGET_EXPR since it's only a single caller.

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 57bac05fe70..ea97be22f07 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -12478,6 +12478,8 @@ set_up_extended_ref_temp (tree decl, tree expr,
vec<tree, va_gc> **cleanups,
      VAR.  */
   if (TREE_CODE (expr) != TARGET_EXPR)
     expr = get_target_expr (expr);
+  else if (TREE_ADDRESSABLE (expr))
+    TREE_ADDRESSABLE (var) = 1;

   if (TREE_CODE (decl) == FIELD_DECL
       && extra_warnings && !TREE_NO_WARNING (decl))

other affected testcases are

g++.dg/cpp2a/paren-init1.C, g++.dg/cpp2a/paren-init29.C,
g++.dg/cpp2a/paren-init4.C and g++.dg/ext/visibility/ref-temp1.C

[Bug c++/100468] set_up_extended_ref_temp via extend_ref_init_temps_1 drops TREE_ADDRESSABLE

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
The bug also causes g++.dg/tree-ssa/array-temp1.C to pass, notably the
gimplifiers gimplify_init_constructor to pass the

           && (flag_merge_constants >= 2 || !TREE_ADDRESSABLE (object))

test even though clearly the variable is address-taken, resulting in

  static const int C.1[3] = {1, 42, 3};

  try
    {
      a = {};
      a._M_len = 3;
      a._M_array = &C.1;
      _1 = std::initializer_list<int>::begin (&a);

I was tempted to add a || DECL_ARTIFICIAL (object) but Jakub says that's
probably not good.

[Bug c++/100468] set_up_extended_ref_temp via extend_ref_init_temps_1 drops TREE_ADDRESSABLE

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
What I fear is that we have some temporary with a const initializer, extend its
lifetime by taking a reference, have another temporary with the same const
initializer, extend its lifetime by taking a reference and compare the
addresses of those two references.  As those are different temporaries, their
addresses should be different, shouldn't they?

[Bug c++/100468] set_up_extended_ref_temp via extend_ref_init_temps_1 drops TREE_ADDRESSABLE

[rguenther at suse dot de]

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

--- Comment #3 from rguenther at suse dot de <rguenther at suse dot de> ---
On Fri, 7 May 2021, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100468
> 
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |jakub at gcc dot gnu.org
> 
> --- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> What I fear is that we have some temporary with a const initializer, extend its
> lifetime by taking a reference, have another temporary with the same const
> initializer, extend its lifetime by taking a reference and compare the
> addresses of those two references.  As those are different temporaries, their
> addresses should be different, shouldn't they?

If you're thinking of a specific case maybe you can come up with a
testcase we can add?  The testsuite comes out clean with the suggested
change.

[Bug c++/100468] set_up_extended_ref_temp via extend_ref_init_temps_1 drops TREE_ADDRESSABLE

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I meant something like
struct S { constexpr S () : s (0), t (0), u(0) {} constexpr S (int x, int y,
int z) : s (x), t (y), u (z) {} int s, t, u; };
constexpr S bar () { return S (0, 1, 2); }

bool
foo (void)
{
  constexpr static const S&& a = bar ();
  constexpr static const S&& b = bar ();
  return &a == &b;
}

int
main ()
{
  if (foo ())
    __builtin_abort ();
}
but haven't tried your patch to see if it triggers.
I certainly see
  static const struct S & a = (const struct S &) &_ZGRZ3foovE1a_;
  static struct S _ZGRZ3foovE1a_ = {.s=0, .t=1, .u=2};
  static const struct S & b = (const struct S &) &_ZGRZ3foovE1b_;
  static struct S _ZGRZ3foovE1b_ = {.s=0, .t=1, .u=2};
in the gimple dump and I believe the _ZGRZ3foovE1*_ vars are DECL_ARTIFICIAL.

[Bug c++/100468] set_up_extended_ref_temp via extend_ref_init_temps_1 drops TREE_ADDRESSABLE

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Though it is static and what you're talking about is making automatic into
static.  So guess we would need automatic temporary with something like { 1, 2,
3 } initializer and have reference bind to that temporary.
My C++-fu is limited.

[Bug c++/100468] set_up_extended_ref_temp via extend_ref_init_temps_1 drops TREE_ADDRESSABLE

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS 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:a076632e274abe344ca7648b7c7f299273d4cbe0

commit r12-657-ga076632e274abe344ca7648b7c7f299273d4cbe0
Author: Richard Biener <rguenther@suse.de>
Date:   Fri May 7 09:51:18 2021 +0200

    middle-end/100464 - avoid spurious TREE_ADDRESSABLE in folding debug stmts

    canonicalize_constructor_val was setting TREE_ADDRESSABLE on bases
    of ADDR_EXPRs but that's futile when we're dealing with CTOR values
    in debug stmts.  This rips out the code which was added for Java
    and should have been an assertion when we didn't have debug stmts.
    To not regress g++.dg/tree-ssa/array-temp1.C we have to adjust the
    testcase to not look for a no longer applied invalid optimization.

    2021-05-10  Richard Biener  <rguenther@suse.de>

            PR middle-end/100464
            PR c++/100468
    gcc/
            * gimple-fold.c (canonicalize_constructor_val): Do not set
            TREE_ADDRESSABLE.

    gcc/cp/
            * call.c (set_up_extended_ref_temp): Mark the temporary
            addressable if the TARGET_EXPR was.

    gcc/testsuite/
            * gcc.dg/pr100464.c: New testcase.
            * g++.dg/tree-ssa/array-temp1.C: Adjust.

[Bug c++/100468] set_up_extended_ref_temp via extend_ref_init_temps_1 drops TREE_ADDRESSABLE

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:462900ba21f5fdf865c93f693083da3179dd3151

commit r11-9591-g462900ba21f5fdf865c93f693083da3179dd3151
Author: Richard Biener <rguenther@suse.de>
Date:   Fri May 7 09:51:18 2021 +0200

    middle-end/100464 - avoid spurious TREE_ADDRESSABLE in folding debug stmts

    canonicalize_constructor_val was setting TREE_ADDRESSABLE on bases
    of ADDR_EXPRs but that's futile when we're dealing with CTOR values
    in debug stmts.  This rips out the code which was added for Java
    and should have been an assertion when we didn't have debug stmts.
    To not regress g++.dg/tree-ssa/array-temp1.C we have to adjust the
    testcase to not look for a no longer applied invalid optimization.

    2021-05-10  Richard Biener  <rguenther@suse.de>

            PR middle-end/100464
            PR c++/100468
    gcc/
            * gimple-fold.c (canonicalize_constructor_val): Do not set
            TREE_ADDRESSABLE.

    gcc/cp/
            * call.c (set_up_extended_ref_temp): Mark the temporary
            addressable if the TARGET_EXPR was.

    gcc/testsuite/
            * gcc.dg/pr100464.c: New testcase.
            * g++.dg/tree-ssa/array-temp1.C: Adjust.

    (cherry picked from commit a076632e274abe344ca7648b7c7f299273d4cbe0)