diff mbox series

[C++] PR 71464 ("[6/7/8 Regression] ICE on invalid code (with redeclared constructor) at -Os: Segmentation fault")

Message ID b670baa3-70d9-d47b-5988-e2a19eb9862e@oracle.com
State New
Headers show
Series [C++] PR 71464 ("[6/7/8 Regression] ICE on invalid code (with redeclared constructor) at -Os: Segmentation fault") | expand

Commit Message

Paolo Carlini March 2, 2018, 9:02 p.m. UTC 
Hi,

this error recovery ICE happens only with -Os and is just a P5 - on the 
other hand I would argue the reproducer isn't that exotic! - but seems 
fixable easily and safely: cdtor_comdat_group immediately calls 
DECL_ASSEMBLER_NAME on both arguments and of course crashes if they are 
null. Tested x86_64-linux.

Thanks, Paolo.

//////////////////
/cp
2018-03-02  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/71464
	* optimize.c (maybe_thunk_body): When HAVE_COMDAT_GROUP is true,
	bail out immediately if either fns[1] or fns[0] is null.

/testsuite
2018-03-02  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/71464
	* g++.dg/torture/pr71464.C: New.

Comments

Jason Merrill March 3, 2018, 5:13 a.m. UTC | #1 
On Fri, Mar 2, 2018 at 4:02 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> this error recovery ICE happens only with -Os and is just a P5 - on the
> other hand I would argue the reproducer isn't that exotic! - but seems
> fixable easily and safely: cdtor_comdat_group immediately calls
> DECL_ASSEMBLER_NAME on both arguments and of course crashes if they are
> null. Tested x86_64-linux.

It would make more sense to me to do this check and return right after
populate_clone_array.

Jason
Paolo Carlini March 3, 2018, 9:50 a.m. UTC | #2 
Hi,

On 03/03/2018 06:13, Jason Merrill wrote:
> On Fri, Mar 2, 2018 at 4:02 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> this error recovery ICE happens only with -Os and is just a P5 - on the
>> other hand I would argue the reproducer isn't that exotic! - but seems
>> fixable easily and safely: cdtor_comdat_group immediately calls
>> DECL_ASSEMBLER_NAME on both arguments and of course crashes if they are
>> null. Tested x86_64-linux.
> It would make more sense to me to do this check and return right after
> populate_clone_array.
Oh nice. Then, given that the existing 'if (fns[0] && ...' looked a bit 
weird to me, to be super safe I also ran the the testsuite with 
'gcc_assert (fns[0] && fns[1]);' and everything went well, it only 
triggered for the new testcase. Is the below OK?

Thanks,
Paolo.

//////////////////////
Index: cp/optimize.c
===================================================================
--- cp/optimize.c	(revision 258165)
+++ cp/optimize.c	(working copy)
@@ -261,8 +261,12 @@ maybe_thunk_body (tree fn, bool force)
 
   populate_clone_array (fn, fns);
 
+  /* Can happen during error recovery (c++/71464).  */
+  if (!fns[0] || !fns[1])
+    return 0;
+
   /* Don't use thunks if the base clone omits inherited parameters.  */
-  if (fns[0] && ctor_omit_inherited_parms (fns[0]))
+  if (ctor_omit_inherited_parms (fns[0]))
     return 0;
 
   DECL_ABSTRACT_P (fn) = false;
Index: testsuite/g++.dg/torture/pr71464.C
===================================================================
--- testsuite/g++.dg/torture/pr71464.C	(nonexistent)
+++ testsuite/g++.dg/torture/pr71464.C	(working copy)
@@ -0,0 +1,7 @@
+struct A {}; 
+
+struct B : virtual A
+{
+  B () {};
+  B () {};  // { dg-error "cannot be overloaded" }
+};
Jason Merrill March 3, 2018, 1:22 p.m. UTC | #3 
OK.

On Sat, Mar 3, 2018 at 4:50 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> On 03/03/2018 06:13, Jason Merrill wrote:
>>
>> On Fri, Mar 2, 2018 at 4:02 PM, Paolo Carlini <paolo.carlini@oracle.com>
>> wrote:
>>>
>>> this error recovery ICE happens only with -Os and is just a P5 - on the
>>> other hand I would argue the reproducer isn't that exotic! - but seems
>>> fixable easily and safely: cdtor_comdat_group immediately calls
>>> DECL_ASSEMBLER_NAME on both arguments and of course crashes if they are
>>> null. Tested x86_64-linux.
>>
>> It would make more sense to me to do this check and return right after
>> populate_clone_array.
>
> Oh nice. Then, given that the existing 'if (fns[0] && ...' looked a bit
> weird to me, to be super safe I also ran the the testsuite with 'gcc_assert
> (fns[0] && fns[1]);' and everything went well, it only triggered for the new
> testcase. Is the below OK?
>
> Thanks,
> Paolo.
>
> //////////////////////
diff mbox series

Patch

Index: cp/optimize.c
===================================================================
--- cp/optimize.c	(revision 258151)
+++ cp/optimize.c	(working copy)
@@ -276,6 +276,9 @@  maybe_thunk_body (tree fn, bool force)
     {
       /* At eof, defer creation of mangling aliases temporarily.  */
       bool save_defer_mangling_aliases = defer_mangling_aliases;
+      if (!fns[1] || !fns[0])
+	/* Can happen during error recovery (c++/71464).  */
+	return 0;
       defer_mangling_aliases = true;
       tree comdat_group = cdtor_comdat_group (fns[1], fns[0]);
       defer_mangling_aliases = save_defer_mangling_aliases;
Index: testsuite/g++.dg/torture/pr71464.C
===================================================================
--- testsuite/g++.dg/torture/pr71464.C	(nonexistent)
+++ testsuite/g++.dg/torture/pr71464.C	(working copy)
@@ -0,0 +1,7 @@ 
+struct A {}; 
+
+struct B : virtual A
+{
+  B () {};
+  B () {};  // { dg-error "cannot be overloaded" }
+};