C++ PATCH for c++/54325 (wrong error initializing abstract base class)

C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]

It's perfectly OK to initialize a base class of abstract type; it's only 
an error to create a full object of such a type.  So this patch moves 
the check from more generic initialization code out into a function 
that's definitely creating a new object.

Tested x86_64-pc-linux-gnu, applying to trunk and 4.7.

[-- Attachment #2: 54325.patch --]
[-- Type: text/x-patch, Size: 2414 bytes --]

commit 6fb305c7c88b07c429e8a39fbd514a417c5b6127
Author: jason <jason@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Fri Dec 7 04:54:27 2012 +0000

    	PR c++/54325
    	* tree.c (build_aggr_init_expr): Don't check for abstract class.
    
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index 28ff0f2..ca82f75 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -407,18 +407,13 @@ build_aggr_init_array (tree return_type, tree fn, tree slot, int nargs,
    callable.  */
 
 tree
-build_aggr_init_expr (tree type, tree init, tsubst_flags_t complain)
+build_aggr_init_expr (tree type, tree init, tsubst_flags_t /*complain*/)
 {
   tree fn;
   tree slot;
   tree rval;
   int is_ctor;
 
-  /* Make sure that we're not trying to create an instance of an
-     abstract class.  */
-  if (abstract_virtuals_error_sfinae (NULL_TREE, type, complain))
-    return error_mark_node;
-
   if (TREE_CODE (init) == CALL_EXPR)
     fn = CALL_EXPR_FN (init);
   else if (TREE_CODE (init) == AGGR_INIT_EXPR)
@@ -477,6 +472,11 @@ build_cplus_new (tree type, tree init, tsubst_flags_t complain)
   tree rval = build_aggr_init_expr (type, init, complain);
   tree slot;
 
+  /* Make sure that we're not trying to create an instance of an
+     abstract class.  */
+  if (abstract_virtuals_error_sfinae (NULL_TREE, type, complain))
+    return error_mark_node;
+
   if (TREE_CODE (rval) == AGGR_INIT_EXPR)
     slot = AGGR_INIT_EXPR_SLOT (rval);
   else if (TREE_CODE (rval) == CALL_EXPR
diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-pure.C b/gcc/testsuite/g++.dg/cpp0x/initlist-pure.C
new file mode 100644
index 0000000..63c341c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/initlist-pure.C
@@ -0,0 +1,25 @@
+// PR c++/54325
+// { dg-options -std=c++11 }
+
+class Base {
+public:
+  Base() {};
+  virtual ~Base() {};
+
+  virtual void do_stuff() = 0;
+};
+
+class Derived: public Base {
+public:
+  Derived() : Base{} {};
+  virtual ~Derived() {};
+
+  virtual void do_stuff() {};
+};
+
+int
+main() {
+  Derived d;
+
+  return 0;
+}
diff --git a/gcc/testsuite/g++.dg/other/abstract3.C b/gcc/testsuite/g++.dg/other/abstract3.C
index 528b7d7..95e293e 100644
--- a/gcc/testsuite/g++.dg/other/abstract3.C
+++ b/gcc/testsuite/g++.dg/other/abstract3.C
@@ -8,5 +8,5 @@ struct A                  // { dg-message "note" }
 struct B
 {
   A a;           // { dg-error "abstract" }
-  B() : a() {}   // { dg-error "abstract" }
+  B() : a() {}
 };

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Matthias Klose]

Am 07.12.2012 06:05, schrieb Jason Merrill:
> It's perfectly OK to initialize a base class of abstract type; it's only an
> error to create a full object of such a type.  So this patch moves the check
> from more generic initialization code out into a function that's definitely
> creating a new object.
> 
> Tested x86_64-pc-linux-gnu, applying to trunk and 4.7.

this doesn't build on the branch:

../gcc/cp/tree.c: In function 'build_aggr_init_expr':
../gcc/cp/tree.c:399:1: error: parameter name omitted

this fixes the bootstrap, currently running the testsuite.

--- cp/tree.c~	2012-12-07 10:01:16.665415647 +0100
+++ cp/tree.c	2012-12-07 10:11:01.373410862 +0100
@@ -396,7 +396,8 @@
    callable.  */

 tree
-build_aggr_init_expr (tree type, tree init, tsubst_flags_t /*complain*/)
+build_aggr_init_expr (tree type, tree init,
+		      tsubst_flags_t complain ATTRIBUTE_UNUSED)
 {
   tree fn;
   tree slot;

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Jakub Jelinek]

On Fri, Dec 07, 2012 at 10:13:11AM +0100, Matthias Klose wrote:
> Am 07.12.2012 06:05, schrieb Jason Merrill:
> > It's perfectly OK to initialize a base class of abstract type; it's only an
> > error to create a full object of such a type.  So this patch moves the check
> > from more generic initialization code out into a function that's definitely
> > creating a new object.
> > 
> > Tested x86_64-pc-linux-gnu, applying to trunk and 4.7.
> 
> this doesn't build on the branch:
> 
> ../gcc/cp/tree.c: In function 'build_aggr_init_expr':
> ../gcc/cp/tree.c:399:1: error: parameter name omitted
> 
> this fixes the bootstrap, currently running the testsuite.

Please commit as obvious with appropriate ChangeLog entry.

> --- cp/tree.c~	2012-12-07 10:01:16.665415647 +0100
> +++ cp/tree.c	2012-12-07 10:11:01.373410862 +0100
> @@ -396,7 +396,8 @@
>     callable.  */
> 
>  tree
> -build_aggr_init_expr (tree type, tree init, tsubst_flags_t /*complain*/)
> +build_aggr_init_expr (tree type, tree init,
> +		      tsubst_flags_t complain ATTRIBUTE_UNUSED)
>  {
>    tree fn;
>    tree slot;

	Jakub

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Matthias Klose]

Am 07.12.2012 10:17, schrieb Jakub Jelinek:
> On Fri, Dec 07, 2012 at 10:13:11AM +0100, Matthias Klose wrote:
>> Am 07.12.2012 06:05, schrieb Jason Merrill:
>>> It's perfectly OK to initialize a base class of abstract type; it's only an
>>> error to create a full object of such a type.  So this patch moves the check
>>> from more generic initialization code out into a function that's definitely
>>> creating a new object.
>>>
>>> Tested x86_64-pc-linux-gnu, applying to trunk and 4.7.
>>
>> this doesn't build on the branch:
>>
>> ../gcc/cp/tree.c: In function 'build_aggr_init_expr':
>> ../gcc/cp/tree.c:399:1: error: parameter name omitted
>>
>> this fixes the bootstrap, currently running the testsuite.
> 
> Please commit as obvious with appropriate ChangeLog entry.
> 
>> --- cp/tree.c~	2012-12-07 10:01:16.665415647 +0100
>> +++ cp/tree.c	2012-12-07 10:11:01.373410862 +0100
>> @@ -396,7 +396,8 @@
>>     callable.  */
>>
>>  tree
>> -build_aggr_init_expr (tree type, tree init, tsubst_flags_t /*complain*/)
>> +build_aggr_init_expr (tree type, tree init,
>> +		      tsubst_flags_t complain ATTRIBUTE_UNUSED)
>>  {
>>    tree fn;
>>    tree slot;
> 
> 	Jakub
> 

comitted.

  Matthias

2012-12-07  Matthias Klose  <doko@ubuntu.com>

        * tree.c (build_aggr_init_expr): Add parameter name, mark as unused.

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 360 bytes --]

Hi,

On 12/07/2012 10:24 AM, Matthias Klose wrote:
> committed.
>
>    Matthias
>
> 2012-12-07  Matthias Klose  <doko@ubuntu.com>
>
>          * tree.c (build_aggr_init_expr): Add parameter name, mark as unused.
Thanks.

I was wondering if in mainline we could just do the below. It seems 
straightforward enough...

Thanks!
Paolo.

///////////////////////




[-- Attachment #2: CL --]
[-- Type: text/plain, Size: 222 bytes --]

2012-12-07  Paolo Carlini  <paolo.carlini@oracle.com>

	* tree.c (build_aggr_init_expr): Remove tsubst_flags_t parameter.
	(build_cplus_new): Adjust.
	* cp-tree.h: Adjust declaration.
	* init.c (build_value_init): Adjust.

[-- Attachment #3: patchlet --]
[-- Type: text/plain, Size: 2249 bytes --]

Index: cp-tree.h
===================================================================
--- cp-tree.h	(revision 194296)
+++ cp-tree.h	(working copy)
@@ -5762,7 +5762,7 @@ extern tree build_min_nt_loc			(location_t, enum t
 extern tree build_min_non_dep			(enum tree_code, tree, ...);
 extern tree build_min_non_dep_call_vec		(tree, tree, vec<tree, va_gc> *);
 extern tree build_cplus_new			(tree, tree, tsubst_flags_t);
-extern tree build_aggr_init_expr		(tree, tree, tsubst_flags_t);
+extern tree build_aggr_init_expr		(tree, tree);
 extern tree get_target_expr			(tree);
 extern tree get_target_expr_sfinae		(tree, tsubst_flags_t);
 extern tree build_cplus_array_type		(tree, tree);
Index: init.c
===================================================================
--- init.c	(revision 194296)
+++ init.c	(working copy)
@@ -350,8 +350,7 @@ build_value_init (tree type, tsubst_flags_t compla
 	  (type,
 	   build_special_member_call (NULL_TREE, complete_ctor_identifier,
 				      NULL, type, LOOKUP_NORMAL,
-				      complain),
-	   complain);
+				      complain));
       else if (TYPE_HAS_COMPLEX_DFLT (type))
 	{
 	  /* This is a class that needs constructing, but doesn't have
@@ -361,7 +360,7 @@ build_value_init (tree type, tsubst_flags_t compla
 	  tree ctor = build_special_member_call
 	    (NULL_TREE, complete_ctor_identifier,
 	     NULL, type, LOOKUP_NORMAL, complain);
-	  ctor = build_aggr_init_expr (type, ctor, complain);
+	  ctor = build_aggr_init_expr (type, ctor);
 	  if (ctor != error_mark_node)
 	    AGGR_INIT_ZERO_FIRST (ctor) = 1;
 	  return ctor;
Index: tree.c
===================================================================
--- tree.c	(revision 194296)
+++ tree.c	(working copy)
@@ -407,7 +407,7 @@ build_aggr_init_array (tree return_type, tree fn,
    callable.  */
 
 tree
-build_aggr_init_expr (tree type, tree init, tsubst_flags_t /*complain*/)
+build_aggr_init_expr (tree type, tree init)
 {
   tree fn;
   tree slot;
@@ -469,7 +469,7 @@ tree
 tree
 build_cplus_new (tree type, tree init, tsubst_flags_t complain)
 {
-  tree rval = build_aggr_init_expr (type, init, complain);
+  tree rval = build_aggr_init_expr (type, init);
   tree slot;
 
   /* Make sure that we're not trying to create an instance of an

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Gabriel Dos Reis]

On Fri, Dec 7, 2012 at 3:13 AM, Matthias Klose <doko@ubuntu.com> wrote:
> Am 07.12.2012 06:05, schrieb Jason Merrill:
>> It's perfectly OK to initialize a base class of abstract type; it's only an
>> error to create a full object of such a type.  So this patch moves the check
>> from more generic initialization code out into a function that's definitely
>> creating a new object.
>>
>> Tested x86_64-pc-linux-gnu, applying to trunk and 4.7.
>
> this doesn't build on the branch:
>
> ../gcc/cp/tree.c: In function 'build_aggr_init_expr':
> ../gcc/cp/tree.c:399:1: error: parameter name omitted
>
> this fixes the bootstrap, currently running the testsuite.
>
> --- cp/tree.c~  2012-12-07 10:01:16.665415647 +0100
> +++ cp/tree.c   2012-12-07 10:11:01.373410862 +0100
> @@ -396,7 +396,8 @@
>     callable.  */
>
>  tree
> -build_aggr_init_expr (tree type, tree init, tsubst_flags_t /*complain*/)
> +build_aggr_init_expr (tree type, tree init,
> +                     tsubst_flags_t complain ATTRIBUTE_UNUSED)
>  {
>    tree fn;
>    tree slot;
>

We should definitely teach the compiler to accept the former and not
be silly in requiring the latter when C++.

-- Gaby

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Jakub Jelinek]

On Fri, Dec 07, 2012 at 07:34:30AM -0600, Gabriel Dos Reis wrote:
> On Fri, Dec 7, 2012 at 3:13 AM, Matthias Klose <doko@ubuntu.com> wrote:
> > Am 07.12.2012 06:05, schrieb Jason Merrill:
> >> It's perfectly OK to initialize a base class of abstract type; it's only an
> >> error to create a full object of such a type.  So this patch moves the check
> >> from more generic initialization code out into a function that's definitely
> >> creating a new object.
> >>
> >> Tested x86_64-pc-linux-gnu, applying to trunk and 4.7.
> >
> > this doesn't build on the branch:
> >
> > ../gcc/cp/tree.c: In function 'build_aggr_init_expr':
> > ../gcc/cp/tree.c:399:1: error: parameter name omitted
> >
> > this fixes the bootstrap, currently running the testsuite.
> >
> > --- cp/tree.c~  2012-12-07 10:01:16.665415647 +0100
> > +++ cp/tree.c   2012-12-07 10:11:01.373410862 +0100
> > @@ -396,7 +396,8 @@
> >     callable.  */
> >
> >  tree
> > -build_aggr_init_expr (tree type, tree init, tsubst_flags_t /*complain*/)
> > +build_aggr_init_expr (tree type, tree init,
> > +                     tsubst_flags_t complain ATTRIBUTE_UNUSED)
> >  {
> >    tree fn;
> >    tree slot;
> >
> 
> We should definitely teach the compiler to accept the former and not
> be silly in requiring the latter when C++.

Except that GCC 4.7 doesn't mandate building with C++, so the sources must
be valid C.

	Jakub

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Gabriel Dos Reis]

On Fri, Dec 7, 2012 at 7:39 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Dec 07, 2012 at 07:34:30AM -0600, Gabriel Dos Reis wrote:
>> On Fri, Dec 7, 2012 at 3:13 AM, Matthias Klose <doko@ubuntu.com> wrote:
>> > Am 07.12.2012 06:05, schrieb Jason Merrill:
>> >> It's perfectly OK to initialize a base class of abstract type; it's only an
>> >> error to create a full object of such a type.  So this patch moves the check
>> >> from more generic initialization code out into a function that's definitely
>> >> creating a new object.
>> >>
>> >> Tested x86_64-pc-linux-gnu, applying to trunk and 4.7.
>> >
>> > this doesn't build on the branch:
>> >
>> > ../gcc/cp/tree.c: In function 'build_aggr_init_expr':
>> > ../gcc/cp/tree.c:399:1: error: parameter name omitted
>> >
>> > this fixes the bootstrap, currently running the testsuite.
>> >
>> > --- cp/tree.c~  2012-12-07 10:01:16.665415647 +0100
>> > +++ cp/tree.c   2012-12-07 10:11:01.373410862 +0100
>> > @@ -396,7 +396,8 @@
>> >     callable.  */
>> >
>> >  tree
>> > -build_aggr_init_expr (tree type, tree init, tsubst_flags_t /*complain*/)
>> > +build_aggr_init_expr (tree type, tree init,
>> > +                     tsubst_flags_t complain ATTRIBUTE_UNUSED)
>> >  {
>> >    tree fn;
>> >    tree slot;
>> >
>>
>> We should definitely teach the compiler to accept the former and not
>> be silly in requiring the latter when C++.
>
> Except that GCC 4.7 doesn't mandate building with C++, so the sources must
> be valid C.
>
>         Jakub

Right you are!

Thanks,

-- Gaby

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Jason Merrill]

OK.

Jason

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Paolo Carlini]

Hi,

On 12/07/2012 06:05 AM, Jason Merrill wrote:
> It's perfectly OK to initialize a base class of abstract type; it's 
> only an error to create a full object of such a type.  So this patch 
> moves the check from more generic initialization code out into a 
> function that's definitely creating a new object.
>
> Tested x86_64-pc-linux-gnu, applying to trunk and 4.7.
I was looking a bit more into this Bug, and something seems still weird 
about the testcase in Comment #1 of the audit trail, which we also 
didn't reject with 4.6.x:

class base
{
     protected:
         base()
         {}
};

class derived : public base
{
     public:
         derived()
             : base{} // <-- Note the c++11 curly brace syntax
         {}
};

int main()
{
     derived d1;
     return 0;
}

???

Thanks,
Paolo.

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Jason Merrill]

On 12/21/2012 06:38 AM, Paolo Carlini wrote:
> I was looking a bit more into this Bug, and something seems still weird about the testcase in Comment #1 of the audit trail, which we also didn't reject with 4.6.x:

What's weird about it?

Jason

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Paolo Carlini]

Hi,

Jason Merrill <jason@redhat.com> ha scritto:

>On 12/21/2012 06:38 AM, Paolo Carlini wrote:
>> I was looking a bit more into this Bug, and something seems still
>weird about the testcase in Comment #1 of the audit trail, which we
>also didn't reject with 4.6.x:
>
>What's weird about it?

Well, we still reject it after the patch, whereas we didn't in 4.6.x. Apparently - I didn't check - clang also accepts it. By 'weird' I meant, isn't clear that the Bug is completely resolved...

Paolo

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Jason Merrill]

On 12/22/2012 06:02 PM, Paolo Carlini wrote:
> Well, we still reject it after the patch

My 4.7 and trunk compilers both accept it (with -std=c++11, of course).

Jason

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Paolo Carlini]

On 12/24/2012 05:56 AM, Jason Merrill wrote:
> On 12/22/2012 06:02 PM, Paolo Carlini wrote:
>> Well, we still reject it after the patch
>
> My 4.7 and trunk compilers both accept it (with -std=c++11, of course).
I just updated and rebuilt my 4.7 for you (which definitely I didn't 
hack over the next days) and it still rejects it. Likewise mainline. 
Note that the *first* testcase in the Bug Report, the one involving the 
abtract base, now is correctly accepted by both.

Are you sure your patch handles the access control issue too?? (isn't 
obvious to me that it does, looking at the patch itself and your comments)

Thanks,
Paolo.

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 149 bytes --]

... to explain more concretely what I mean, if I *brutally* hack 
mainline per the below, then the testcase is accepted.

Paolo.

//////////////////

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 615 bytes --]

Index: call.c
===================================================================
--- call.c	(revision 194659)
+++ call.c	(working copy)
@@ -7535,7 +7535,11 @@ build_new_method_call_1 (tree instance, tree fns,
       if (CONSTRUCTOR_NELTS (init_list) == 0
 	  && TYPE_HAS_DEFAULT_CONSTRUCTOR (basetype)
 	  && !processing_template_decl)
-	init = build_value_init (basetype, complain);
+	{
+	  push_deferring_access_checks (dk_no_check);
+	  init = build_value_init (basetype, complain);
+	  pop_deferring_access_checks ();
+	}
 
       /* If BASETYPE is an aggregate, we need to do aggregate
 	 initialization.  */

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Jason Merrill]

On 12/24/2012 03:29 AM, Paolo Carlini wrote:
> Are you sure your patch handles the access control issue too?? (isn't
> obvious to me that it does, looking at the patch itself and your comments)

Nope, you're right.  I put the testcase in one file and then compiled a 
different one.  /facepalm

Jason

Re: C++ PATCH for c++/54325 (wrong error initializing abstract base class)

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 237 bytes --]

Fixed thus.  For a user-provided default constructor we don't need to 
play with zeroing the object first, so we can use the normal logic that 
works properly for protected access.

Tested x86_64-pc-linux-gnu, applying to trunk and 4.7.

[-- Attachment #2: 54325-2.patch --]
[-- Type: text/x-patch, Size: 1409 bytes --]

commit 0ebf6baa1f5f27bd96db44514425075cad2cbd97
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Jan 2 15:31:02 2013 -0500

    	PR c++/54325
    	* call.c (build_new_method_call_1): Don't use build_value_init for
    	user-provided default constructors.

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index bba5d9f..ad39637 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7534,6 +7534,9 @@ build_new_method_call_1 (tree instance, tree fns, vec<tree, va_gc> **args,
 	 build_special_member_call.  */
       if (CONSTRUCTOR_NELTS (init_list) == 0
 	  && TYPE_HAS_DEFAULT_CONSTRUCTOR (basetype)
+	  /* For a user-provided default constructor, use the normal
+	     mechanisms so that protected access works.  */
+	  && !type_has_user_provided_default_constructor (basetype)
 	  && !processing_template_decl)
 	init = build_value_init (basetype, complain);
 
diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-protected.C b/gcc/testsuite/g++.dg/cpp0x/initlist-protected.C
new file mode 100644
index 0000000..fb5cc6a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/initlist-protected.C
@@ -0,0 +1,23 @@
+// PR c++/54325
+// { dg-options -std=c++11 }
+
+class base
+{
+    protected:
+        base()
+        {}
+};
+
+class derived : public base
+{
+    public:
+        derived()
+            : base{} // <-- Note the c++11 curly brace syntax
+        {}
+};
+
+int main()
+{
+    derived d1;
+    return 0;
+}