[PATCH] Make dtor of mapped_index_base virtual

[PATCH] Make dtor of mapped_index_base virtual

[Павел Крюков]

gdb/Changelog:
2017-12-30  Pavel I. Kryukov  <kryukov@frtk.ru>

       * dwarf2read.c (mapped_index_base): Make dtor virtual

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8555b55..fc4c1b3 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2017-12-30  Pavel I. Kryukov  <kryukov@frtk.ru>
+
+       * dwarf2read.c (mapped_index_base): Make dtor virtual
+
 2017-12-28  Simon Marchi  <simon.marchi@polymtl.ca>

        * target.h (enum target_object) <TARGET_OBJECT_HPUX_UREGS,
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 484cbce..3ac4380 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -272,7 +272,7 @@ struct mapped_index_base

   /* Prevent deleting/destroying via a base class pointer.  */
 protected:
-  ~mapped_index_base() = default;
+  virtual ~mapped_index_base() = default;
 };

 /* A description of the mapped index.  The file format is described in
--
2.7.4

Re: [PATCH] Make dtor of mapped_index_base virtual

[Simon Marchi]

On 2017-12-30 13:21, Павел Крюков wrote:
> gdb/Changelog:
> 2017-12-30  Pavel I. Kryukov  <kryukov@frtk.ru>
> 
>        * dwarf2read.c (mapped_index_base): Make dtor virtual
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 8555b55..fc4c1b3 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,7 @@
> +2017-12-30  Pavel I. Kryukov  <kryukov@frtk.ru>
> +
> +       * dwarf2read.c (mapped_index_base): Make dtor virtual
> +
>  2017-12-28  Simon Marchi  <simon.marchi@polymtl.ca>
> 
>         * target.h (enum target_object) <TARGET_OBJECT_HPUX_UREGS,
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 484cbce..3ac4380 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -272,7 +272,7 @@ struct mapped_index_base
> 
>    /* Prevent deleting/destroying via a base class pointer.  */
>  protected:
> -  ~mapped_index_base() = default;
> +  virtual ~mapped_index_base() = default;
>  };
> 
>  /* A description of the mapped index.  The file format is described in
> --
> 2.7.4

Hi Pavel,

Can you clarify what you are trying to fix/improve with this patch?  
Since the goal is that we don't delete through a pointer of this class, 
does the destructor need to be virtual (not that it would hurt 
anything)?  Do you get a build error or something?

Simon

Re: [PATCH] Make dtor of mapped_index_base virtual

[Павел Крюков]

Hi Simon

> Do you get a build error or something?

Yes, I get a build error with Clang:

dwarf2read.c:25409:5: error: destructor called on non-final 'mapped_index'
that
                    has virtual functions but non-virtual destructor
                    [-Werror,-Wdelete-non-virtual-dtor]
                    data->index_table->~mapped_index ();

> Since the goal is that we don't delete through a pointer of this class, does
the destructor need to be virtual (not that it would hurt anything)?

Just to handle the case if someone would delete through a pointer to
"mapped_index_base" class.

Thanks,
--
Pavel

2017-12-31 4:28 GMT+03:00 Simon Marchi <simon.marchi@polymtl.ca>:

> On 2017-12-30 13:21, Павел Крюков wrote:
>
>> gdb/Changelog:
>> 2017-12-30  Pavel I. Kryukov  <kryukov@frtk.ru>
>>
>>        * dwarf2read.c (mapped_index_base): Make dtor virtual
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 8555b55..fc4c1b3 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,7 @@
>> +2017-12-30  Pavel I. Kryukov  <kryukov@frtk.ru>
>> +
>> +       * dwarf2read.c (mapped_index_base): Make dtor virtual
>> +
>>  2017-12-28  Simon Marchi  <simon.marchi@polymtl.ca>
>>
>>         * target.h (enum target_object) <TARGET_OBJECT_HPUX_UREGS,
>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>> index 484cbce..3ac4380 100644
>> --- a/gdb/dwarf2read.c
>> +++ b/gdb/dwarf2read.c
>> @@ -272,7 +272,7 @@ struct mapped_index_base
>>
>>    /* Prevent deleting/destroying via a base class pointer.  */
>>  protected:
>> -  ~mapped_index_base() = default;
>> +  virtual ~mapped_index_base() = default;
>>  };
>>
>>  /* A description of the mapped index.  The file format is described in
>> --
>> 2.7.4
>>
>
> Hi Pavel,
>
> Can you clarify what you are trying to fix/improve with this patch?  Since
> the goal is that we don't delete through a pointer of this class, does the
> destructor need to be virtual (not that it would hurt anything)?  Do you
> get a build error or something?
>
> Simon
>

Re: [PATCH] Make dtor of mapped_index_base virtual

[Simon Marchi]

On 2017-12-31 05:12, Павел Крюков wrote:
> Hi Simon
> 
>> Do you get a build error or something?
> 
> Yes, I get a build error with Clang:
> 
> dwarf2read.c:25409:5: error: destructor called on non-final 
> 'mapped_index'
> that
>                     has virtual functions but non-virtual destructor
>                     [-Werror,-Wdelete-non-virtual-dtor]
>                     data->index_table->~mapped_index ();
> 
>> Since the goal is that we don't delete through a pointer of this 
>> class, does
> the destructor need to be virtual (not that it would hurt anything)?
> 
> Just to handle the case if someone would delete through a pointer to
> "mapped_index_base" class.
> 
> Thanks,

Hi Pavel,

I think this warning was fixed by the following commit:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=fc898b42e355fef58e6a029799fdd71b9dda5dc6

We don't want anything deleting through a pointer of the base class, 
because (for now) instances of the two child classes (mapped_index and 
mapped_debug_names) are allocated differently.  One is allocated on the 
objfile obstack (some kind of manual memory block allocation) and the 
other is allocated with new.  Trying to delete or manually call the 
destructor through a pointer to mapped_index_base would do the wrong 
thing for one of the two children.  So it's on purpose that 
mapped_index_base's destructor is hidden, and it doesn't matter whether 
or not it is virtual.

Thanks,

Simon