Re: [PATCH] gcc/c/c-decl.c: Let NEWDECL dereference shared data to fix segment fault issue.

[Marek Polacek <polacek@redhat.com>]

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 <gang.chen.5i5j@gmail.com>

Two spaces after the name.

> 	Jan Hubicka  <hubicka@ucw.cz>
> 
> 	* 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