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

[Martin Jambor]

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 <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.

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