diff mbox series

PR C++/88114 - patch for destructor not generated for "virtual ~destructor() = default"

Message ID 20181121121935.tm4y2wyie6bc72sn@physik.fu-berlin.de
State New
Headers show
Series PR C++/88114 - patch for destructor not generated for "virtual ~destructor() = default" | expand

Commit Message

Tobias Burnus Nov. 21, 2018, 12:19 p.m. UTC 
Hello all,

if a class contains any 'virtual ... = 0', it's an abstract class and for an
abstract class, the destructor not added to the vtable.

For a normal
  virtual ~class() { }
that's not a problem as the class::~class() destructor will be generated during
the parsing of the function.

But for
  virtual ~class() = default;
the destructor will be generated via mark_used via the vtable.


If one now declares a derived class and uses it, the class::~class() is generated
in that translation unit.  Unless, #pragma interface/implementation is used.

In that case, the 'default' destructor will never be generated.


The following code seems to work both for the big code and for the example;
without '#pragma implementation', the destructor is not generated for the example,
only with.

The patch survived boostrapping GCC with default languages on x86-64-gnu-linux
and "make check-g++".*

[One probably could get rid of some of the conditions for generating the code,
e.g. TREE_USED and DECL_DEFAULTED_FN are probably not both needed; one might
want to set some additional DECL to the fn decl.]

Does the patch and the test case make sense? Or is something else/in addition
needed?

Tobias


*I do get the following failures on this CentOS6 system:

FAIL: g++.dg/pr83239.C  -std=gnu++98 (test for excess errors)
Excess errors:
cc1plus: warning: 'void* __builtin_memset(void*, int, long unsigned int)' specified size 18446744073709551608 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]
cc1plus: warning: 'void* __builtin_memset(void*, int, long unsigned int)' specified size 18446744073709551600 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]

FAIL: g++.dg/tls/thread_local-order2.C  -std=c++14 execution test
FAIL: g++.dg/tls/thread_local-order2.C  -std=c++17 execution test

plus each 32 times:
FAIL: guality/guality.h: 0 PASS, 1 FAIL, 0 UNRESOLVED
FAIL: guality/guality.h: varl is -1, not 6

Comments

Tobias Burnus Nov. 25, 2018, 8:44 a.m. UTC | #1 
On 21 November 2018, Tobias Burnus wrote:
> Hello all,
>
> if a class contains any 'virtual ... = 0', it's an abstract class and for an
> abstract class, the destructor not added to the vtable.
>
> For a normal
>    virtual ~class() { }
> that's not a problem as the class::~class() destructor will be generated during
> the parsing of the function.
>
> But for
>    virtual ~class() = default;
> the destructor will be generated via mark_used via the vtable.
>
>
> If one now declares a derived class and uses it, the class::~class() is generated
> in that translation unit.  Unless, #pragma interface/implementation is used.
>
> In that case, the 'default' destructor will never be generated.
>
>
> The following code seems to work both for the big code and for the example;
> without '#pragma implementation', the destructor is not generated for the example,
> only with.
>
> The patch survived boostrapping GCC with default languages on x86-64-gnu-linux
> and "make check-g++".*
>
> [One probably could get rid of some of the conditions for generating the code,
> e.g. TREE_USED and DECL_DEFAULTED_FN are probably not both needed; one might
> want to set some additional DECL to the fn decl.]
>
> Does the patch and the test case make sense? Or is something else/in addition
> needed?
>
> Tobias
>
>
> *I do get the following failures on this CentOS6 system:
>
> FAIL: g++.dg/pr83239.C  -std=gnu++98 (test for excess errors)
> Excess errors:
> cc1plus: warning: 'void* __builtin_memset(void*, int, long unsigned int)' specified size 18446744073709551608 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]
> cc1plus: warning: 'void* __builtin_memset(void*, int, long unsigned int)' specified size 18446744073709551600 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]
>
> FAIL: g++.dg/tls/thread_local-order2.C  -std=c++14 execution test
> FAIL: g++.dg/tls/thread_local-order2.C  -std=c++17 execution test
>
> plus each 32 times:
> FAIL: guality/guality.h: 0 PASS, 1 FAIL, 0 UNRESOLVED
> FAIL: guality/guality.h: varl is -1, not 6
Tobias Burnus Nov. 28, 2018, 8:35 p.m. UTC | #2 
On the 25th November 2018, schrieb Tobias Burnus wrote:

> On 21 November 2018, Tobias Burnus wrote:
>> Hello all,
>>
>> if a class contains any 'virtual ... = 0', it's an abstract class and 
>> for an
>> abstract class, the destructor not added to the vtable.
>>
>> For a normal
>>    virtual ~class() { }
>> that's not a problem as the class::~class() destructor will be 
>> generated during
>> the parsing of the function.
>>
>> But for
>>    virtual ~class() = default;
>> the destructor will be generated via mark_used via the vtable.
>>
>>
>> If one now declares a derived class and uses it, the class::~class() 
>> is generated
>> in that translation unit.  Unless, #pragma interface/implementation 
>> is used.
>>
>> In that case, the 'default' destructor will never be generated.
>>
>>
>> The following code seems to work both for the big code and for the 
>> example;
>> without '#pragma implementation', the destructor is not generated for 
>> the example,
>> only with.
>>
>> The patch survived boostrapping GCC with default languages on 
>> x86-64-gnu-linux
>> and "make check-g++".*
>>
>> [One probably could get rid of some of the conditions for generating 
>> the code,
>> e.g. TREE_USED and DECL_DEFAULTED_FN are probably not both needed; 
>> one might
>> want to set some additional DECL to the fn decl.]
>>
>> Does the patch and the test case make sense? Or is something else/in 
>> addition
>> needed?
>>
>> Tobias
>>
>>
>> *I do get the following failures on this CentOS6 system:
>>
>> FAIL: g++.dg/pr83239.C  -std=gnu++98 (test for excess errors)
>> Excess errors:
>> cc1plus: warning: 'void* __builtin_memset(void*, int, long unsigned 
>> int)' specified size 18446744073709551608 exceeds maximum object size 
>> 9223372036854775807 [-Wstringop-overflow=]
>> cc1plus: warning: 'void* __builtin_memset(void*, int, long unsigned 
>> int)' specified size 18446744073709551600 exceeds maximum object size 
>> 9223372036854775807 [-Wstringop-overflow=]
>>
>> FAIL: g++.dg/tls/thread_local-order2.C  -std=c++14 execution test
>> FAIL: g++.dg/tls/thread_local-order2.C  -std=c++17 execution test
>>
>> plus each 32 times:
>> FAIL: guality/guality.h: 0 PASS, 1 FAIL, 0 UNRESOLVED
>> FAIL: guality/guality.h: varl is -1, not 6
Tobias Burnus Dec. 4, 2018, 12:42 p.m. UTC | #3 
On Wed, Nov 28, 2018 at 09:35:53PM +0100, Tobias Burnus wrote:
> On the 25th November 2018, schrieb Tobias Burnus wrote:
> > On 21 November 2018, Tobias Burnus wrote:
> > > Hello all,
> > > 
> > > if a class contains any 'virtual ... = 0', it's an abstract class
> > > and for an
> > > abstract class, the destructor not added to the vtable.
> > > 
> > > For a normal
> > >    virtual ~class() { }
> > > that's not a problem as the class::~class() destructor will be
> > > generated during
> > > the parsing of the function.
> > > 
> > > But for
> > >    virtual ~class() = default;
> > > the destructor will be generated via mark_used via the vtable.
> > > 
> > > 
> > > If one now declares a derived class and uses it, the class::~class()
> > > is generated
> > > in that translation unit.  Unless, #pragma interface/implementation
> > > is used.
> > > 
> > > In that case, the 'default' destructor will never be generated.
> > > 
> > > 
> > > The following code seems to work both for the big code and for the
> > > example;
> > > without '#pragma implementation', the destructor is not generated
> > > for the example,
> > > only with.
> > > 
> > > The patch survived boostrapping GCC with default languages on
> > > x86-64-gnu-linux
> > > and "make check-g++".*
> > > 
> > > [One probably could get rid of some of the conditions for generating
> > > the code,
> > > e.g. TREE_USED and DECL_DEFAULTED_FN are probably not both needed;
> > > one might
> > > want to set some additional DECL to the fn decl.]
> > > 
> > > Does the patch and the test case make sense? Or is something else/in
> > > addition
> > > needed?
> > > 
> > > Tobias
> > > 
> > > 
> > > *I do get the following failures on this CentOS6 system:
> > > 
> > > FAIL: g++.dg/pr83239.C  -std=gnu++98 (test for excess errors)
> > > Excess errors:
> > > cc1plus: warning: 'void* __builtin_memset(void*, int, long unsigned
> > > int)' specified size 18446744073709551608 exceeds maximum object
> > > size 9223372036854775807 [-Wstringop-overflow=]
> > > cc1plus: warning: 'void* __builtin_memset(void*, int, long unsigned
> > > int)' specified size 18446744073709551600 exceeds maximum object
> > > size 9223372036854775807 [-Wstringop-overflow=]
> > > 
> > > FAIL: g++.dg/tls/thread_local-order2.C  -std=c++14 execution test
> > > FAIL: g++.dg/tls/thread_local-order2.C  -std=c++17 execution test
> > > 
> > > plus each 32 times:
> > > FAIL: guality/guality.h: 0 PASS, 1 FAIL, 0 UNRESOLVED
> > > FAIL: guality/guality.h: varl is -1, not 6
Jason Merrill Dec. 5, 2018, 10:50 p.m. UTC | #4 
On 11/21/18 7:19 AM, Tobias Burnus wrote:
> Hello all,
> 
> if a class contains any 'virtual ... = 0', it's an abstract class and for an
> abstract class, the destructor not added to the vtable.
> 
> For a normal
>    virtual ~class() { }
> that's not a problem as the class::~class() destructor will be generated during
> the parsing of the function.
> 
> But for
>    virtual ~class() = default;
> the destructor will be generated via mark_used via the vtable.
> 
> 
> If one now declares a derived class and uses it, the class::~class() is generated
> in that translation unit.  Unless, #pragma interface/implementation is used.
> 
> In that case, the 'default' destructor will never be generated.
> 
> 
> The following code seems to work both for the big code and for the example;
> without '#pragma implementation', the destructor is not generated for the example,
> only with.
> 
> The patch survived boostrapping GCC with default languages on x86-64-gnu-linux
> and "make check-g++".*
> 
> [One probably could get rid of some of the conditions for generating the code,
> e.g. TREE_USED and DECL_DEFAULTED_FN are probably not both needed; one might
> want to set some additional DECL to the fn decl.]

You can get at the destructor with CLASSTYPE_DESTRUCTOR rather than 
walking through TYPE_FIELDS.  I'd also check DECL_DEFAULTED_IN_CLASS_P.

I'd also do this in maybe_emit_vtables rather than here, so that it only 
happens once per class.

Jason
Tobias Burnus Jan. 11, 2019, 6:36 p.m. UTC | #5 
Dear Jason, dear all,

Jason Merrill wrote on 5 Dec 2018:
> You can get at the destructor with CLASSTYPE_DESTRUCTOR rather than walking through TYPE_FIELDS. I'd also check DECL_DEFAULTED_IN_CLASS_P.
> I'd also do this in maybe_emit_vtables rather than here, so that it only happens once per class. 

Updated patch. I also added a test case which checks that the destructor
is not generated.

[ Original patch: https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01824.html ]

Background again:
   If a class contains any 'virtual ... = 0', it's an abstract class and for an
   abstract class, the destructor not added to the vtable.

   For a normal
     virtual ~class() { }
   that's not a problem as the class::~class() destructor will be generated during
   the parsing of the function.

   But for
     virtual ~class() = default;
   the destructor will be generated via mark_used via the vtable.

   If one now declares a derived class and uses it, the class::~class() is generated
   in that translation unit.  Unless, #pragma interface/implementation is used.

   In that case, the 'default' destructor will never be generated.

Build and regtested on x86_64-gnu-linux.
OK? Or do you have more suggested changes?

Tobias
2019-01-11  Tobias Burnus  <burnus@net-b.de>

	PR C++/88114
	* decl2.c (maybe_emit_vtables): If needed, generate code for
	the destructor of an abstract class.
	(mark_used): Update comment for older function-name change.

	PR C++/88114
	* g++.dg/cpp0x/defaulted61.C: New.
	* g++.dg/cpp0x/defaulted62.C: New.

diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
index dbab95fbc96..2e7ecdaa01c 100644
--- a/gcc/cp/decl2.c
+++ b/gcc/cp/decl2.c
@@ -2220,6 +2220,17 @@ maybe_emit_vtables (tree ctype)
 	}
     }
 
+  /* For abstract classes, the destructor has been removed from the
+     vtable (in class.c's build_vtbl_initializer).  For a compiler-
+     generated destructor, it hence might not have been generated in
+     this translation unit - and with '#pragma interface' it might
+     never get generated.  */
+  if (CLASSTYPE_PURE_VIRTUALS (ctype)
+      && TYPE_HAS_NONTRIVIAL_DESTRUCTOR (ctype)
+      && DECL_DEFAULTED_IN_CLASS_P(CLASSTYPE_DESTRUCTOR(ctype))
+      && DECL_DEFAULTED_FN (CLASSTYPE_DESTRUCTOR(ctype)))
+    note_vague_linkage_fn (CLASSTYPE_DESTRUCTOR(ctype));
+
   /* Since we're writing out the vtable here, also write the debug
      info.  */
   note_debug_info_needed (ctype);
@@ -5497,7 +5508,7 @@ mark_used (tree decl, tsubst_flags_t complain)
 	 within the body of a function so as to avoid collecting live data
 	 on the stack (such as overload resolution candidates).
 
-         We could just let cp_write_global_declarations handle synthesizing
+         We could just let c_parse_final_cleanups handle synthesizing
          this function by adding it to deferred_fns, but doing
          it at the use site produces better error messages.  */
       ++function_depth;
diff --git a/gcc/testsuite/g++.dg/cpp0x/defaulted61.C b/gcc/testsuite/g++.dg/cpp0x/defaulted61.C
new file mode 100644
index 00000000000..e7e0a486292
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/defaulted61.C
@@ -0,0 +1,22 @@
+// { dg-do compile { target c++11 } }
+// { dg-final { scan-assembler "_ZN3OneD0Ev" } }
+
+// PR C++/88114
+// Destructor of an abstract class was never generated
+// when compiling the class - nor later due to the
+// '#pragma interface'
+
+#pragma implementation
+#pragma interface
+
+class One
+{
+ public:
+  virtual ~One() = default;
+  void some_fn();
+  virtual void later() = 0;
+ private:
+  int m_int;
+};
+
+void One::some_fn() { }
diff --git a/gcc/testsuite/g++.dg/cpp0x/defaulted62.C b/gcc/testsuite/g++.dg/cpp0x/defaulted62.C
new file mode 100644
index 00000000000..d8dab608816
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/defaulted62.C
@@ -0,0 +1,25 @@
+// { dg-do compile { target c++11 } }
+// { dg-final { scan-assembler-not "_ZN3OneD0Ev" } }
+
+// PR C++/88114
+// Destructor of an abstract class was never generated
+// when compiling the class - nor later due to the
+// '#pragma interface'
+// -> g++.dg/cpp0x/defaulted61.C
+
+// HERE, in g++.dg/cpp0x/defaulted62.C:
+// As we have commented the pragmas, it should NOT be created
+// #pragma implementation
+// #pragma interface
+
+class One
+{
+ public:
+  virtual ~One() = default;
+  void some_fn();
+  virtual void later() = 0;
+ private:
+  int m_int;
+};
+
+void One::some_fn() { }
Jason Merrill Jan. 11, 2019, 6:57 p.m. UTC | #6 
On 1/11/19 1:36 PM, Tobias Burnus wrote:
> Dear Jason, dear all,
> 
> Jason Merrill wrote on 5 Dec 2018:
>> You can get at the destructor with CLASSTYPE_DESTRUCTOR rather than walking through TYPE_FIELDS. I'd also check DECL_DEFAULTED_IN_CLASS_P.
>> I'd also do this in maybe_emit_vtables rather than here, so that it only happens once per class.
> 
> Updated patch. I also added a test case which checks that the destructor
> is not generated.
> 
> [ Original patch: https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01824.html ]
> 
> Background again:
>     If a class contains any 'virtual ... = 0', it's an abstract class and for an
>     abstract class, the destructor not added to the vtable.
> 
>     For a normal
>       virtual ~class() { }
>     that's not a problem as the class::~class() destructor will be generated during
>     the parsing of the function.
> 
>     But for
>       virtual ~class() = default;
>     the destructor will be generated via mark_used via the vtable.
> 
>     If one now declares a derived class and uses it, the class::~class() is generated
>     in that translation unit.  Unless, #pragma interface/implementation is used.
> 
>     In that case, the 'default' destructor will never be generated.
> 
> Build and regtested on x86_64-gnu-linux.
> OK? Or do you have more suggested changes?

> +      && DECL_DEFAULTED_IN_CLASS_P(CLASSTYPE_DESTRUCTOR(ctype))
> +      && DECL_DEFAULTED_FN (CLASSTYPE_DESTRUCTOR(ctype)))

Checking DECL_DEFAULTED_FN again is redundant, it's checked by 
DECL_DEFAULTED_IN_CLASS_P.  OK with that last condition removed.

Jason
diff mbox series

Patch

	PR C++/88114
	* decl2.c (c_parse_final_cleanups): If needed, generate code for 
	the destructor of an abstract class.
	(mark_used): Update comment for older function-name change.

	PR C++/88114
	* g++.dg/cpp0x/defaulted61.C: New.

diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
index ffc0d0d6ec4..056e49ad88a 100644
--- a/gcc/cp/decl2.c
+++ b/gcc/cp/decl2.c
@@ -4782,6 +4782,18 @@  c_parse_final_cleanups (void)
 	  {
 	    reconsider = true;
 	    keyed_classes->unordered_remove (i);
+
+	    /* For abstract classes, the destructor has been removed from the
+	       vtable (in class.c's build_vtbl_initializer).  For a compiler-
+	       generated destructor, it hence might not have been generated in
+	       this translation unit - and with '#pragma interface' it might
+	       never get generated.  */
+	    if (CLASSTYPE_PURE_VIRTUALS (t)
+		&& TYPE_HAS_NONTRIVIAL_DESTRUCTOR (t))
+	      for (tree x = TYPE_FIELDS (t); x; x = DECL_CHAIN (x))
+		if (DECL_DECLARES_FUNCTION_P (x) && DECL_DESTRUCTOR_P (x)
+		    && !TREE_USED (x) && DECL_DEFAULTED_FN (x))
+		  note_vague_linkage_fn (x);
 	  }
       /* The input_location may have been changed during marking of
 	 vtable entries.  */
@@ -5465,7 +5477,7 @@  mark_used (tree decl, tsubst_flags_t complain)
 	 within the body of a function so as to avoid collecting live data
 	 on the stack (such as overload resolution candidates).
 
-         We could just let cp_write_global_declarations handle synthesizing
+         We could just let c_parse_final_cleanups handle synthesizing
          this function by adding it to deferred_fns, but doing
          it at the use site produces better error messages.  */
       ++function_depth;
diff --git a/gcc/testsuite/g++.dg/cpp0x/defaulted61.C b/gcc/testsuite/g++.dg/cpp0x/defaulted61.C
new file mode 100644
index 00000000000..e7e0a486292
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/defaulted61.C
@@ -0,0 +1,22 @@ 
+// { dg-do compile { target c++11 } }
+// { dg-final { scan-assembler "_ZN3OneD0Ev" } }
+
+// PR C++/88114
+// Destructor of an abstract class was never generated
+// when compiling the class - nor later due to the
+// '#pragma interface'
+
+#pragma implementation
+#pragma interface
+
+class One
+{
+ public:
+  virtual ~One() = default;
+  void some_fn();
+  virtual void later() = 0;
+ private:
+  int m_int;
+};
+
+void One::some_fn() { }