[PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[Pedro Alves]

The first 4 patches finally make GDB build cleanly in C++, at least on
x86_64 GNU/Linux and x86_64 mingw.  At this point, I think we should
drop the -fpermissive hack and default to -Werror in C++ mode too,
which is what the last two patches do.

This lets the buildbot catch C++ build regressions promptly -- we
already have a Fedora buildslave building in C++ mode specifically for
that, but it currently misses regressions around pointer casts and
enum conversions exactly due to -fpermissive/-Wno-error.

Other ports still need further C++ conversion work, though the build
failures shouldn't be too many and should be mostly localized to
host/target-specific code.

Pedro Alves (5):
  [C++] remote.c: Avoid enum arithmetic
  [C++] linux-thread-db.c: dladdr cast
  [C++] breakpoint.c: "no memory" software watchpoints and enum casts
  [C++] Drop -fpermissive hack
  [C++] Default to -Werror in C++ mode too

Simon Marchi (1):
  [C++] s390: Fix enum gdb_syscall conversion

 gdb/breakpoint.c           | 49 ++++++++++++++++++++++++++++++++++------------
 gdb/build-with-cxx.m4      |  3 +--
 gdb/configure              | 10 +++-------
 gdb/configure.ac           |  7 ++-----
 gdb/gdbserver/configure    |  6 ++----
 gdb/gdbserver/configure.ac |  3 +--
 gdb/linux-thread-db.c      |  2 +-
 gdb/remote.c               |  2 +-
 gdb/s390-linux-tdep.c      | 41 ++++++++++++++++++++++----------------
 gdb/target.c               | 36 +++++++++++++++++++++++-----------
 gdb/target.h               |  4 ++++
 11 files changed, 100 insertions(+), 63 deletions(-)

-- 
1.9.3

[PATCH 4/6] [C++] breakpoint.c: "no memory" software watchpoints and enum casts

[Pedro Alves]

Fixes:

 src/gdb/breakpoint.c: In function ‘void update_watchpoint(watchpoint*, int)’:
 src/gdb/breakpoint.c:2147:31: error: invalid conversion from ‘int’ to ‘target_hw_bp_type’ [-fpermissive]
     base->loc->watchpoint_type = -1;
				^

Seems better to rely on "address == -1 && length == -1" than on a enum
value that's not really part of the set of supposedly valid enum
values.  Also, factor that out to separate functions for better
localization of the concept.

gdb/ChangeLog:
2015-11-18  Pedro Alves  <palves@redhat.com>

	* breakpoint.c (software_watchpoint_add_memoryless_location)
	(is_memoryless_software_watchpoint): New functions.
	(update_watchpoint): Use
	software_watchpoint_add_memoryless_location.
	(breakpoint_address_bits): Use is_memoryless_software_watchpoint.
---
 gdb/breakpoint.c | 49 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 13 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5863573..1425d2d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1775,6 +1775,36 @@ extract_bitfield_from_watchpoint_value (struct watchpoint *w, struct value *val)
   return bit_val;
 }
 
+/* Allocate a dummy location and add it to B, which must be a software
+   watchpoint.  This is required because even if a software watchpoint
+   is not watching any memory, bpstat_stop_status requires a location
+   to be able to report stops.  */
+
+static void
+software_watchpoint_add_no_memory_location (struct breakpoint *b,
+					    struct program_space *pspace)
+{
+  gdb_assert (b->type == bp_watchpoint && b->loc == NULL);
+
+  b->loc = allocate_bp_location (b);
+  b->loc->pspace = pspace;
+  b->loc->address = -1;
+  b->loc->length = -1;
+}
+
+/* Returns true if B is a software watchpoint that is not watching any
+   memory (e.g., "watch $pc").  */
+
+static int
+is_no_memory_software_watchpoint (struct breakpoint *b)
+{
+  return (b->type == bp_watchpoint
+	  && b->loc != NULL
+	  && b->loc->next == NULL
+	  && b->loc->address == -1
+	  && b->loc->length == -1);
+}
+
 /* Assuming that B is a watchpoint:
    - Reparse watchpoint expression, if REPARSE is non-zero
    - Evaluate expression and store the result in B->val
@@ -2138,14 +2168,7 @@ update_watchpoint (struct watchpoint *b, int reparse)
 	 bpstat_stop_status requires a location to be able to report
 	 stops, so make sure there's at least a dummy one.  */
       if (b->base.type == bp_watchpoint && b->base.loc == NULL)
-	{
-	  struct breakpoint *base = &b->base;
-	  base->loc = allocate_bp_location (base);
-	  base->loc->pspace = frame_pspace;
-	  base->loc->address = -1;
-	  base->loc->length = -1;
-	  base->loc->watchpoint_type = -1;
-	}
+	software_watchpoint_add_no_memory_location (&b->base, frame_pspace);
     }
   else if (!within_current_scope)
     {
@@ -6667,15 +6690,15 @@ breakpoint_address_bits (struct breakpoint *b)
   int print_address_bits = 0;
   struct bp_location *loc;
 
+  /* Software watchpoints that aren't watching memory don't have an
+     address to print.  */
+  if (is_no_memory_software_watchpoint (b))
+    return 0;
+
   for (loc = b->loc; loc; loc = loc->next)
     {
       int addr_bit;
 
-      /* Software watchpoints that aren't watching memory don't have
-	 an address to print.  */
-      if (b->type == bp_watchpoint && loc->watchpoint_type == -1)
-	continue;
-
       addr_bit = gdbarch_addr_bit (loc->gdbarch);
       if (addr_bit > print_address_bits)
 	print_address_bits = addr_bit;
-- 
1.9.3

[PATCH 1/6] [C++] remote.c: Avoid enum arithmetic

[Pedro Alves]

Fixes:

  src/gdb/remote.c: In function ‘void remote_unpush_target()’:
  src/gdb/remote.c:4610:45: error: invalid conversion from ‘int’ to ‘strata’ [-fpermissive]
     pop_all_targets_above (process_stratum - 1);
					       ^
  In file included from src/gdb/inferior.h:38:0,
		   from src/gdb/remote.c:25:
  src/gdb/target.h:2299:13: error:   initializing argument 1 of ‘void pop_all_targets_above(strata)’ [-fpermissive]
   extern void pop_all_targets_above (enum strata above_stratum);
	       ^

I used to carry a patch in the C++ branch that just did:

 -  pop_all_targets_above (process_stratum - 1);
 +  pop_all_targets_above ((enum strata) (process_stratum - 1));

But then thought that maybe adding a routine that does exactly what we
need results in clearer code.  This is the result.

gdb/ChangeLog:
2015-11-18  Pedro Alves  <palves@redhat.com>

	* remote.c (remote_unpush_target): Use
	pop_all_targets_at_and_above instead of pop_all_targets_above.
	* target.c (unpush_target_and_assert): New function, factored out
	from ...
	(pop_all_targets_above): ... here.
	(pop_all_targets_at_and_above): New function.
	* target.h (pop_all_targets_at_and_above): Declare.
---
 gdb/remote.c |  2 +-
 gdb/target.c | 36 +++++++++++++++++++++++++-----------
 gdb/target.h |  4 ++++
 3 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index fed397a..6c86ab2 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4607,7 +4607,7 @@ remote_query_supported (void)
 static void
 remote_unpush_target (void)
 {
-  pop_all_targets_above (process_stratum - 1);
+  pop_all_targets_at_and_above (process_stratum);
 }
 
 static void
diff --git a/gdb/target.c b/gdb/target.c
index 93786c3..0ae6708 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -746,21 +746,35 @@ unpush_target (struct target_ops *t)
   return 1;
 }
 
+/* Unpush TARGET and assert that it worked.  */
+
+static void
+unpush_target_and_assert (struct target_ops *target)
+{
+  if (!unpush_target (target))
+    {
+      fprintf_unfiltered (gdb_stderr,
+			  "pop_all_targets couldn't find target %s\n",
+			  target->to_shortname);
+      internal_error (__FILE__, __LINE__,
+		      _("failed internal consistency check"));
+    }
+}
+
 void
 pop_all_targets_above (enum strata above_stratum)
 {
   while ((int) (current_target.to_stratum) > (int) above_stratum)
-    {
-      if (!unpush_target (target_stack))
-	{
-	  fprintf_unfiltered (gdb_stderr,
-			      "pop_all_targets couldn't find target %s\n",
-			      target_stack->to_shortname);
-	  internal_error (__FILE__, __LINE__,
-			  _("failed internal consistency check"));
-	  break;
-	}
-    }
+    unpush_target_and_assert (target_stack);
+}
+
+/* See target.h.  */
+
+void
+pop_all_targets_at_and_above (enum strata stratum)
+{
+  while ((int) (current_target.to_stratum) >= (int) stratum)
+    unpush_target_and_assert (target_stack);
 }
 
 void
diff --git a/gdb/target.h b/gdb/target.h
index 0105db2..e80bee5 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2294,6 +2294,10 @@ extern void target_preopen (int);
 /* Does whatever cleanup is required to get rid of all pushed targets.  */
 extern void pop_all_targets (void);
 
+/* Like pop_all_targets, but pops only targets whose stratum is at or
+   above STRATUM.  */
+extern void pop_all_targets_at_and_above (enum strata stratum);
+
 /* Like pop_all_targets, but pops only targets whose stratum is
    strictly above ABOVE_STRATUM.  */
 extern void pop_all_targets_above (enum strata above_stratum);
-- 
1.9.3

[PATCH 5/6] [C++] Drop -fpermissive hack

[Pedro Alves]

Both x86_64 GNU/Linux and x86_64 mingw-w64 build cleanly with
--enable-targets=all.  Let's drop the -fpermissive hack, in order to
let the buildbot catch C++ build regressions for us.

gdb/ChangeLog:
2015-11-18  Pedro Alves  <palves@redhat.com>

	* build-with-cxx.m4 (GDB_AC_BUILD_WITH_CXX): Remove -fpermissive.
	* configure: Regenerate.

gdb/gdbserver/ChangeLog:
2015-11-18  Pedro Alves  <palves@redhat.com>

	* configure: Regenerate.
---
 gdb/build-with-cxx.m4   | 3 +--
 gdb/configure           | 3 +--
 gdb/gdbserver/configure | 3 +--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/gdb/build-with-cxx.m4 b/gdb/build-with-cxx.m4
index 8802f4d..b6284fd 100644
--- a/gdb/build-with-cxx.m4
+++ b/gdb/build-with-cxx.m4
@@ -32,8 +32,7 @@ AC_DEFUN([GDB_AC_BUILD_WITH_CXX],
     [enable_build_with_cxx=no])
 
   if test "$enable_build_with_cxx" = "yes"; then
-    # We're using -fpermissive as shortcut for now.
-    COMPILER='$(CXX) -fpermissive'
+    COMPILER='$(CXX)'
    else
     COMPILER='$(CC)'
   fi
diff --git a/gdb/configure b/gdb/configure
index fd7ea1c..8ef3cf2 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -5015,8 +5015,7 @@ fi
 
 
   if test "$enable_build_with_cxx" = "yes"; then
-    # We're using -fpermissive as shortcut for now.
-    COMPILER='$(CXX) -fpermissive'
+    COMPILER='$(CXX)'
    else
     COMPILER='$(CC)'
   fi
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index 409f42b..4b9aa95 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -4827,8 +4827,7 @@ fi
 
 
   if test "$enable_build_with_cxx" = "yes"; then
-    # We're using -fpermissive as shortcut for now.
-    COMPILER='$(CXX) -fpermissive'
+    COMPILER='$(CXX)'
    else
     COMPILER='$(CC)'
   fi
-- 
1.9.3

[PATCH 3/6] [C++] s390: Fix enum gdb_syscall conversion

[Pedro Alves]

From: Simon Marchi <simon.marchi@ericsson.com>

Fixes:

 src/gdb/s390-linux-tdep.c: In function ‘gdb_syscall s390_canonicalize_syscall(int, s390_abi_kind)’:
 src/gdb/s390-linux-tdep.c:2622:16: error: invalid conversion from ‘int’ to ‘gdb_syscall’ [-fpermissive]
	  return syscall;
		 ^
 src/gdb/s390-linux-tdep.c:2722:16: error: invalid conversion from ‘int’ to ‘gdb_syscall’ [-fpermissive]
	  return syscall;
		 ^
 src/gdb/s390-linux-tdep.c:2725:24: error: invalid conversion from ‘int’ to ‘gdb_syscall’ [-fpermissive]
	  return syscall + 2;
			 ^
 src/gdb/s390-linux-tdep.c:2728:24: error: invalid conversion from ‘int’ to ‘gdb_syscall’ [-fpermissive]
	  return syscall + 5;
			 ^
 src/gdb/s390-linux-tdep.c:2731:24: error: invalid conversion from ‘int’ to ‘gdb_syscall’ [-fpermissive]
	  return syscall + 6;
			 ^
 src/gdb/s390-linux-tdep.c:2734:24: error: invalid conversion from ‘int’ to ‘gdb_syscall’ [-fpermissive]
	  return syscall + 7;
			 ^

gdb/ChangeLog:
2015-11-18  Simon Marchi  <simon.marchi@ericsson.com>
	    Pedro Alves  <palves@redhat.com>

	* s390-linux-tdep.c (s390_canonicalize_syscall): Add casts and
	intermediate 'int' variable.
---
 gdb/s390-linux-tdep.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index 3ce6336..0f8a3a8 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -2619,7 +2619,7 @@ s390_canonicalize_syscall (int syscall, enum s390_abi_kind abi)
     case 197: /* fstat64 */
     case 221: /* fcntl64 */
       if (abi == ABI_LINUX_S390)
-        return syscall;
+        return (enum gdb_syscall) syscall;
       return gdb_sys_no_syscall;
     /* These syscalls don't exist on s390.  */
     case 17: /* break */
@@ -2717,22 +2717,29 @@ s390_canonicalize_syscall (int syscall, enum s390_abi_kind abi)
       return gdb_sys_newfstatat;
     /* 313+ not yet supported */
     default:
-      /* Most "old" syscalls copied from i386.  */
-      if (syscall <= 221)
-        return syscall;
-      /* xattr syscalls.  */
-      if (syscall >= 224 && syscall <= 235)
-        return syscall + 2;
-      /* timer syscalls.  */
-      if (syscall >= 254 && syscall <= 262)
-        return syscall + 5;
-      /* mq_* and kexec_load */
-      if (syscall >= 271 && syscall <= 277)
-        return syscall + 6;
-      /* ioprio_set .. epoll_pwait */
-      if (syscall >= 282 && syscall <= 312)
-        return syscall + 7;
-      return gdb_sys_no_syscall;
+      {
+	int ret;
+
+	/* Most "old" syscalls copied from i386.  */
+	if (syscall <= 221)
+	  ret = syscall;
+	/* xattr syscalls.  */
+	else if (syscall >= 224 && syscall <= 235)
+	  ret = syscall + 2;
+	/* timer syscalls.  */
+	else if (syscall >= 254 && syscall <= 262)
+	  ret = syscall + 5;
+	/* mq_* and kexec_load */
+	else if (syscall >= 271 && syscall <= 277)
+	  ret = syscall + 6;
+	/* ioprio_set .. epoll_pwait */
+	else if (syscall >= 282 && syscall <= 312)
+	  ret = syscall + 7;
+	else
+	  ret = gdb_sys_no_syscall;
+
+	return (enum gdb_syscall) ret;
+      }
     }
 }
 
-- 
1.9.3

[PATCH 2/6] [C++] linux-thread-db.c: dladdr cast

[Pedro Alves]

Fixes:

 src/gdb/linux-thread-db.c: In function ‘int try_thread_db_load_1(thread_db_info*)’:
 src/gdb/linux-thread-db.c:769:53: error: invalid conversion from ‘td_err_e (*)(ps_prochandle*, td_thragent_t**) {aka td_err_e (*)(ps_prochandle*, td_thragent**)}’ to ‘const void*’ [-fpermissive]
	library = dladdr_to_soname (*info->td_ta_new_p);
						      ^
 src/gdb/linux-thread-db.c:637:1: error:   initializing argument 1 of ‘const char* dladdr_to_soname(const void*)’ [-fpermissive]
  dladdr_to_soname (const void *addr)
  ^

gdb/ChangeLog:
2015-11-18  Pedro Alves  <palves@redhat.com>

	* linux-thread-db.c (try_thread_db_load_1): Add cast.
---
 gdb/linux-thread-db.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index 41db29a..229bb0b 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -766,7 +766,7 @@ try_thread_db_load_1 (struct thread_db_info *info)
       struct ui_file *file;
       const char *library;
 
-      library = dladdr_to_soname (*info->td_ta_new_p);
+      library = dladdr_to_soname ((const void *) *info->td_ta_new_p);
       if (library == NULL)
 	library = LIBTHREAD_DB_SO;
 
-- 
1.9.3

Re: [PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[Simon Marchi]

On 15-11-18 11:39 AM, Pedro Alves wrote:
> The first 4 patches finally make GDB build cleanly in C++, at least on
> x86_64 GNU/Linux and x86_64 mingw.  At this point, I think we should
> drop the -fpermissive hack and default to -Werror in C++ mode too,
> which is what the last two patches do.
> 
> This lets the buildbot catch C++ build regressions promptly -- we
> already have a Fedora buildslave building in C++ mode specifically for
> that, but it currently misses regressions around pointer casts and
> enum conversions exactly due to -fpermissive/-Wno-error.
> 
> Other ports still need further C++ conversion work, though the build
> failures shouldn't be too many and should be mostly localized to
> host/target-specific code.
> 
> Pedro Alves (5):
>   [C++] remote.c: Avoid enum arithmetic
>   [C++] linux-thread-db.c: dladdr cast
>   [C++] breakpoint.c: "no memory" software watchpoints and enum casts
>   [C++] Drop -fpermissive hack
>   [C++] Default to -Werror in C++ mode too
> 
> Simon Marchi (1):
>   [C++] s390: Fix enum gdb_syscall conversion
> 
>  gdb/breakpoint.c           | 49 ++++++++++++++++++++++++++++++++++------------
>  gdb/build-with-cxx.m4      |  3 +--
>  gdb/configure              | 10 +++-------
>  gdb/configure.ac           |  7 ++-----
>  gdb/gdbserver/configure    |  6 ++----
>  gdb/gdbserver/configure.ac |  3 +--
>  gdb/linux-thread-db.c      |  2 +-
>  gdb/remote.c               |  2 +-
>  gdb/s390-linux-tdep.c      | 41 ++++++++++++++++++++++----------------
>  gdb/target.c               | 36 +++++++++++++++++++++++-----------
>  gdb/target.h               |  4 ++++
>  11 files changed, 100 insertions(+), 63 deletions(-)
> 

This all looks good to me!

[PATCH 6/6] [C++] Default to -Werror in C++ mode too

[Pedro Alves]

Both x86_64 GNU/Linux and x86_64 mingw-w64 build cleanly with
--enable-targets=all.  This enables -Werror by default in C++ mode
too, in order to let the buildbot catch C++ build regressions for us.

gdb/ChangeLog:
2015-11-18  Pedro Alves  <palves@redhat.com>

        * configure.ac (ERROR_ON_WARNING): Don't check whether in C++
	mode.
        * configure: Regenerate.

gdb/gdbserver/ChangeLog:
2015-11-18  Pedro Alves  <palves@redhat.com>

        * configure.ac (ERROR_ON_WARNING): Don't check whether in C++
	mode.
        * configure: Regenerate.
---
 gdb/configure              | 7 ++-----
 gdb/configure.ac           | 7 ++-----
 gdb/gdbserver/configure    | 3 +--
 gdb/gdbserver/configure.ac | 3 +--
 4 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/gdb/configure b/gdb/configure
index 8ef3cf2..249a399 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -14293,11 +14293,8 @@ if test "${enable_werror+set}" = set; then :
 fi
 
 
-# Enable -Werror by default when using gcc in C mode.  Leave it off
-# for C++ until we're warning clean.  Turn it off for releases.
-if test "${GCC}" = yes -a -z "${ERROR_ON_WARNING}" \
-   && test x"$enable_build_with_cxx" != x"yes" \
-   && $development; then
+# Enable -Werror by default when using gcc.  Turn it off for releases.
+if test "${GCC}" = yes -a -z "${ERROR_ON_WARNING}" && $development; then
     ERROR_ON_WARNING=yes
 fi
 
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 29d0b63..ebd797b 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1932,11 +1932,8 @@ AC_ARG_ENABLE(werror,
      *) AC_MSG_ERROR(bad value ${enableval} for --enable-werror) ;;
    esac])
 
-# Enable -Werror by default when using gcc in C mode.  Leave it off
-# for C++ until we're warning clean.  Turn it off for releases.
-if test "${GCC}" = yes -a -z "${ERROR_ON_WARNING}" \
-   && test x"$enable_build_with_cxx" != x"yes" \
-   && $development; then
+# Enable -Werror by default when using gcc.  Turn it off for releases.
+if test "${GCC}" = yes -a -z "${ERROR_ON_WARNING}" && $development; then
     ERROR_ON_WARNING=yes
 fi
 
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index 4b9aa95..87dfda0 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -6106,8 +6106,7 @@ if test "${enable_werror+set}" = set; then :
 fi
 
 
-# Enable -Werror by default when using gcc in C mode.  Leave it off
-# for C++ until we're warning clean.  Turn it off for releases.
+# Enable -Werror by default when using gcc.  Turn it off for releases.
 if test "${GCC}" = yes -a -z "${ERROR_ON_WARNING}" \
    && test x"$enable_build_with_cxx" != x"yes" \
    && $development; then
diff --git a/gdb/gdbserver/configure.ac b/gdb/gdbserver/configure.ac
index 0fe0a35..d50d0f1 100644
--- a/gdb/gdbserver/configure.ac
+++ b/gdb/gdbserver/configure.ac
@@ -157,8 +157,7 @@ AC_ARG_ENABLE(werror,
      *) AC_MSG_ERROR(bad value ${enableval} for --enable-werror) ;;
    esac])
 
-# Enable -Werror by default when using gcc in C mode.  Leave it off
-# for C++ until we're warning clean.  Turn it off for releases.
+# Enable -Werror by default when using gcc.  Turn it off for releases.
 if test "${GCC}" = yes -a -z "${ERROR_ON_WARNING}" \
    && test x"$enable_build_with_cxx" != x"yes" \
    && $development; then
-- 
1.9.3

Re: [PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[Yao Qi]

On 18/11/15 16:39, Pedro Alves wrote:
> The first 4 patches finally make GDB build cleanly in C++, at least on
> x86_64 GNU/Linux and x86_64 mingw.  At this point, I think we should
> drop the -fpermissive hack and default to -Werror in C++ mode too,
> which is what the last two patches do.

Yeah! Good to see the switch to C++ will happen soon.  Thanks, Pedro
and Simon.

>
> This lets the buildbot catch C++ build regressions promptly -- we
> already have a Fedora buildslave building in C++ mode specifically for
> that, but it currently misses regressions around pointer casts and
> enum conversions exactly due to -fpermissive/-Wno-error.
>
> Other ports still need further C++ conversion work, though the build
> failures shouldn't be too many and should be mostly localized to
> host/target-specific code.

I don't have objections to this patch set, just want to know what is
your plan to commit it.  I'll fix C++ warnings and errors in ARM
and AArch64 code.

-- 
Yao (齐尧)

Re: [PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[Pedro Alves]

Hi Yao,

On 11/18/2015 05:44 PM, Yao Qi wrote:

> I don't have objections to this patch set, just want to know what is
> your plan to commit it.

I was planning on committing it as soon as I got positive
reviews, which seems like I have now.  :-)  Do you see a reason to
hold it for now?

My thinking is that most of the C++ problems are now sorted out, so
the -fpermissive hack no longer helps that much.  Note one can always
explicitly configure with CXX="g++ -fpermissive" if necessary.

> I'll fix C++ warnings and errors in ARM and AArch64 code.

Thanks!

-- 
Pedro Alves

Re: [PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> I was planning on committing it as soon as I got positive
> reviews, which seems like I have now.  :-)  Do you see a reason to
> hold it for now?

No, I don't.  Looks we still need --enable-build-with-cxx to turn C++
mode on.

-- 
Yao (齐尧)

Re: [PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[Pedro Alves]

On 11/19/2015 11:27 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> I was planning on committing it as soon as I got positive
>> reviews, which seems like I have now.  :-)  Do you see a reason to
>> hold it for now?
> 
> No, I don't.  Looks we still need --enable-build-with-cxx to turn C++
> mode on.

Yeah, my plan here was to "lock" (*) ports to C++ mode one by one, as soon
as they build in C++ mode.  Actually, I think a negative/reverse list is even
better.  This allows keeping track of ports/hosts people really still care
about, and, gives us an easy defined stopping point (when the list is clear).
What would you think of this approach?

I should probably move this to a separate thread, push this to a branch on
sourceware.org (to collect a better initial set of still-needs-conversion-work
hosts, with community help) and announce the intent on the gdb@ list, for
wider visibility/discussion.

(*) - For "locked" ports, one can still force C mode meanwhile
with --enable-build-with-cxx=no, until all ports are converted and we
decide to drop C mode.

---
From b60a477edf5b58a950a9ddf2d1520f95874a1089 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 19 Nov 2015 14:40:04 +0000
Subject: [PATCH] Default to building in C++ mode

... except on hosts/targets that haven't been converted yet.

gdb/ChangeLog:
2015-11-19  Pedro Alves  <palves@redhat.com>

	* build-with-cxx.m4 (GDB_AC_BUILD_WITH_CXX): Default to yes unless
	building on some hosts.
	* configure: Renegerate.

gdb/gdbserver/ChangeLog:
2015-11-19  Pedro Alves  <palves@redhat.com>

	* configure: Renegerate.
---
 gdb/build-with-cxx.m4   | 18 ++++++++++++++++--
 gdb/configure           | 17 +++++++++++++++--
 gdb/gdbserver/configure | 17 +++++++++++++++--
 3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/gdb/build-with-cxx.m4 b/gdb/build-with-cxx.m4
index b6284fd..03ff54d 100644
--- a/gdb/build-with-cxx.m4
+++ b/gdb/build-with-cxx.m4
@@ -21,6 +21,21 @@ dnl allowing a user to build with a C++ compiler.
 
 AC_DEFUN([GDB_AC_BUILD_WITH_CXX],
 [
+  # The "doesn't support C++ yet" hall of shame.
+  case $host in
+    *-*aix* | \
+    *-*go32* | \
+    *-*darwin* | \
+    *-*solaris* | \
+    *-*nto* | \
+    *-*bsd* | \
+    xtensa*-*-linux* | \
+    null)
+      enable_build_with_cxx=no ;;
+    *)
+      enable_build_with_cxx=yes ;;
+  esac
+
   AC_ARG_ENABLE(build-with-cxx,
   AS_HELP_STRING([--enable-build-with-cxx], [build with C++ compiler instead of C compiler]),
     [case $enableval in
@@ -28,8 +43,7 @@ AC_DEFUN([GDB_AC_BUILD_WITH_CXX],
 	  ;;
       *)
 	  AC_MSG_ERROR([bad value $enableval for --enable-build-with-cxx]) ;;
-    esac],
-    [enable_build_with_cxx=no])
+    esac])
 
   if test "$enable_build_with_cxx" = "yes"; then
     COMPILER='$(CXX)'
diff --git a/gdb/configure b/gdb/configure
index 249a399..fce2154 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -5001,6 +5001,21 @@ program_transform_name=`$as_echo "$program_transform_name" | sed "$ac_script"`
 
 # See if we are building with C++, and substitute COMPILER.
 
+  # The "doesn't support C++ yet" hall of shame.
+  case $host in
+    *-*aix* | \
+    *-*go32* | \
+    *-*darwin* | \
+    *-*solaris* | \
+    *-*nto* | \
+    *-*bsd* | \
+    xtensa*-*-linux* | \
+    null)
+      enable_build_with_cxx=no ;;
+    *)
+      enable_build_with_cxx=yes ;;
+  esac
+
   # Check whether --enable-build-with-cxx was given.
 if test "${enable_build_with_cxx+set}" = set; then :
   enableval=$enable_build_with_cxx; case $enableval in
@@ -5009,8 +5024,6 @@ if test "${enable_build_with_cxx+set}" = set; then :
       *)
 	  as_fn_error "bad value $enableval for --enable-build-with-cxx" "$LINENO" 5 ;;
     esac
-else
-  enable_build_with_cxx=no
 fi
 
 
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index 87dfda0..9b2018c 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -4813,6 +4813,21 @@ fi
 
 # See if we are building with C++, and substitute COMPILER.
 
+  # The "doesn't support C++ yet" hall of shame.
+  case $host in
+    *-*aix* | \
+    *-*go32* | \
+    *-*darwin* | \
+    *-*solaris* | \
+    *-*nto* | \
+    *-*bsd* | \
+    xtensa*-*-linux* | \
+    null)
+      enable_build_with_cxx=no ;;
+    *)
+      enable_build_with_cxx=yes ;;
+  esac
+
   # Check whether --enable-build-with-cxx was given.
 if test "${enable_build_with_cxx+set}" = set; then :
   enableval=$enable_build_with_cxx; case $enableval in
@@ -4821,8 +4836,6 @@ if test "${enable_build_with_cxx+set}" = set; then :
       *)
 	  as_fn_error "bad value $enableval for --enable-build-with-cxx" "$LINENO" 5 ;;
     esac
-else
-  enable_build_with_cxx=no
 fi
 
 
-- 
1.9.3

Re: [PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Yeah, my plan here was to "lock" (*) ports to C++ mode one by one, as soon
> as they build in C++ mode.  Actually, I think a negative/reverse list is even
> better.  This allows keeping track of ports/hosts people really still care
> about, and, gives us an easy defined stopping point (when the list is clear).
> What would you think of this approach?

Yes, this approach is OK to me.  However, as you said, we need to
announce this on gdb@ first.  Before we build GDB in C++ mode for some
hosts, we need to test it.  IIRC, we didn't test GDB built in C++
before.

Another thing in my mind is the release schedule.  We have two months,
but people are off around Christmas and New Year.  Do we want GDB 8.0
built in C++ in default in some hosts, such as linux?  I am not sure.

>
> I should probably move this to a separate thread, push this to a branch on
> sourceware.org (to collect a better initial set of still-needs-conversion-work
> hosts, with community help) and announce the intent on the gdb@ list, for
> wider visibility/discussion.

Yes, let's do that.

> ---
>  gdb/build-with-cxx.m4   | 18 ++++++++++++++++--
>  gdb/configure           | 17 +++++++++++++++--
>  gdb/gdbserver/configure | 17 +++++++++++++++--
>  3 files changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/build-with-cxx.m4 b/gdb/build-with-cxx.m4
> index b6284fd..03ff54d 100644
> --- a/gdb/build-with-cxx.m4
> +++ b/gdb/build-with-cxx.m4
> @@ -21,6 +21,21 @@ dnl allowing a user to build with a C++ compiler.
>  
>  AC_DEFUN([GDB_AC_BUILD_WITH_CXX],
>  [
> +  # The "doesn't support C++ yet" hall of shame.
> +  case $host in
> +    *-*aix* | \
> +    *-*go32* | \
> +    *-*darwin* | \
> +    *-*solaris* | \
> +    *-*nto* | \
> +    *-*bsd* | \
> +    xtensa*-*-linux* | \

Why do we especially exclude xtensa*-*-linux* from building in C++ mode?

-- 
Yao (齐尧)

Re: [PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[Pedro Alves]

On 11/20/2015 09:46 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Yeah, my plan here was to "lock" (*) ports to C++ mode one by one, as soon
>> as they build in C++ mode.  Actually, I think a negative/reverse list is even
>> better.  This allows keeping track of ports/hosts people really still care
>> about, and, gives us an easy defined stopping point (when the list is clear).
>> What would you think of this approach?
> 
> Yes, this approach is OK to me.  However, as you said, we need to
> announce this on gdb@ first.  Before we build GDB in C++ mode for some
> hosts, we need to test it.  IIRC, we didn't test GDB built in C++
> before.

On my machine (x86_64 Fedora 20), C++ mode has no regressions compared to C mode.
If C++ is flipped to on by default on (e.g.) x86_64 GNU/Linux, then the x86_64 GNU/Linux
buildbot builders will automatically start testing in C++ mode too.  If we do this,
then the Fedora builder (Fedora-x86_64-cxx-build-m64) that is specifically building
with --enable-build-with-cxx should be flipped to build with --enable-build-with-cxx=no,
to catch C mode regressions, for as long as we support C mode.

> 
> Another thing in my mind is the release schedule.  We have two months,
> but people are off around Christmas and New Year.  Do we want GDB 8.0
> built in C++ in default in some hosts, such as linux?  I am not sure.

Can't see why not.  But we can always flip back to C for the release.

> 
>>
>> I should probably move this to a separate thread, push this to a branch on
>> sourceware.org (to collect a better initial set of still-needs-conversion-work
>> hosts, with community help) and announce the intent on the gdb@ list, for
>> wider visibility/discussion.
> 
> Yes, let's do that.

OK, I'll try to find some time to do it.

>>  AC_DEFUN([GDB_AC_BUILD_WITH_CXX],
>>  [
>> +  # The "doesn't support C++ yet" hall of shame.
>> +  case $host in
>> +    *-*aix* | \
>> +    *-*go32* | \
>> +    *-*darwin* | \
>> +    *-*solaris* | \
>> +    *-*nto* | \
>> +    *-*bsd* | \
>> +    xtensa*-*-linux* | \
> 
> Why do we especially exclude xtensa*-*-linux* from building in C++ mode?

Just because that one I know for sure doesn't build in C++ mode
yet -- I have a patch from Simon in my github C++ branch that fixes
gdb/xtensa-linux-nat.c in C++ mode, which is not upstream yet.

Thanks,
Pedro Alves

Re: [PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> On my machine (x86_64 Fedora 20), C++ mode has no regressions compared
> to C mode.

I compared the test result on native arm and aarch64 gdb, there is no
regression either.

> If C++ is flipped to on by default on (e.g.) x86_64 GNU/Linux, then
> the x86_64 GNU/Linux
> buildbot builders will automatically start testing in C++ mode too.
> If we do this,
> then the Fedora builder (Fedora-x86_64-cxx-build-m64) that is
> specifically building
> with --enable-build-with-cxx should be flipped to build with
> --enable-build-with-cxx=no,
> to catch C mode regressions, for as long as we support C mode.
>

OK, that is fine to me.  After we start to build GDB in C++ mode on some
hosts, I hope we don't have to support both modes too long.  According the
c++ conversion in gcc, ENABLE_BUILD_WITH_CXX on configure was added in
2009, and was removed on 2012.

-- 
Yao (齐尧)

Re: [PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[Pedro Alves]

On 11/24/2015 11:01 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> On my machine (x86_64 Fedora 20), C++ mode has no regressions compared
>> to C mode.
> 
> I compared the test result on native arm and aarch64 gdb, there is no
> regression either.

Great, thanks!

>> If C++ is flipped to on by default on (e.g.) x86_64 GNU/Linux, then
>> the x86_64 GNU/Linux
>> buildbot builders will automatically start testing in C++ mode too.
>> If we do this,
>> then the Fedora builder (Fedora-x86_64-cxx-build-m64) that is
>> specifically building
>> with --enable-build-with-cxx should be flipped to build with
>> --enable-build-with-cxx=no,
>> to catch C mode regressions, for as long as we support C mode.
>>
> 
> OK, that is fine to me.  After we start to build GDB in C++ mode on some
> hosts, I hope we don't have to support both modes too long.  According the
> c++ conversion in gcc, ENABLE_BUILD_WITH_CXX on configure was added in
> 2009, and was removed on 2012.

Wow, that long?  I seriously hope we move faster.  All depends on the help
we get though...

Thanks,
Pedro Alves

Re: [PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[Joel Brobecker]

> Wow, that long?  I seriously hope we move faster.  All depends on the help
> we get though...

I'll try to move as quickly as I can, but some targets might be
a bit difficult for us, because we now have to build a C++ compiler,
which is not completely obvious (I'm thinking AIX or LynxOS).

-- 
Joel

Re: [PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[Pedro Alves]

On 11/20/2015 11:21 AM, Pedro Alves wrote:
>>> >> I should probably move this to a separate thread, push this to a branch on
>>> >> sourceware.org (to collect a better initial set of still-needs-conversion-work
>>> >> hosts, with community help) and announce the intent on the gdb@ list, for
>>> >> wider visibility/discussion.
>> > 
>> > Yes, let's do that.
> OK, I'll try to find some time to do it.
> 

Now done:
  https://sourceware.org/ml/gdb/2015-11/msg00035.html

Thanks,
Pedro Alves

Re: [PATCH 0/6] [C++] Drop -fpermissive hack, enable -Werror

[Pedro Alves]

On 11/19/2015 11:27 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> I was planning on committing it as soon as I got positive
>> reviews, which seems like I have now.  :-)  Do you see a reason to
>> hold it for now?
> 
> No, I don't.

BTW, I pushed the series in.

So long -fpermissive, you've been a good friend, but we need to
move on.  :-)

Thanks,
Pedro Alves