[RFA 1/2] Change minimal_symbol_reader::record_full to take a bool

[RFA 1/2] Change minimal_symbol_reader::record_full to take a bool

[Tom Tromey]

This changes an "int" to a "bool" in the signature for
minimal_symbol_reader::record_full, and then fixes the callers.

2016-10-20  Tom Tromey  <tom@tromey.com>

	* minsyms.h (minimal_symbol_reader::record_full): "copy_name" now
	a bool.
	(record, record_with_info): Update.
	* minsyms.c (record): Fix indentation.
	(record_full): Fix indentation.  Update for type change.
	* elfread.c (record_minimal_symbol): "copy_name" now a bool.
	(elf_symtab_read): "copy_names" now a bool.
	(elf_rel_plt_read, elf_read_minimal_symbols): Update.
---
 gdb/ChangeLog | 11 +++++++++++
 gdb/elfread.c | 19 ++++++++++---------
 gdb/minsyms.c |  9 ++++-----
 gdb/minsyms.h | 14 +++++++-------
 4 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f3dd5d7..35df7f0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,16 @@
 2016-10-20  Tom Tromey  <tom@tromey.com>
 
+	* minsyms.h (minimal_symbol_reader::record_full): "copy_name" now
+	a bool.
+	(record, record_with_info): Update.
+	* minsyms.c (record): Fix indentation.
+	(record_full): Fix indentation.  Update for type change.
+	* elfread.c (record_minimal_symbol): "copy_name" now a bool.
+	(elf_symtab_read): "copy_names" now a bool.
+	(elf_rel_plt_read, elf_read_minimal_symbols): Update.
+
+2016-10-20  Tom Tromey  <tom@tromey.com>
+
 	* main.c: Include <vector>.
 	(cmdarg_s): Remove typedef.  Don't define VEC.
 	(captured_main_1): Use vector, not VEC.  Remove cleanups.
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 485e55d..f5aa55e 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -192,7 +192,7 @@ elf_locate_sections (bfd *ignore_abfd, asection *sectp, void *eip)
 
 static struct minimal_symbol *
 record_minimal_symbol (minimal_symbol_reader &reader,
-		       const char *name, int name_len, int copy_name,
+		       const char *name, int name_len, bool copy_name,
 		       CORE_ADDR address,
 		       enum minimal_symbol_type ms_type,
 		       asection *bfd_section, struct objfile *objfile)
@@ -229,7 +229,7 @@ static void
 elf_symtab_read (minimal_symbol_reader &reader,
 		 struct objfile *objfile, int type,
 		 long number_of_symbols, asymbol **symbol_table,
-		 int copy_names)
+		 bool copy_names)
 {
   struct gdbarch *gdbarch = get_objfile_arch (objfile);
   asymbol *sym;
@@ -488,7 +488,7 @@ elf_symtab_read (minimal_symbol_reader &reader,
 		{
 		  int len = atsign - sym->name;
 
-		  record_minimal_symbol (reader, sym->name, len, 1, symaddr,
+		  record_minimal_symbol (reader, sym->name, len, true, symaddr,
 					 ms_type, sym->section, objfile);
 		}
 	    }
@@ -505,8 +505,8 @@ elf_symtab_read (minimal_symbol_reader &reader,
 		{
 		  struct minimal_symbol *mtramp;
 
-		  mtramp = record_minimal_symbol (reader, sym->name, len - 4, 1,
-						  symaddr,
+		  mtramp = record_minimal_symbol (reader, sym->name, len - 4,
+						  true, symaddr,
 						  mst_solib_trampoline,
 						  sym->section, objfile);
 		  if (mtramp)
@@ -612,7 +612,7 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
 
       msym = record_minimal_symbol (reader, string_buffer,
 				    name_len + got_suffix_len,
-                                    1, address, mst_slot_got_plt, got_plt,
+                                    true, address, mst_slot_got_plt, got_plt,
 				    objfile);
       if (msym)
 	SET_MSYMBOL_SIZE (msym, ptr_size);
@@ -1078,7 +1078,8 @@ elf_read_minimal_symbols (struct objfile *objfile, int symfile_flags,
 	       bfd_get_filename (objfile->obfd),
 	       bfd_errmsg (bfd_get_error ()));
 
-      elf_symtab_read (reader, objfile, ST_REGULAR, symcount, symbol_table, 0);
+      elf_symtab_read (reader, objfile, ST_REGULAR, symcount, symbol_table,
+		       false);
     }
 
   /* Add the dynamic symbols.  */
@@ -1104,7 +1105,7 @@ elf_read_minimal_symbols (struct objfile *objfile, int symfile_flags,
 	       bfd_errmsg (bfd_get_error ()));
 
       elf_symtab_read (reader, objfile, ST_DYNAMIC, dynsymcount,
-		       dyn_symbol_table, 0);
+		       dyn_symbol_table, false);
 
       elf_rel_plt_read (reader, objfile, dyn_symbol_table);
     }
@@ -1140,7 +1141,7 @@ elf_read_minimal_symbols (struct objfile *objfile, int symfile_flags,
       for (i = 0; i < synthcount; i++)
 	synth_symbol_table[i] = synthsyms + i;
       elf_symtab_read (reader, objfile, ST_SYNTHETIC, synthcount,
-		       synth_symbol_table.get (), 1);
+		       synth_symbol_table.get (), true);
     }
 
   /* Install any minimal symbols that have been collected as the current
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 5f6db60..d804c95 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -943,7 +943,7 @@ minimal_symbol_reader::~minimal_symbol_reader ()
 
 void
 minimal_symbol_reader::record (const char *name, CORE_ADDR address,
-				      enum minimal_symbol_type ms_type)
+			       enum minimal_symbol_type ms_type)
 {
   int section;
 
@@ -974,10 +974,9 @@ minimal_symbol_reader::record (const char *name, CORE_ADDR address,
 
 struct minimal_symbol *
 minimal_symbol_reader::record_full (const char *name, int name_len,
-					   int copy_name,
-					   CORE_ADDR address,
-					   enum minimal_symbol_type ms_type,
-					   int section)
+				    bool copy_name, CORE_ADDR address,
+				    enum minimal_symbol_type ms_type,
+				    int section)
 {
   struct msym_bunch *newobj;
   struct minimal_symbol *msymbol;
diff --git a/gdb/minsyms.h b/gdb/minsyms.h
index b22920b..06b3b4e 100644
--- a/gdb/minsyms.h
+++ b/gdb/minsyms.h
@@ -93,35 +93,35 @@ class minimal_symbol_reader
      ADDRESS - the address of the symbol
      MS_TYPE - the type of the symbol
      SECTION - the symbol's section
-     appropriate obj_section for the minimal symbol.  This can be NULL.
-     OBJFILE - the objfile associated with the minimal symbol.  */
+  */
 
   struct minimal_symbol *record_full (const char *name,
 				      int name_len,
-				      int copy_name,
+				      bool copy_name,
 				      CORE_ADDR address,
 				      enum minimal_symbol_type ms_type,
 				      int section);
 
   /* Like record_full, but:
      - uses strlen to compute NAME_LEN,
-     - passes COPY_NAME = 1,
+     - passes COPY_NAME = true,
      - and passes a default SECTION, depending on the type
 
      This variant does not return the new symbol.  */
 
-  void record (const char *, CORE_ADDR, enum minimal_symbol_type);
+  void record (const char *name, CORE_ADDR address,
+	       enum minimal_symbol_type ms_type);
 
   /* Like record_full, but:
      - uses strlen to compute NAME_LEN,
-     - passes COPY_NAME = 1.  */
+     - passes COPY_NAME = true.  */
 
   struct minimal_symbol *record_with_info (const char *name,
 					   CORE_ADDR address,
 					   enum minimal_symbol_type ms_type,
 					   int section)
   {
-    return record_full (name, strlen (name), 1, address, ms_type, section);
+    return record_full (name, strlen (name), true, address, ms_type, section);
   }
 
  private:
-- 
2.7.4

[RFA 2/2] Make some dwarf_expr_context methods pure virtual

[Tom Tromey]

This patch changes some dwarf_expr_context to be pure virtual, as
mentioned during the discussion of an earlier patch in this series.

2016-10-20  Tom Tromey  <tom@tromey.com>

	* dwarf2expr.h (class dwarf_expr_context)
	<get_frame_base, get_frame_cfa, get_tls_address, dwarf_call,
	push_dwarf_block_entry_value, get_addr_index, get_object_address>:
	Now pure virtual.
	* dwarf2-frame.c (class dwarf_expr_executor)
	<get_frame_base, get_frame_cfa, get_tls_address, dwarf_call,
	push_dwarf_block_entry_value, get_addr_index, get_object_address>:
	New methods.
	<invalid>: New method.
---
 gdb/ChangeLog      | 12 ++++++++++++
 gdb/dwarf2-frame.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/dwarf2expr.h   | 38 +++++++-------------------------------
 3 files changed, 63 insertions(+), 31 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 35df7f0..ad7da6b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,17 @@
 2016-10-20  Tom Tromey  <tom@tromey.com>
 
+	* dwarf2expr.h (class dwarf_expr_context)
+	<get_frame_base, get_frame_cfa, get_tls_address, dwarf_call,
+	push_dwarf_block_entry_value, get_addr_index, get_object_address>:
+	Now pure virtual.
+	* dwarf2-frame.c (class dwarf_expr_executor)
+	<get_frame_base, get_frame_cfa, get_tls_address, dwarf_call,
+	push_dwarf_block_entry_value, get_addr_index, get_object_address>:
+	New methods.
+	<invalid>: New method.
+
+2016-10-20  Tom Tromey  <tom@tromey.com>
+
 	* minsyms.h (minimal_symbol_reader::record_full): "copy_name" now
 	a bool.
 	(record, record_with_info): Update.
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index c9962e1..beab304 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -351,6 +351,50 @@ class dwarf_expr_executor : public dwarf_expr_context
   {
     read_memory (addr, buf, len);
   }
+
+  void get_frame_base (const gdb_byte **start, size_t *length) OVERRIDE
+  {
+    invalid ("DW_OP_fbreg");
+  }
+
+  void push_dwarf_reg_entry_value (enum call_site_parameter_kind kind,
+				   union call_site_parameter_u kind_u,
+				   int deref_size) OVERRIDE
+  {
+    invalid ("DW_OP_GNU_entry_value");
+  }
+
+  CORE_ADDR get_object_address () OVERRIDE
+  {
+    invalid ("DW_OP_push_object_address");
+  }
+
+  CORE_ADDR get_frame_cfa () OVERRIDE
+  {
+    invalid ("DW_OP_call_frame_cfa");
+  }
+
+  CORE_ADDR get_tls_address (CORE_ADDR offset) OVERRIDE
+  {
+    invalid ("DW_OP_form_tls_address");
+  }
+
+  void dwarf_call (cu_offset die_offset) OVERRIDE
+  {
+    invalid ("DW_OP_call*");
+  }
+
+  CORE_ADDR get_addr_index (unsigned int index)
+  {
+    invalid ("DW_OP_GNU_addr_index");
+  }
+
+ private:
+
+  void invalid (const char *op) ATTRIBUTE_NORETURN
+  {
+    error (_("%s is invalid in this context"), op);
+  }
 };
 
 static CORE_ADDR
diff --git a/gdb/dwarf2expr.h b/gdb/dwarf2expr.h
index 63bdc6e..7bf194a 100644
--- a/gdb/dwarf2expr.h
+++ b/gdb/dwarf2expr.h
@@ -156,16 +156,10 @@ struct dwarf_expr_context
   /* Return the location expression for the frame base attribute, in
      START and LENGTH.  The result must be live until the current
      expression evaluation is complete.  */
-  virtual void get_frame_base (const gdb_byte **start, size_t *length)
-  {
-    error (_("%s is invalid in this context"), "DW_OP_fbreg");
-  }
+  virtual void get_frame_base (const gdb_byte **start, size_t *length) = 0;
 
   /* Return the CFA for the frame.  */
-  virtual CORE_ADDR get_frame_cfa ()
-  {
-    error (_("%s is invalid in this context"), "DW_OP_call_frame_cfa");
-  }
+  virtual CORE_ADDR get_frame_cfa () = 0;
 
   /* Return the PC for the frame.  */
   virtual CORE_ADDR get_frame_pc ()
@@ -175,19 +169,13 @@ struct dwarf_expr_context
 
   /* Return the thread-local storage address for
      DW_OP_GNU_push_tls_address or DW_OP_form_tls_address.  */
-  virtual CORE_ADDR get_tls_address (CORE_ADDR offset)
-  {
-    error (_("%s is invalid in this context"), "DW_OP_form_tls_address");
-  }
+  virtual CORE_ADDR get_tls_address (CORE_ADDR offset) = 0;
 
   /* Execute DW_AT_location expression for the DWARF expression
      subroutine in the DIE at DIE_OFFSET in the CU.  Do not touch
      STACK while it being passed to and returned from the called DWARF
      subroutine.  */
-  virtual void dwarf_call (cu_offset die_offset)
-  {
-    error (_("%s is invalid in this context"), "DW_OP_call*");
-  }
+  virtual void dwarf_call (cu_offset die_offset) = 0;
 
   /* Return the base type given by the indicated DIE.  This can throw
      an exception if the DIE is invalid or does not represent a base
@@ -206,26 +194,14 @@ struct dwarf_expr_context
      DW_AT_GNU_call_site_value.  */
   virtual void push_dwarf_reg_entry_value (enum call_site_parameter_kind kind,
 					   union call_site_parameter_u kind_u,
-					   int deref_size)
-  {
-    internal_error (__FILE__, __LINE__,
-		    _("Support for DW_OP_GNU_entry_value is unimplemented"));
-  }
+					   int deref_size) = 0;
 
   /* Return the address indexed by DW_OP_GNU_addr_index.
      This can throw an exception if the index is out of range.  */
-  virtual CORE_ADDR get_addr_index (unsigned int index)
-  {
-    error (_("%s is invalid in this context"), "DW_OP_GNU_addr_index");
-  }
+  virtual CORE_ADDR get_addr_index (unsigned int index) = 0;
 
   /* Return the `object address' for DW_OP_push_object_address.  */
-  virtual CORE_ADDR get_object_address ()
-  {
-    internal_error (__FILE__, __LINE__,
-		    _("Support for DW_OP_push_object_address "
-		      "is unimplemented"));
-  }
+  virtual CORE_ADDR get_object_address () = 0;
 
 private:
 
-- 
2.7.4

[RFA 0/2] c++ patch follow-ups

[Tom Tromey]

These are follow-up patches that Pedro requested during review of my
earlier C++ series.

The first patch changes a type in minimal_symbol_reader.
The second patch changes some dwarf_expr_context methods to be pure virtual.

Built and regtested on x86-64 Fedora 24.

Tom

Re: [RFA 1/2] Change minimal_symbol_reader::record_full to take a bool

[Pedro Alves]

OK.

Thanks,
Pedro Alves

Re: [RFA 2/2] Make some dwarf_expr_context methods pure virtual

[Pedro Alves]

On 10/20/2016 10:51 PM, Tom Tromey wrote:
> This patch changes some dwarf_expr_context to be pure virtual, as
> mentioned during the discussion of an earlier patch in this series.
> 
> 2016-10-20  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2expr.h (class dwarf_expr_context)
> 	<get_frame_base, get_frame_cfa, get_tls_address, dwarf_call,
> 	push_dwarf_block_entry_value, get_addr_index, get_object_address>:
> 	Now pure virtual.
> 	* dwarf2-frame.c (class dwarf_expr_executor)
> 	<get_frame_base, get_frame_cfa, get_tls_address, dwarf_call,
> 	push_dwarf_block_entry_value, get_addr_index, get_object_address>:
> 	New methods.
> 	<invalid>: New method.

Nice, thanks.  LGTM.

Thanks,
Pedro Alves

Re: [RFA 2/2] Make some dwarf_expr_context methods pure virtual

[Ulrich Weigand]

Pedro Alves wrote:
> On 10/20/2016 10:51 PM, Tom Tromey wrote:
> > This patch changes some dwarf_expr_context to be pure virtual, as
> > mentioned during the discussion of an earlier patch in this series.
> > 
> > 2016-10-20  Tom Tromey  <tom@tromey.com>
> > 
> > 	* dwarf2expr.h (class dwarf_expr_context)
> > 	<get_frame_base, get_frame_cfa, get_tls_address, dwarf_call,
> > 	push_dwarf_block_entry_value, get_addr_index, get_object_address>:
> > 	Now pure virtual.
> > 	* dwarf2-frame.c (class dwarf_expr_executor)
> > 	<get_frame_base, get_frame_cfa, get_tls_address, dwarf_call,
> > 	push_dwarf_block_entry_value, get_addr_index, get_object_address>:
> > 	New methods.
> > 	<invalid>: New method.
> 
> Nice, thanks.  LGTM.

This seems to have broken my SPU daily build (running on RHEL 5 with a
GCC 4.1 system compiler):

gdb/dwarf2expr.h:69: warning: 'struct dwarf_expr_context' has virtual functions but non-virtual destructor

Is this a C++ version issue?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [RFA 2/2] Make some dwarf_expr_context methods pure virtual

[Tom Tromey]

>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:

Ulrich> This seems to have broken my SPU daily build (running on RHEL 5 with a
Ulrich> GCC 4.1 system compiler):

Ulrich> gdb/dwarf2expr.h:69: warning: 'struct dwarf_expr_context' has
Ulrich> virtual functions but non-virtual destructor

Ulrich> Is this a C++ version issue?

I don't know.  Adding a "virtual" to the destructor is safe though.  I
would do it but I'm traveling this week.

Tom

Re: [RFA 2/2] Make some dwarf_expr_context methods pure virtual

[Pedro Alves]

On 10/24/2016 02:36 PM, Tom Tromey wrote:
>>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:
> 
> Ulrich> This seems to have broken my SPU daily build (running on RHEL 5 with a
> Ulrich> GCC 4.1 system compiler):

Any chance you could install the newer GCC from DTS on that machine?
Otherwise, if/when we go C++11, that builder will stop working.

> Ulrich> gdb/dwarf2expr.h:69: warning: 'struct dwarf_expr_context' has
> Ulrich> virtual functions but non-virtual destructor
> 
> Ulrich> Is this a C++ version issue?
> 
> I don't know.  Adding a "virtual" to the destructor is safe though.  I
> would do it but I'm traveling this week.

Looks like -Wnon-virtual-dtor was removed from (guessing) -Wall at some point.

With GCC 7 / trunk:

$ make WERROR_CFLAGS="-Wnon-virtual-dtor" dwarf2expr.o
...
In file included from .../src/gdb/dwarf2expr.c:28:0:
.../src/gdb/dwarf2expr.h:68:8: warning: ‘struct dwarf_expr_context’ has virtual functions and accessible non-virtual destructor [-Wnon-virtual-dtor]
 struct dwarf_expr_context
        ^~~~~~~~~~~~~~~~~~

I'll add the "virtual" in a bit.

Thanks,
Pedro Alves

Re: [RFA 2/2] Make some dwarf_expr_context methods pure virtual

[Ulrich Weigand]

Pedro Alves wrote:
> >>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:
> > 
> > Ulrich> This seems to have broken my SPU daily build (running on RHEL 5 with a
> > Ulrich> GCC 4.1 system compiler):
> 
> Any chance you could install the newer GCC from DTS on that machine?

I don't think there even is a DTS for RHEL 5 on Power, or has that changed?

> Otherwise, if/when we go C++11, that builder will stop working.

Well, once GDB officially no longer supports building with GCC 4.1, I'll
have to come up with another solution; I'll probably just build my own
compiler then.  However, as long as GCC 4.1 *is* supported, I think it
is a good to actually still have a system testing that.

(In any case, I'm not sure how long it makes sense to keep the Cell SPU
daily build up and running, given that RHEL 5 is about to go out of
service anyway and more recent distros no longer support Cell ...)

> In file included from .../src/gdb/dwarf2expr.c:28:0:
> .../src/gdb/dwarf2expr.h:68:8: warning: ‘struct dwarf_expr_context’ has virtual functions and accessible non-virtual destructor [-Wnon-virtual-dtor]
>  struct dwarf_expr_context
>         ^~~~~~~~~~~~~~~~~~
> 
> I'll add the "virtual" in a bit.

Thanks!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [RFA 2/2] Make some dwarf_expr_context methods pure virtual

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Looks like -Wnon-virtual-dtor was removed from (guessing) -Wall
Pedro> at some point.

Perhaps it's worth adding it explicitly in configure.ac.
(Though I don't know why it was removed from -Wall, so maybe it's not
always appropriate, for reasons I don't know.)

Tom

[pushed] Make dwarf_expr_context's destructor virtual (Re: [RFA 2/2] Make some dwarf_expr_context methods pure virtual)

[Pedro Alves]

On 10/25/2016 02:29 PM, Ulrich Weigand wrote:
> Pedro Alves wrote:
>>>>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:
>>>
>>> Ulrich> This seems to have broken my SPU daily build (running on RHEL 5 with a
>>> Ulrich> GCC 4.1 system compiler):
>>
>> Any chance you could install the newer GCC from DTS on that machine?
> 
> I don't think there even is a DTS for RHEL 5 on Power, or has that changed?

Whoops, you're right.  Sorry, missed that.

>> Otherwise, if/when we go C++11, that builder will stop working.
> 
> Well, once GDB officially no longer supports building with GCC 4.1, I'll
> have to come up with another solution; I'll probably just build my own
> compiler then.  However, as long as GCC 4.1 *is* supported, I think it
> is a good to actually still have a system testing that.

Agreed.  I'd like to move forward with requiring C++11 soon though.
I was mainly giving it some more time:

 https://sourceware.org/ml/gdb-patches/2016-10/msg00607.html

> (In any case, I'm not sure how long it makes sense to keep the Cell SPU
> daily build up and running, given that RHEL 5 is about to go out of
> service anyway and more recent distros no longer support Cell ...)

It's up to you of course.  As we chatted on the Cauldron, it's unfortunate
since that's the best way we have to avoid multi-arch support from bit rotting.

I'd like to see gdb move in the direction of supporting seamless
remote-procedure calls.  E.g., following an RPC call across
32-bit client x 64-bit server and back, even on the same machine.
Seamless CPU+GPU debugging support likely will take advantage of
it as well in the future.  So maybe we'll gain back testing
some other way.

> 
>> In file included from .../src/gdb/dwarf2expr.c:28:0:
>> .../src/gdb/dwarf2expr.h:68:8: warning: ‘struct dwarf_expr_context’ has virtual functions and accessible non-virtual destructor [-Wnon-virtual-dtor]
>>  struct dwarf_expr_context
>>         ^~~~~~~~~~~~~~~~~~
>>
>> I'll add the "virtual" in a bit.
> 
> Thanks!

Pushed now.

From beb18c865c42ab57176099eecb65bb52e71def85 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 25 Oct 2016 14:32:35 +0100
Subject: [PATCH] Make dwarf_expr_context's destructor virtual
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Ref: https://sourceware.org/ml/gdb-patches/2016-10/msg00662.html

 $ make WERROR_CFLAGS="-Wnon-virtual-dtor" dwarf2expr.o
 ...
 In file included from .../src/gdb/dwarf2expr.c:28:0:
 .../src/gdb/dwarf2expr.h:68:8: warning: ‘struct dwarf_expr_context’ has virtual functions and accessible non-virtual destructor [-Wnon-virtual-dtor]
  struct dwarf_expr_context
	 ^~~~~~~~~~~~~~~~~~

Happens to not be a problem in practice currently because concrete
subclasses are allocated on the stack.  I.e., we don't ever delete
objects of types that derive from dwarf_expr_context through pointers
to dwarf_expr_context.

gdb/ChangeLog:
2016-10-25  Pedro Alves  <palves@redhat.com>

	* dwarf2expr.h (struct dwarf_expr_context) <~dwarf_expr_context>:
	Make virtual.
---
 gdb/ChangeLog    | 5 +++++
 gdb/dwarf2expr.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 10d6866..91f36f9 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2016-10-25  Pedro Alves  <palves@redhat.com>
+
+	* dwarf2expr.h (struct dwarf_expr_context) <~dwarf_expr_context>:
+	Make virtual.
+
 2016-10-25  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
 
 	PR build/20712
diff --git a/gdb/dwarf2expr.h b/gdb/dwarf2expr.h
index 7bf194a..3d08120 100644
--- a/gdb/dwarf2expr.h
+++ b/gdb/dwarf2expr.h
@@ -68,7 +68,7 @@ struct dwarf_stack_value
 struct dwarf_expr_context
 {
   dwarf_expr_context ();
-  ~dwarf_expr_context ();
+  virtual ~dwarf_expr_context ();
 
   void push_address (CORE_ADDR value, int in_stack_memory);
   void eval (const gdb_byte *addr, size_t len);
-- 
2.5.5

Re: [RFA 2/2] Make some dwarf_expr_context methods pure virtual

[Pedro Alves]

On 10/25/2016 02:47 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Looks like -Wnon-virtual-dtor was removed from (guessing) -Wall
> Pedro> at some point.
> 
> Perhaps it's worth adding it explicitly in configure.ac.
> (Though I don't know why it was removed from -Wall, so maybe it's not
> always appropriate, for reasons I don't know.)

Yeah, cross my mind too.  I'd like to understand why it was
removed first.  (I don't know whether it used to be in -Wall, or
default, or enabled along some other -Wfoo option.)

I found these:

 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=7302
 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=15214

but those fixed false positives.  I think that would
have supported enabling by default.  Sounds like something
else might have been identified.

Thanks,
Pedro Alves

Re: [RFA 2/2] Make some dwarf_expr_context methods pure virtual

[Trevor Saunders]

On Tue, Oct 25, 2016 at 03:04:57PM +0100, Pedro Alves wrote:
> On 10/25/2016 02:47 PM, Tom Tromey wrote:
> >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> > 
> > Pedro> Looks like -Wnon-virtual-dtor was removed from (guessing) -Wall
> > Pedro> at some point.
> > 
> > Perhaps it's worth adding it explicitly in configure.ac.
> > (Though I don't know why it was removed from -Wall, so maybe it's not
> > always appropriate, for reasons I don't know.)
> 
> Yeah, cross my mind too.  I'd like to understand why it was
> removed first.  (I don't know whether it used to be in -Wall, or
> default, or enabled along some other -Wfoo option.)

with g++ 6 I'm seeing -Wdelete-non-virtual-dtor enabled by -Wall, which
seems to be warning about the same thing?

oh, accept -wdelete-non-virtual-dtor looks like it wouldn't warn if the
class is always on the stack like it is here.

Trev

Re: [RFA 2/2] Make some dwarf_expr_context methods pure virtual

[Pedro Alves]

On 10/25/2016 05:33 PM, Trevor Saunders wrote:
> On Tue, Oct 25, 2016 at 03:04:57PM +0100, Pedro Alves wrote:
>> On 10/25/2016 02:47 PM, Tom Tromey wrote:
>>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>>>
>>> Pedro> Looks like -Wnon-virtual-dtor was removed from (guessing) -Wall
>>> Pedro> at some point.
>>>
>>> Perhaps it's worth adding it explicitly in configure.ac.
>>> (Though I don't know why it was removed from -Wall, so maybe it's not
>>> always appropriate, for reasons I don't know.)
>>
>> Yeah, cross my mind too.  I'd like to understand why it was
>> removed first.  (I don't know whether it used to be in -Wall, or
>> default, or enabled along some other -Wfoo option.)
> 
> with g++ 6 I'm seeing -Wdelete-non-virtual-dtor enabled by -Wall, which
> seems to be warning about the same thing?
> 
> oh, accept -wdelete-non-virtual-dtor looks like it wouldn't warn if the
> class is always on the stack like it is here.

Ah, nice.  Sounds like we're already covered then.

Thanks,
Pedro Alves