Re: [PATCH] Restore defensive check in tree_nop_conversion (also PR89779 "fix")

[Christophe Lyon <christophe.lyon@linaro.org>]

On Thu, 21 Mar 2019 at 14:44, Richard Biener <rguenther@suse.de> wrote:
>
> On Thu, 21 Mar 2019, Richard Biener wrote:
>
> >
> > This also avoids the ICE in PR89779 but IMHO is not a real fix.
> >
> > Still it restores a previously active check against released SSA names
> > which now have error_mark_node type rather than NULL.  The new way
> > opens up consolidation so I've adjusted tree_nop_conversion plus
> > operand_equal_p which got a defensive check at the same time
> > (and checks for error_mark_node right before that check).
> >
> > Bootstrap & regtest running on x86_64-unknown-linux-gnu.
> >
> > The testcase comes with the other patch (but still included below).
>
> Ugh, looks like my defense installed for released SSA names is now
> relied upon by the C++ FE via
>
> #2  0x0000000001864a96 in tree_strip_nop_conversions (
>     exp=<convert_expr 0x7ffff69bc920>)
>     at /space/rguenther/src/svn/trunk2/gcc/tree.c:12848
> #3  0x0000000000ab2d2f in iterative_hash_template_arg (
>     arg=<convert_expr 0x7ffff69bc920>, val=3363282904)
>     at /space/rguenther/src/svn/trunk2/gcc/cp/pt.c:1751
>
> for
>
>  <convert_expr 0x7ffff69bc920
>     type <integer_type 0x7ffff68985e8 int public type_6 SI
>         size <integer_cst 0x7ffff689c078 constant 32>
>         unit-size <integer_cst 0x7ffff689c090 constant 4>
>         align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
> 0x7ffff68985e8 precision:32 min <integer_cst 0x7ffff689c030 -2147483648>
> max <integer_cst 0x7ffff689c048 2147483647>
>         pointer_to_this <pointer_type 0x7ffff68a19d8>>
>
>     arg:0 <scope_ref 0x7ffff69d0b40 tree_0
>         arg:0 <template_type_parm 0x7ffff69d17e0 i tree_0 type_0 type_6
> VOID
>             align:8 warn_if_not_align:0 symtab:0 alias-set -1
> canonical-type 0x7ffff69d1150
>            index 0 level 1 orig_level 1
>             chain <type_decl 0x7ffff69d2098 i>>
>         arg:1 <identifier_node 0x7ffff69c39c0 c
>             normal local bindings <(nil)>>>>
>
> where both the SCOPE_REF and TEMPLATE_TYPE_PARM have NULL
> TREE_TYPE.
>
> So I'll restore the NULL check in tree_nop_conversion.
>
> But since the C++ FE overloads almost every tree field possible
> I wonder whether that STRIP_NOPS in iterative_hash_template_arg
> is a good idea?
>
> Also I hope the FE doesn't call into operand_equal_p with such
> typed trees.
>
> Updated patch below, retesting now.
>
> Thanks,
> Richard.
>
> 2019-03-21  Richard Biener  <rguenther@suse.de>
>
>         PR tree-optimization/89779
>         * tree.c (tree_nop_conversion): Consolidate and fix defensive
>         checks with respect to released SSA names now having error_mark_node
>         type.
>         * fold-const.c (operand_equal_p): Likewise.
>
Hi,

I think this patch is causing an ICE when building gdb-8.2. I'm seeing:
/home/tcwg-buildslave/workspace/tcwg-buildfarm_0/_build/builds/destdir/x86_64-unknown-linux-gnu/include/c++/9.0.1/bits/stl_iterator_base_funcs.h:
In function 'constexpr void std::__advance(_RandomAccessIter
ator&, _Distance, std::random_access_iterator_tag)':
/home/tcwg-buildslave/workspace/tcwg-buildfarm_0/_build/builds/destdir/x86_64-unknown-linux-gnu/include/c++/9.0.1/bits/stl_iterator_base_funcs.h:182:54:
internal compiler error: Segmentation fault
  182 |       else if (__builtin_constant_p(__n) && __n == -1)
      |                                                      ^
0xf9964f crash_signal
        /home/tcwg-buildslave/workspace/tcwg-buildfarm_0/snapshots/gcc.git~master_rev_ae5efb2cfe93a4277b5fa185c2c8582592b47f7a/gcc/toplev.c:326
0xf9964f crash_signal
        /home/tcwg-buildslave/workspace/tcwg-buildfarm_0/snapshots/gcc.git~master_rev_ae5efb2cfe93a4277b5fa185c2c8582592b47f7a/gcc/toplev.c:326
0xf9964f crash_signal
        /home/tcwg-buildslave/workspace/tcwg-buildfarm_0/snapshots/gcc.git~master_rev_ae5efb2cfe93a4277b5fa185c2c8582592b47f7a/gcc/toplev.c:326
0xc980dc operand_equal_p(tree_node const*, tree_node const*, unsigned int)
        /home/tcwg-buildslave/workspace/tcwg-buildfarm_0/snapshots/gcc.git~master_rev_ae5efb2cfe93a4277b5fa185c2c8582592b47f7a/gcc/fold-const.c:2977
0xc9829a operand_equal_p(tree_node const*, tree_node const*, unsigned int)
        /home/tcwg-buildslave/workspace/tcwg-buildfarm_0/snapshots/gcc.git~master_rev_ae5efb2cfe93a4277b5fa185c2c8582592b47f7a/gcc/fold-const.c:2950
0x9c1cf5 cp_parser_selection_statement
        /home/tcwg-buildslave/workspace/tcwg-buildfarm_0/snapshots/gcc.git~master_rev_ae5efb2cfe93a4277b5fa185c2c8582592b47f7a/gcc/cp/parser.c:11784
0x9c1cf5 cp_parser_statement
        /home/tcwg-buildslave/workspace/tcwg-buildfarm_0/snapshots/gcc.git~master_rev_ae5efb2cfe93a4277b5fa185c2c8582592b47f7a/gcc/cp/parser.c:11147
0x9db9a7 cp_parser_implicitly_scoped_statement
        /home/tcwg-buildslave/workspace/tcwg-buildfarm_0/snapshots/gcc.git~master_rev_ae5efb2cfe93a4277b5fa185c2c8582592b47f7a/gcc/cp/parser.c:13004
0x9c1c37 cp_parser_selection_statement
        /home/tcwg-buildslave/workspace/tcwg-buildfarm_0/snapshots/gcc.git~master_rev_ae5efb2cfe93a4277b5fa185c2c8582592b47f7a/gcc/cp/parser.c:11865
0x9c1c37 cp_parser_statement
        /home/tcwg-buildslave/workspace/tcwg-buildfarm_0/snapshots/gcc.git~master_rev_ae5efb2cfe93a4277b5fa185c2c8582592b47f7a/gcc/cp/parser.c:11147
0x9c2218 cp_parser_statement_seq_opt
        /home/tcwg-buildslave/workspace/tcwg-buildfarm_0/snapshots/gcc.git~master_rev_ae5efb2cfe93a4277b5fa185c2c8582592b47f7a/gcc/cp/parser.c:11628
0x9c22f8 cp_parser_compound_statement
[...]

while compiling linux-procfs.o, gdb.o, linux-waitpid.o, arch/amd64.o,
linux-ptrace.o and others

Seeing that on x86_64, arm, aarch64.

Christophe

> Index: gcc/tree.c
> ===================================================================
> --- gcc/tree.c  (revision 269832)
> +++ gcc/tree.c  (working copy)
> @@ -12812,13 +12812,10 @@ tree_nop_conversion (const_tree exp)
>    if (!CONVERT_EXPR_P (exp)
>        && TREE_CODE (exp) != NON_LVALUE_EXPR)
>      return false;
> -  if (TREE_OPERAND (exp, 0) == error_mark_node)
> -    return false;
>
>    outer_type = TREE_TYPE (exp);
>    inner_type = TREE_TYPE (TREE_OPERAND (exp, 0));
> -
> -  if (!inner_type)
> +  if (!inner_type || inner_type == error_mark_node)
>      return false;
>
>    return tree_nop_conversion_p (outer_type, inner_type);
> Index: gcc/fold-const.c
> ===================================================================
> --- gcc/fold-const.c    (revision 269832)
> +++ gcc/fold-const.c    (working copy)
> @@ -2973,11 +2973,6 @@ operand_equal_p (const_tree arg0, const_
>        || TREE_TYPE (arg1) == error_mark_node)
>      return 0;
>
> -  /* Similar, if either does not have a type (like a released SSA name),
> -     they aren't equal.  */
> -  if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1))
> -    return 0;
> -
>    /* We cannot consider pointers to different address space equal.  */
>    if (POINTER_TYPE_P (TREE_TYPE (arg0))
>        && POINTER_TYPE_P (TREE_TYPE (arg1))
> Index: gcc/testsuite/gcc.dg/torture/pr89779.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/torture/pr89779.c      (nonexistent)
> +++ gcc/testsuite/gcc.dg/torture/pr89779.c      (working copy)
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +
> +typedef int a;
> +void h(a);
> +void c(a *d, int b)
> +{
> +  int e, f, g;
> +  for (; e; e++)
> +    for (f = 0; f < 4; f++)
> +      if (d)
> +       for (g = e + 1; g; g++)
> +         h(d[g]);
> +}
> +void i()
> +{
> +  a *j;
> +  int k, l;
> +  for (; k; k++)
> +    c(j, l);
> +}
>