Re: [PATCH] Fix bogus use of cfun in gen_subprogram_die and premark_used_types

[Richard Guenther <richard dot guenther at gmail dot com>]

On Tue, Sep 4, 2012 at 3:12 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> while looking into how to remove push/pop_cfun from dwarf2out.c, I
> have noticed the following wrong use of cfun in premark_used_types,
> which is the first thing called by gen_subprogram_die.
>
> What happens is that:
>
> 1. early inliner calls dwarf2out_abstract_function, cfun corresponds
>    to the function being inlined to, argument decl is the function
>    being inlined.
>
> 2. dwarf2out_abstract_function calls gen_type_die_for_member to
>    generate an "in-class declaration DIE."  It does this before
>    changing cfun.
>
> 3. gen_type_die_for_member calls gen_type_die_for_member because
>    member is a function decl.
>
> 4. gen_subprogram_die calls premark_used_types to mark DIEs of all
>    types in cfun->used_types_hash as perennial.  But cfun does not
>    correspond to the decl it is supposed to be emitting a DIE for,
>    instead, used_types of the function decl is being inlined to are
>    being marked as perennial.
>
> Similarly, when dealing with nested functions, gen_subprogram_die can
> call itself, just with a different decl parameter but unchanged cfun
> through decls_for_scope.
>
> I was not able to produce a failing testcase similar to
> gcc.dg/20060410.c, mainly because dwarf2out_abstract_function then
> changes cfun and indirectly invokes gen_subprogram_die again but still
> I believe the intention was to use DECL_STRUCT_FUNCTION(decl) rather
> than cfun in premark_used_types and everywhere in gen_subprogram_die.
> The patch below does exactly that and as far as my experiments go,
> seems to work.
>
> This patch also removes push/pop cfun from dwarf2out_abstract_function
> and only leaves the change of current_function_decl.  Richi suggested
> that we push NULL cfun at this point but my goal is to enforce that
> cfun and current_function_decl match at each push_cfun and since
> dwarf2out_abstract_function can call itself, that is not the case.
> Nevertheless, I also bootstrapped, tested and compiled Firefox with a
> version in which I do push_cfun(NULL) when cfun is not already NULL
> and there were no problems.
>
> Bootstrapped and tested on x86_64-linux.  OK for trunk?

Ok if nobody objects.  Note that I see some uses of cfun in dwarf2out.c
that are protected by cfun &&, so those may be still broken even though
you tested with push_cfun (NULL).  We should add more struct function
arguments to functions accessing cfun in dwarf2out.c.

Thanks,
Richard.

> Thanks,
>
> Martin
>
>
> 2012-08-30  Martin Jambor  <mjambor@suse.cz>
>
>         * dwarf2out.c (dwarf2out_abstract_function): Do not change cfun.
>         (premark_used_types): New parameter fun, use it instead of cfun.
>         (gen_subprogram_die): Use DECL_STRUCT_FUNCTION (decl) instead of cfun,
>         also pass it to premark_used_types.
>
>
> *** /tmp/3GCMxa_dwarf2out.c     Tue Sep  4 15:10:23 2012
> --- gcc/dwarf2out.c     Mon Sep  3 14:48:02 2012
> *************** dwarf2out_abstract_function (tree decl)
> *** 16765,16771 ****
>     /* Pretend we've just finished compiling this function.  */
>     save_fn = current_function_decl;
>     current_function_decl = decl;
> -   push_cfun (DECL_STRUCT_FUNCTION (decl));
>
>     was_abstract = DECL_ABSTRACT (decl);
>     set_decl_abstract_flags (decl, 1);
> --- 16765,16770 ----
> *************** dwarf2out_abstract_function (tree decl)
> *** 16779,16785 ****
>     call_arg_locations = old_call_arg_locations;
>     call_site_count = old_call_site_count;
>     tail_call_site_count = old_tail_call_site_count;
> -   pop_cfun ();
>   }
>
>   /* Helper function of premark_used_types() which gets called through
> --- 16778,16783 ----
> *************** premark_types_used_by_global_vars_helper
> *** 16838,16847 ****
>   /* Mark all members of used_types_hash as perennial.  */
>
>   static void
> ! premark_used_types (void)
>   {
> !   if (cfun && cfun->used_types_hash)
> !     htab_traverse (cfun->used_types_hash, premark_used_types_helper, NULL);
>   }
>
>   /* Mark all members of types_used_by_vars_entry as perennial.  */
> --- 16836,16845 ----
>   /* Mark all members of used_types_hash as perennial.  */
>
>   static void
> ! premark_used_types (struct function *fun)
>   {
> !   if (fun && fun->used_types_hash)
> !     htab_traverse (fun->used_types_hash, premark_used_types_helper, NULL);
>   }
>
>   /* Mark all members of types_used_by_vars_entry as perennial.  */
> *************** gen_subprogram_die (tree decl, dw_die_re
> *** 16904,16910 ****
>     int declaration = (current_function_decl != decl
>                      || class_or_namespace_scope_p (context_die));
>
> !   premark_used_types ();
>
>     /* It is possible to have both DECL_ABSTRACT and DECLARATION be true if we
>        started to generate the abstract instance of an inline, decided to output
> --- 16902,16908 ----
>     int declaration = (current_function_decl != decl
>                      || class_or_namespace_scope_p (context_die));
>
> !   premark_used_types (DECL_STRUCT_FUNCTION (decl));
>
>     /* It is possible to have both DECL_ABSTRACT and DECLARATION be true if we
>        started to generate the abstract instance of an inline, decided to output
> *************** gen_subprogram_die (tree decl, dw_die_re
> *** 17067,17079 ****
>     else if (!DECL_EXTERNAL (decl))
>       {
>         HOST_WIDE_INT cfa_fb_offset;
>
>         if (!old_die || !get_AT (old_die, DW_AT_inline))
>         equate_decl_number_to_die (decl, subr_die);
>
>         if (!flag_reorder_blocks_and_partition)
>         {
> !         dw_fde_ref fde = cfun->fde;
>           if (fde->dw_fde_begin)
>             {
>               /* We have already generated the labels.  */
> --- 17065,17079 ----
>     else if (!DECL_EXTERNAL (decl))
>       {
>         HOST_WIDE_INT cfa_fb_offset;
> +       struct function *fun = DECL_STRUCT_FUNCTION (decl);
>
>         if (!old_die || !get_AT (old_die, DW_AT_inline))
>         equate_decl_number_to_die (decl, subr_die);
>
> +       gcc_checking_assert (fun);
>         if (!flag_reorder_blocks_and_partition)
>         {
> !         dw_fde_ref fde = fun->fde;
>           if (fde->dw_fde_begin)
>             {
>               /* We have already generated the labels.  */
> *************** gen_subprogram_die (tree decl, dw_die_re
> *** 17119,17125 ****
>         else
>         {
>           /* Generate pubnames entries for the split function code ranges.  */
> !         dw_fde_ref fde = cfun->fde;
>
>           if (fde->dw_fde_second_begin)
>             {
> --- 17119,17125 ----
>         else
>         {
>           /* Generate pubnames entries for the split function code ranges.  */
> !         dw_fde_ref fde = fun->fde;
>
>           if (fde->dw_fde_second_begin)
>             {
> *************** gen_subprogram_die (tree decl, dw_die_re
> *** 17219,17227 ****
>          by this displacement.  */
>         compute_frame_pointer_to_fb_displacement (cfa_fb_offset);
>
> !       if (cfun->static_chain_decl)
>         add_AT_location_description (subr_die, DW_AT_static_link,
> !                loc_list_from_tree (cfun->static_chain_decl, 2));
>       }
>
>     /* Generate child dies for template paramaters.  */
> --- 17219,17227 ----
>          by this displacement.  */
>         compute_frame_pointer_to_fb_displacement (cfa_fb_offset);
>
> !       if (fun->static_chain_decl)
>         add_AT_location_description (subr_die, DW_AT_static_link,
> !                loc_list_from_tree (fun->static_chain_decl, 2));
>       }
>
>     /* Generate child dies for template paramaters.  */