Re: [asan] Fix some asan ICEs

[Xinliang David Li <davidxl@google.com>]

On Thu, Oct 18, 2012 at 5:58 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> Dodji reported to me an ICE about NOTE_INSN_BASIC_BLOCK in a middle
> of a bb.  The following patch (cfgexpand.c hunk) fixes that.
> I then run asan cc1/cc1plus with -O2 -fasan on a portion of cc1files
> and got other ICEs, which is the reason for the two asan.c changes
> - one was that base wasn't unshared, so we could hit tree sharing
> verification failures, and the other issue is that cgraph_build_static_cdtor
> can call ggc_collect, so holding trees around in automatic variables
> across it doesn't work very well.  I'd have to build too many trees
> again, so I think easiest is to add a new GTY root.
>
> Ok for asan?
>
> 2012-10-18  Jakub Jelinek  <jakub@redhat.com>
>
>         * asan.c (build_check_stmt): Unshare base.
>
>         * asan.c (asan_ctor_statements): New variable.
>         (asan_finish_file): Use asan_ctor_statements instead
>         of ctor_statements.
>
>         * cfgexpand.c (gimple_expand_cfg): If return_label is
>         followed by NOTE_INSN_BASIC_BLOCK, emit var_ret_seq
>         after the note instead of before it.
>
> --- gcc/asan.c.jj       2012-10-17 20:43:00.000000000 +0200
> +++ gcc/asan.c  2012-10-18 14:20:23.837423900 +0200
> @@ -459,6 +459,8 @@ build_check_stmt (tree base,
>        set_immediate_dominator (CDI_DOMINATORS, else_bb, cond_bb);
>      }
>
> +  base = unshare_expr (base);
> +
>    gsi = gsi_last_bb (cond_bb);
>    g = gimple_build_assign_with_ops (TREE_CODE (base),
>                                     make_ssa_name (TREE_TYPE (base), NULL),
> @@ -748,6 +750,10 @@ asan_add_global (tree decl, tree type, V
>    CONSTRUCTOR_APPEND_ELT (v, NULL_TREE, init);
>  }
>
> +/* Needs to be GTY(()), because cgraph_build_static_cdtor may
> +   invoke ggc_collect.  */
> +static GTY(()) tree asan_ctor_statements;
> +
>  /* Module-level instrumentation.
>     - Insert __asan_init() into the list of CTORs.
>     - TODO: insert redzones around globals.
> @@ -756,12 +762,11 @@ asan_add_global (tree decl, tree type, V
>  void
>  asan_finish_file (void)
>  {
> -  tree ctor_statements = NULL_TREE;
>    struct varpool_node *vnode;
>    unsigned HOST_WIDE_INT gcount = 0;
>
>    append_to_statement_list (build_call_expr (asan_init_func (), 0),
> -                           &ctor_statements);
> +                           &asan_ctor_statements);
>    FOR_EACH_DEFINED_VARIABLE (vnode)
>      if (asan_protect_global (vnode->symbol.decl))
>        ++gcount;
> @@ -799,7 +805,7 @@ asan_finish_file (void)
>        append_to_statement_list (build_call_expr (decl, 2,
>                                                  build_fold_addr_expr (var),
>                                                  build_int_cst (uptr, gcount)),
> -                               &ctor_statements);
> +                               &asan_ctor_statements);
>
>        decl = build_fn_decl ("__asan_unregister_globals", type);
>        TREE_NOTHROW (decl) = 1;
> @@ -810,7 +816,7 @@ asan_finish_file (void)
>        cgraph_build_static_cdtor ('D', dtor_statements,
>                                  MAX_RESERVED_INIT_PRIORITY - 1);
>      }
> -  cgraph_build_static_cdtor ('I', ctor_statements,
> +  cgraph_build_static_cdtor ('I', asan_ctor_statements,
>                              MAX_RESERVED_INIT_PRIORITY - 1);
>  }
>
> --- gcc/cfgexpand.c.jj  2012-10-17 20:41:02.000000000 +0200
> +++ gcc/cfgexpand.c     2012-10-18 13:01:06.084479343 +0200
> @@ -4615,7 +4615,12 @@ gimple_expand_cfg (void)
>    insn_locations_finalize ();
>
>    if (var_ret_seq)
> -    emit_insn_after (var_ret_seq, return_label);
> +    {
> +      rtx after = return_label;
> +      if (NEXT_INSN (after) && NOTE_INSN_BASIC_BLOCK_P (NEXT_INSN (after)))
> +       after = NEXT_INSN (after);


Just a nit: NEXT_INSN (after) can be commoned.

Other than that, looks fine to me.

thanks,

David

> +      emit_insn_after (var_ret_seq, after);
> +    }
>
>    /* Zap the tree EH table.  */
>    set_eh_throw_stmt_table (cfun, NULL);
>
>         Jakub