* [PATCH] gcc/c/c-decl.c: Let NEWDECL dereference shared data to fix segment fault issue.
@ 2014-06-29 12:52 Chen Gang
2014-06-29 13:30 ` Joseph S. Myers
0 siblings, 1 reply; 6+ messages in thread
From: Chen Gang @ 2014-06-29 12:52 UTC (permalink / raw)
To: Joseph S. Myers, rth, hubicka
Cc: gcc-patches, davem, Jeff Law, Guenter Roeck, Bin.Cheng
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.
The related git number is 71e19e54060804493e13748613077b0e69c0cfd9, and
the related information:
root@gchen:/upstream/linux# cat elevator.i
extern int __attribute__ ((__section__(".init.text"))) elv_register(void)
{
return 0;
}
extern typeof(elv_register) elv_register;
root@gchen:/upstream/linux# /usr/local/libexec/gcc/score-elf/4.10.0/cc1 elevator.i
elv_register
Analyzing compilation unit
Segmentation fault (core dumped)
root@gchen:/upstream/linux# /usr/local/bin/score-elf-gcc -v
Using built-in specs.
COLLECT_GCC=/usr/local/bin/score-elf-gcc
COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc/score-elf/4.10.0/lto-wrapper
Target: score-elf
Configured with: ../gcc/configure --without-header --disable-nls --enable-language=c --disable-threads --disable-shared --enable-werror=no target_configargs=enable_vtable_verify=yes --target=score-elf --enable-obsolete : (reconfigured) ../gcc/configure --without-header --disable-nls --enable-language=c --disable-threads --disable-shared --enable-werror=no target_configargs=enable_vtable_verify=yes --target=score-elf --enable-obsolete --enable-debug --disable-release
Thread model: single
gcc version 4.10.0 20140625 (experimental) (GCC)
After this patch, can let Linux kernel pass defconfig under score.
Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
gcc/ChangeLog | 5 +++++
gcc/c/c-decl.c | 11 +++++++++++
2 files changed, 16 insertions(+)
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 34e7c93..241a7b7 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2014-06-29 Chen Gang <gang.chen.5i5j@gmail.com>
+
+ * c/c-decl.cl (merge_decls): NEWDECL dereferences shared data of
+ OLDDECL, or when free NEWDECL, also have effect with OLDDECL.
+
2014-06-28 Jan Hubicka <hubicka@ucw.cz>
* tree-streamer-out.c (pack_ts_type_common_value_fields): Stream if type
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index def10a2..373e5fb 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -2552,6 +2552,17 @@ merge_decls (tree newdecl, tree olddecl, tree newtype, tree oldtype)
|| (TREE_CODE (olddecl) == VAR_DECL
&& TREE_STATIC (olddecl))))
make_decl_rtl (olddecl);
+
+ /* NEWDECL dereferences shared data of OLDDECL, or after free NEWDECL,
+ also have effect with OLDDECL */
+ if (TREE_CODE (newdecl) == FUNCTION_DECL)
+ {
+ DECL_FUNCTION_SPECIFIC_TARGET (newdecl) = NULL;
+ DECL_FUNCTION_SPECIFIC_OPTIMIZATION (newdecl) = NULL;
+ DECL_STRUCT_FUNCTION (newdecl) = NULL;
+ DECL_RESULT (newdecl) = NULL;
+ DECL_INITIAL (newdecl) = NULL;
+ }
}
/* Handle when a new declaration NEWDECL has the same name as an old
--
1.7.11.7
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gcc/c/c-decl.c: Let NEWDECL dereference shared data to fix segment fault issue.
2014-06-29 12:52 [PATCH] gcc/c/c-decl.c: Let NEWDECL dereference shared data to fix segment fault issue Chen Gang
@ 2014-06-29 13:30 ` Joseph S. Myers
2014-06-29 20:00 ` Jan Hubicka
0 siblings, 1 reply; 6+ messages in thread
From: Joseph S. Myers @ 2014-06-29 13:30 UTC (permalink / raw)
To: Chen Gang
Cc: rth, hubicka, gcc-patches, davem, Jeff Law, Guenter Roeck, Bin.Cheng
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?
> 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.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gcc/c/c-decl.c: Let NEWDECL dereference shared data to fix segment fault issue.
2014-06-29 13:30 ` Joseph S. Myers
@ 2014-06-29 20:00 ` Jan Hubicka
2014-06-29 20:59 ` Marek Polacek
2014-06-29 21:24 ` Joseph S. Myers
0 siblings, 2 replies; 6+ messages in thread
From: Jan Hubicka @ 2014-06-29 20:00 UTC (permalink / raw)
To: Joseph S. Myers
Cc: Chen Gang, rth, hubicka, gcc-patches, davem, Jeff Law,
Guenter Roeck, Bin.Cheng
> 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?
Honza
Chen Gang <gang.chen.5i5j@gmail.com>
Jan Hubicka <hubicka@ucw.cz>
* c-decl.c (duplicate_decls): CLear DECL_STRUCT_FUNCTION before releasing
symbol.
* gcc.c-torture/compile/section.c: New testcase
Index: testsuite/gcc.c-torture/compile/section.c
===================================================================
--- testsuite/gcc.c-torture/compile/section.c (revision 0)
+++ testsuite/gcc.c-torture/compile/section.c (revision 0)
@@ -0,0 +1,8 @@
+/* { dg-do compile } */
+
+/* { dg-require-effective-target named_sections } */
+extern int __attribute__ ((__section__(".init.text"))) elv_register(void)
+{
+ return 0;
+}
+extern typeof(elv_register) elv_register;
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
+ table that might have been inserted there to record comdat group.
+ Be sure to however do not free DECL_STRUCT_FUNCTION becuase this
+ structure is shared in between newdecl and oldecl. */
+ if (TREE_CODE (newdecl) == FUNCTION_DECL)
+ DECL_STRUCT_FUNCTION (newdecl) = NULL;
if (TREE_CODE (newdecl) == FUNCTION_DECL
|| TREE_CODE (newdecl) == VAR_DECL)
{
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gcc/c/c-decl.c: Let NEWDECL dereference shared data to fix segment fault issue.
2014-06-29 20:00 ` Jan Hubicka
@ 2014-06-29 20:59 ` Marek Polacek
2014-06-29 21:24 ` Joseph S. Myers
1 sibling, 0 replies; 6+ messages in thread
From: Marek Polacek @ 2014-06-29 20:59 UTC (permalink / raw)
To: Jan Hubicka
Cc: Joseph S. Myers, Chen Gang, rth, gcc-patches, davem, Jeff Law,
Guenter Roeck, Bin.Cheng
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gcc/c/c-decl.c: Let NEWDECL dereference shared data to fix segment fault issue.
2014-06-29 20:00 ` Jan Hubicka
2014-06-29 20:59 ` Marek Polacek
@ 2014-06-29 21:24 ` Joseph S. Myers
2014-06-30 1:26 ` Chen Gang
1 sibling, 1 reply; 6+ messages in thread
From: Joseph S. Myers @ 2014-06-29 21:24 UTC (permalink / raw)
To: Jan Hubicka
Cc: Chen Gang, rth, gcc-patches, davem, Jeff Law, Guenter Roeck, Bin.Cheng
On Sun, 29 Jun 2014, Jan Hubicka wrote:
> 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.
Thanks for the explanation. This patch is OK with the typo fixes Marek
has noted.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gcc/c/c-decl.c: Let NEWDECL dereference shared data to fix segment fault issue.
2014-06-29 21:24 ` Joseph S. Myers
@ 2014-06-30 1:26 ` Chen Gang
0 siblings, 0 replies; 6+ messages in thread
From: Chen Gang @ 2014-06-30 1:26 UTC (permalink / raw)
To: Joseph S. Myers, Jan Hubicka
Cc: rth, gcc-patches, davem, Jeff Law, Guenter Roeck, Bin.Cheng
On 06/30/2014 05:23 AM, Joseph S. Myers wrote:
> On Sun, 29 Jun 2014, Jan Hubicka wrote:
>
>> 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.
>
> Thanks for the explanation. This patch is OK with the typo fixes Marek
> has noted.
>
Thank you very much for all of your work!
And I shall continue to find and try to solve other gcc issues, hope I
can provide a patch per month.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-30 1:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-29 12:52 [PATCH] gcc/c/c-decl.c: Let NEWDECL dereference shared data to fix segment fault issue Chen Gang
2014-06-29 13:30 ` Joseph S. Myers
2014-06-29 20:00 ` Jan Hubicka
2014-06-29 20:59 ` Marek Polacek
2014-06-29 21:24 ` Joseph S. Myers
2014-06-30 1:26 ` Chen Gang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).