On Sun, Jun 29, 2014 at 10:00:34PM +0200, Jan Hubicka wrote:
> > On Sun, 29 Jun 2014, Chen Gang wrote:
> >
> > > NEWDECL dereferences shared data of OLDDECL, or when free NEWDECL, also
> > > have effect with OLDDECL. At present, only fix the related reference for
> > > current issue. If find another related issue, can follow this fix.
> >
> > I'm afraid I can't make any sense of this.
> >
> > We need a carefully written analysis of the issue you are trying to fix
> > that explains what is going wrong and why, in terms of the underlying
> > design of the code, the proposed change is logically correct to fix the
> > issue.
> >
> > That is, explain how NEWDECL gets used after merge_decls returns, why your
> > proposed changes to NEWDECL are correct for the subsequent use of NEWDECL,
> > why the previous contents of NEWDECL would be incorrect for the subsequent
> > uses of NEWDECL, and why it is correct for the subsequent uses to make
> > whatever use of NEWDECL is causing problems. Where does the segfault
> > occur anyway?
>
> Actually this problem was introduced by me (and I made patch for it once
> already
> but apparently it did not reach the ML). THe problem is that we now create
> symbols
> to hold information about decl that is no longer represented in trees -
> sections,
> comdats etc.
>
> Now in duplicate_decl we have two symbol nodes for the duplicated declarations
> and remove one of them at the end. cgraph_remove_node calls
> cgraph_release_function_body that assumes that we never have two functions
> with
> same DECL_STRUCT_FUNCTION.
>
> So Cheng is right we need to clear DECL_STRUCT_FUNCTION or we end up
> ggc_freeing
> it that later ICEs in push_cfun. We do not need to clear the other fields.
>
> The patch bellow just copy identical hunk of code from cp/decl.c
> >
> > > root@gchen:/upstream/linux# cat elevator.i
> > > extern int __attribute__ ((__section__(".init.text")))
> > > elv_register(void)
> > > {
> > > return 0;
> > > }
> > > extern typeof(elv_register) elv_register;
> >
> > Patch submissions should add a test to the testsuite, when it's something
> > small like this.
>
> Added.
>
> thanks for working on this! I am bootstrapping/regtesting the attacked patch
> on x86_64-linux
> OK?
Just some typos:
> Chen Gang <[email protected]>
Two spaces after the name.
> Jan Hubicka <[email protected]>
>
> * c-decl.c (duplicate_decls): CLear DECL_STRUCT_FUNCTION before
> releasing
> symbol.
s/CLear/Clear/
> Index: c/c-decl.c
> ===================================================================
> --- c/c-decl.c (revision 212098)
> +++ c/c-decl.c (working copy)
> @@ -2575,7 +2575,14 @@ duplicate_decls (tree newdecl, tree oldd
>
> merge_decls (newdecl, olddecl, newtype, oldtype);
>
> - /* The NEWDECL will no longer be needed. */
> + /* The NEWDECL will no longer be needed.
> +
> + Before releasing the node, be sore to remove function from symbol
s/sore/sure/
> + table that might have been inserted there to record comdat group.
> + Be sure to however do not free DECL_STRUCT_FUNCTION becuase this
s/becuase/because/
> + structure is shared in between newdecl and oldecl. */
s/newdecl/NEWDECL/;s/oldecl/OLDDECL/
Marek
