Hi, On Tue, Sep 04, 2012 at 04:26:43PM +0200, Richard Guenther wrote: > 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.
Well, apart from premark_used_types (which gets a function parameter in my patch) and testing of cfun in an assert (which is bogus because cfun is dereferenced immediately before it) there is only one cfun test, in secname_for_decl (which, I agree, I somehow missed). The problem with replacing that one with a function parameter is that it does not access cfun data in any way, it only uses cfun as a guard whether it can access a field in crtl, which IIUC gets re-generated at each cfun change and thus cannot be somehow passed when another function is active. To fix this we'd need to make x_rtls permanent, I suppose. Short of that we can only hope that cold_section_label is the same for a function and its abstract origin and all nested functions and all functions in a class too... Thanks for the review, I will commit the patch shortly, Martin
