On Tue, Sep 4, 2012 at 3:12 PM, Martin Jambor <mjam...@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 <mjam...@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. */