[Bug analyzer/105287] New: [12 Regression] ICE in get_region_for_local, at analyzer/region.cc:874

[Bug analyzer/105287] New: [12 Regression] ICE in get_region_for_local, at analyzer/region.cc:874

[asolokha at gmx dot com]

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

            Bug ID: 105287
           Summary: [12 Regression] ICE in get_region_for_local, at
                    analyzer/region.cc:874
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Keywords: ice-on-valid-code
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: asolokha at gmx dot com
  Target Milestone: ---

Created attachment 52816
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52816&action=edit
Testcase

g++ 12.0.1 20220410 snapshot (g:54c5e064cc3dc3c9b3dff12b6d48dc3efd482b07) ICEs
when compiling the attached testcase, reduced from
gcc/testsuite/g++.dg/coroutines/torture/co-await-18-if-cond.C, w/ -std=c++20
-fanalyzer:

% g++-12.0.1 -std=c++20 -fanalyzer -c refdaee6.C
during IPA pass: analyzer
refdaee6.C:35:73: internal compiler error: in get_region_for_local, at
analyzer/region.cc:874
   35 | coro1 my_coro() { co_await BoolAwaiter(true) && co_await IntAwaiter(1);
}
      |                                                                        
^
0x89b2d6 ana::frame_region::get_region_for_local(ana::region_model_manager*,
tree_node*, ana::region_model_context const*) const
       
/var/tmp/portage/sys-devel/gcc-12.0.1_p20220410/work/gcc-12-20220410/gcc/analyzer/region.cc:874
0x157c5e9 ana::region_model::get_lvalue(ana::path_var,
ana::region_model_context*) const
       
/var/tmp/portage/sys-devel/gcc-12.0.1_p20220410/work/gcc-12-20220410/gcc/analyzer/region-model.cc:2138
0x157c5e9 ana::region_model::get_lvalue(tree_node*, ana::region_model_context*)
const
       
/var/tmp/portage/sys-devel/gcc-12.0.1_p20220410/work/gcc-12-20220410/gcc/analyzer/region-model.cc:2149
0x15b8915 fully_overwrites_p
       
/var/tmp/portage/sys-devel/gcc-12.0.1_p20220410/work/gcc-12-20220410/gcc/analyzer/state-purge.cc:840
0x15b8915
ana::state_purge_per_decl::process_point_backwards(ana::function_point const&,
auto_vec<ana::function_point, 0ul>*, hash_set<ana::function_point, false,
default_hash_traits<ana::function_point> >*, ana::state_purge_map const&,
ana::region_model const&)
       
/var/tmp/portage/sys-devel/gcc-12.0.1_p20220410/work/gcc-12-20220410/gcc/analyzer/state-purge.cc:928
0x15b8fae ana::state_purge_per_decl::process_worklists(ana::state_purge_map
const&, ana::region_model_manager*)
       
/var/tmp/portage/sys-devel/gcc-12.0.1_p20220410/work/gcc-12-20220410/gcc/analyzer/state-purge.cc:742
0x15bb1f1 ana::state_purge_map::state_purge_map(ana::supergraph const&,
ana::region_model_manager*, ana::logger*)
       
/var/tmp/portage/sys-devel/gcc-12.0.1_p20220410/work/gcc-12-20220410/gcc/analyzer/state-purge.cc:262
0x155f824 ana::impl_run_checkers(ana::logger*)
       
/var/tmp/portage/sys-devel/gcc-12.0.1_p20220410/work/gcc-12-20220410/gcc/analyzer/engine.cc:5728
0x156047e ana::run_checkers()
       
/var/tmp/portage/sys-devel/gcc-12.0.1_p20220410/work/gcc-12-20220410/gcc/analyzer/engine.cc:5851
0x154f108 execute
       
/var/tmp/portage/sys-devel/gcc-12.0.1_p20220410/work/gcc-12-20220410/gcc/analyzer/analyzer-pass.cc:87

[Bug analyzer/105287] [12 Regression] ICE in get_region_for_local, at analyzer/region.cc:874

[dmalcolm at gcc dot gnu.org]

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

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-04-15
                 CC|                            |iains at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #1 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Confirmed; it's an assertion failing in
ana::frame_region::get_region_for_local, due to a variable having NULL for its
DECL_CONTEXT:

(gdb) list
869             case SSA_NAME:
870               {
871                 if (tree var = SSA_NAME_VAR (expr))
872                   {
873                     if (DECL_P (var))
874                       gcc_assert (DECL_CONTEXT (var) == m_fun->decl);
875                   }

This is probably benign if assertions are disabled (perhaps the analyzer is
being more fussy than the rest of the middle-end?)

(gdb) call debug_tree(var)
 <var_decl 0x7fffea7c8750 D.2381
    type <boolean_type 0x7fffea665b28 bool sizes-gimplified public unsigned
type_6 QI
        size <integer_cst 0x7fffea667048 constant 8>
        unit-size <integer_cst 0x7fffea667060 constant 1>
        align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7fffea665b28 precision:1 min <integer_cst 0x7fffea6672a0 0> max <integer_cst
0x7fffea6672d0 1>
        pointer_to_this <pointer_type 0x7fffea7bddc8>>
    unsigned QI ../../src/gcc/testsuite/g++.dg/analyzer/pr105287.C:35:73 size
<integer_cst 0x7fffea667048 8> unit-size <integer_cst 0x7fffea667060 1>
    align:8 warn_if_not_align:0>
(gdb) p var.decl_minimal.context 
$2 = <tree 0x0>


The var_decl is "cond_var" created here in the C++ FE:

#4  0x0000000000b30ec2 in flatten_await_stmt (n=0x317b9b0,
promoted=0x7fffffffcc40, temps_used=0x7fffffffcc70, replace_in=<optimized out>)
    at ../../src/gcc/cp/coroutines.cc:2792
2792                  tree cond_var  = build_lang_decl (VAR_DECL, NULL_TREE,
cond_type);
(gdb) list
2787              /* If the condition contains an await expression, then we
need to
2788                 set that first and use a separate var.  */
2789              if (cp_walk_tree (&cond, find_any_await, &found, NULL))
2790                {
2791                  tree cond_type = TREE_TYPE (cond);
2792                  tree cond_var  = build_lang_decl (VAR_DECL, NULL_TREE,
cond_type);
2793                  DECL_ARTIFICIAL (cond_var) = true;
2794                  layout_decl (cond_var, 0);
2795                  gcc_checking_assert (!TYPE_NEEDS_CONSTRUCTING
(cond_type));
2796                  cond = build2 (INIT_EXPR, cond_type, cond_var, cond);
2797                  var_nest_node *ins
2798                    = new var_nest_node (cond_var, cond, n->prev, n);
2799                  COND_EXPR_COND (n->init) = cond_var;
2800                  flatten_await_stmt (ins, promoted, temps_used, NULL);
2801                }

Iain: shouldn't the DECL_CONTEXT be being set on var_decls created here?

[Bug analyzer/105287] [12 Regression] ICE in analyzer get_region_for_local on C++ await cond_var

[iains at gcc dot gnu.org]

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

--- Comment #2 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to David Malcolm from comment #1)
> Confirmed; it's an assertion failing in
> ana::frame_region::get_region_for_local, due to a variable having NULL for
> its DECL_CONTEXT:
> 
> (gdb) list
> 869		case SSA_NAME:
> 870		  {
> 871		    if (tree var = SSA_NAME_VAR (expr))
> 872		      {
> 873			if (DECL_P (var))
> 874			  gcc_assert (DECL_CONTEXT (var) == m_fun->decl);
> 875		      }
> 
> This is probably benign if assertions are disabled (perhaps the analyzer is
> being more fussy than the rest of the middle-end?)
> 
> (gdb) call debug_tree(var)
>  <var_decl 0x7fffea7c8750 D.2381
>     type <boolean_type 0x7fffea665b28 bool sizes-gimplified public unsigned
> type_6 QI
>         size <integer_cst 0x7fffea667048 constant 8>
>         unit-size <integer_cst 0x7fffea667060 constant 1>
>         align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
> 0x7fffea665b28 precision:1 min <integer_cst 0x7fffea6672a0 0> max
> <integer_cst 0x7fffea6672d0 1>
>         pointer_to_this <pointer_type 0x7fffea7bddc8>>
>     unsigned QI ../../src/gcc/testsuite/g++.dg/analyzer/pr105287.C:35:73
> size <integer_cst 0x7fffea667048 8> unit-size <integer_cst 0x7fffea667060 1>
>     align:8 warn_if_not_align:0>
> (gdb) p var.decl_minimal.context 
> $2 = <tree 0x0>
> 
> 
> The var_decl is "cond_var" created here in the C++ FE:
> 
> #4  0x0000000000b30ec2 in flatten_await_stmt (n=0x317b9b0,
> promoted=0x7fffffffcc40, temps_used=0x7fffffffcc70, replace_in=<optimized
> out>)
>     at ../../src/gcc/cp/coroutines.cc:2792
> 2792		      tree cond_var  = build_lang_decl (VAR_DECL, NULL_TREE,
> cond_type);
> (gdb) list
> 2787		  /* If the condition contains an await expression, then we need to
> 2788		     set that first and use a separate var.  */
> 2789		  if (cp_walk_tree (&cond, find_any_await, &found, NULL))
> 2790		    {
> 2791		      tree cond_type = TREE_TYPE (cond);
> 2792		      tree cond_var  = build_lang_decl (VAR_DECL, NULL_TREE,
> cond_type);
> 2793		      DECL_ARTIFICIAL (cond_var) = true;
> 2794		      layout_decl (cond_var, 0);
> 2795		      gcc_checking_assert (!TYPE_NEEDS_CONSTRUCTING (cond_type));
> 2796		      cond = build2 (INIT_EXPR, cond_type, cond_var, cond);
> 2797		      var_nest_node *ins
> 2798			= new var_nest_node (cond_var, cond, n->prev, n);
> 2799		      COND_EXPR_COND (n->init) = cond_var;
> 2800		      flatten_await_stmt (ins, promoted, temps_used, NULL);
> 2801		    }
> 
> Iain: shouldn't the DECL_CONTEXT be being set on var_decls created here?

I'd expect so; does build_lang_decl() not add them to the current context? [I
didn't look at the code, and memory might be faulty there].  If it doesn't then
I need to take a pass through and add the appropriate one.

[Bug analyzer/105287] [12 Regression] ICE in analyzer get_region_for_local on C++ await cond_var

[iains at gcc dot gnu.org]

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

--- Comment #3 from Iain Sandoe <iains at gcc dot gnu.org> ---
Initial analysis ...

1. The coroutines code is supposed to ensure that the local variables are given
the context of the enclosing function (whether that is the ramp or the outlined
actor).

coroutines.cc:
   2023           /* Re-write the variable's context to be in the actor func. 
*/
   2024           DECL_CONTEXT (lvar) = lvd->context;

So, one issue to resolve is why that is apparently not working for the
temporary condition variable in question.

2. DECL_CONTEXT can validly be NULL_TREE (when the decl is in the global
namespace)

GCC internals:

DECL_CONTEXT
This macro returns the enclosing namespace. The DECL_CONTEXT for the
global_namespace is NULL_TREE.

Both things need to be addressed (even if fixing [1] will make the ICE go away
for this code, there will surely be code that intentionally places vars in the
global namespace).

====

FWIW, I looked at the output of the analyser with my WIP branch (for which the
ICE does not fire) - and TBH suspect that it will not be terribly useful until
we can teach it about the outlining done for coroutines (the fact that some
values persist, intentionally, beyond the closing brace of the initial call is
going to be an interesting thing to figure out).

[Bug analyzer/105287] [12 Regression] ICE in analyzer get_region_for_local on C++ await cond_var

[iains at gcc dot gnu.org]

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

--- Comment #4 from Iain Sandoe <iains at gcc dot gnu.org> ---
candidate patch for the ICE (I am not sure if there's anything needed on the
analyser side).

https://gcc.gnu.org/pipermail/gcc-patches/2022-April/593317.html

[Bug analyzer/105287] [12 Regression] ICE in analyzer get_region_for_local on C++ await cond_var

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0

[Bug analyzer/105287] [12 Regression] ICE in analyzer get_region_for_local on C++ await cond_var

[dmalcolm at gcc dot gnu.org]

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

--- Comment #5 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks.  FWIW I've filed PR 105382 to track the various other issues I'm seeing
with -fanalyzer with coroutines (though given that we don't properly support
C++ yet, that's relatively low priority for me).

[Bug analyzer/105287] [12 Regression] ICE in analyzer get_region_for_local on C++ await cond_var

[iains at gcc dot gnu.org]

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

--- Comment #6 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to David Malcolm from comment #5)
> Thanks.  FWIW I've filed PR 105382 to track the various other issues I'm
> seeing with -fanalyzer with coroutines (though given that we don't properly
> support C++ yet, that's relatively low priority for me).

Yeah, even given bug-free coroutines (heh) I think we would need to teach the
analyser about the outlining and deferred destruction etc.

[Bug analyzer/105287] [12 Regression] ICE in analyzer get_region_for_local on C++ await cond_var

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Iain D Sandoe <iains@gcc.gnu.org>:

https://gcc.gnu.org/g:15a176a833f23e64ad38690a678bf938227ce46f

commit r12-8308-g15a176a833f23e64ad38690a678bf938227ce46f
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Sun Apr 17 20:58:28 2022 +0100

    c++, coroutines: Make sure our temporaries are in a bind expr [PR105287]

    There are a few cases where we can generate a temporary that does not need
    to be added to the coroutine frame (i.e. these are genuinely ephemeral). 
The
    intent was that unnamed temporaries should not be 'promoted' to coroutine
    frame entries.  However there was a thinko and these were not actually ever
    added to the bind expressions being generated for the expanded awaits. 
This
    meant that they were showing in the global namspace, leading to an empty
    DECL_CONTEXT and the ICE reported.

    Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

            PR c++/105287

    gcc/cp/ChangeLog:

            * coroutines.cc (maybe_promote_temps): Ensure generated temporaries
            are added to the bind expr.
            (add_var_to_bind): Fix local var naming to use portable
punctuation.
            (register_local_var_uses): Do not add synthetic names to unnamed
            temporaries.

    gcc/testsuite/ChangeLog:

            * g++.dg/coroutines/pr105287.C: New test.

[Bug analyzer/105287] [12 Regression] ICE in analyzer get_region_for_local on C++ await cond_var

[iains at gcc dot gnu.org]

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

--- Comment #8 from Iain Sandoe <iains at gcc dot gnu.org> ---
so the ICE should be fixed for gcc-12, and given tracking the issues in PR
105382  this could be closed now, I'd think.

[Bug analyzer/105287] [12/13 Regression] ICE in analyzer get_region_for_local on C++ await cond_var

[dmalcolm at gcc dot gnu.org]

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

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

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

--- Comment #9 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
I've confirmed the fix of the ICE, which happened immediately before the branch
for gcc 12; closing.

Thanks for fixing this.

Let's use PR 105382 for the remaining "analyzer doesn't understand C++
coroutines" issues.

[Bug analyzer/105287] [12/13 Regression] ICE in analyzer get_region_for_local on C++ await cond_var

[avi at scylladb dot com]

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

Avi Kivity <avi at scylladb dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |avi at scylladb dot com

--- Comment #10 from Avi Kivity <avi at scylladb dot com> ---
This causes a wrong-code regression, PR105426.

[Bug analyzer/105287] [12/13 Regression] ICE in analyzer get_region_for_local on C++ await cond_var

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Iain D Sandoe <iains@gcc.gnu.org>:

https://gcc.gnu.org/g:3d8d093e820b10a4b4b2af8949a368377c0888cb

commit r13-28-g3d8d093e820b10a4b4b2af8949a368377c0888cb
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Thu Apr 28 20:06:29 2022 +0100

    c++, coroutines: Partial reversion of r12-8308-g15a176a833f23e [PR105426].

    The changes to fix PR 105287 included a tightening of the constraints on
which
    variables are promoted to frame copies.  This has exposed that we are
failing
    to name some variables that should be promoted.

    We avoid the use of DECL_UID to build anonymous symbols since that might
not
    be stable for -fcompare-debug.

    The long-term fix is to address the cases where the naming has been missed,
    but for the short-term (and for the GCC-12 branch) backing out the
additional
    constraint is proposed.

    Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

            PR c++/105426

    gcc/cp/ChangeLog:

            * coroutines.cc (register_local_var_uses): Allow promotion of
unnamed
            temporaries to coroutine frame copies.

[Bug analyzer/105287] [12/13 Regression] ICE in analyzer get_region_for_local on C++ await cond_var

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Iain D Sandoe
<iains@gcc.gnu.org>:

https://gcc.gnu.org/g:7cc5a20ba3f05a783fb75762cfb77ccb571285ab

commit r12-8319-g7cc5a20ba3f05a783fb75762cfb77ccb571285ab
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Thu Apr 28 20:06:29 2022 +0100

    c++, coroutines: Partial reversion of r12-8308-g15a176a833f23e [PR105426].

    The changes to fix PR 105287 included a tightening of the constraints on
which
    variables are promoted to frame copies.  This has exposed that we are
failing
    to name some variables that should be promoted.

    We avoid the use of DECL_UID to build anonymous symbols since that might
not
    be stable for -fcompare-debug.

    The long-term fix is to address the cases where the naming has been missed,
    but for the short-term (and for the GCC-12 branch) backing out the
additional
    constraint is proposed.

    Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

            PR c++/105426

    gcc/cp/ChangeLog:

            * coroutines.cc (register_local_var_uses): Allow promotion of
unnamed
            temporaries to coroutine frame copies.

    (cherry picked from commit 3d8d093e820b10a4b4b2af8949a368377c0888cb)