Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Eli Zaretskii]

> From: Joel Brobecker <brobecker@adacore.com>
> Date: Fri,  5 Jan 2018 08:18:05 +0400 (+04)
> 
> A quick message to announce that the GDB 8.1 branch has just been created.
> 
> The prerelease snapshots will be available at:
> 
>     ftp://sourceware.org/pub/gdb/snapshots/branch/gdb.tar.xz
>     ftp://sourceware.org/pub/gdb/snapshots/branch/gdb.tar.gz

I've built this pre-release with mingw.org's MinGW, and found a few
problems.

The most serious one is the following compilation error:

     g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -fno-strict-aliasing -DNDEBUG -fwrapv -c -o python/py-arch.o -MT python/py-arch.o -MMD -MP  -MF python/.deps/py-arch.Tpo python/py-arch.c
     In file included from d:\usr\lib\gcc\mingw32\6.3.0\include\c++\math.h:36:0,
		      from build-gnulib/import/math.h:27,
		      from d:/usr/Python26/include/pyport.h:235,
		      from d:/usr/Python26/include/Python.h:58,
		      from python/python-internal.h:94,
		      from python/py-arch.c:24:
     d:\usr\lib\gcc\mingw32\6.3.0\include\c++\cmath:1157:11: error: '::hypot' has not been declared
	using ::hypot;
		^~~~~
     Makefile:1618: recipe for target `python/py-arch.o' failed

Googling suggests the following solution; is it okay to push this
(with the necessary logs and after doing the "paperwork" required for
branch changes)?  Or does someone have better ideas?  (Does this work
in MinGW64?)

--- gdb/python/python-internal.h~0	2018-01-12 05:31:04.000000000 +0200
+++ gdb/python/python-internal.h	2018-01-16 08:56:10.717759900 +0200
@@ -85,6 +85,12 @@
 #define HAVE_SNPRINTF 1
 #endif
 
+/* Another kludge to avoid compilation errors because MinGW defines
+   'hypot' to '_hypot', but the C++ headers says "using ::hypot".  */
+#if defined(__MINGW32__) && defined(__cplusplus)
+# define _hypot hypot
+#endif
+
 /* Request clean size types from Python.  */
 #define PY_SSIZE_T_CLEAN
 

The other problems I saw are compilation warnings, see below.  Some of
them sound like GCC doesn't understand our TRY/CATCH blocks, but others
seem like real problems, at least from the POV of a naïve reader such
as myself.  I wonder how come others don't see these warnings, and why
weren't they fixed during development, when we use -Werror.  Here are
the warnings I saw:

     g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -c -o cli/cli-cmds.o -MT cli/cli-cmds.o -MMD -MP  -MF cli/.deps/cli-cmds.Tpo cli/cli-cmds.c
     cli/cli-cmds.c: In function 'void complete_command(const char*, int)':
     cli/cli-cmds.c:277:71: warning: 'word' may be used uninitialized in this function [-Wmaybe-uninitialized]
	  = tracker->build_completion_result (word, word - arg, strlen (arg));
									    ^
     cli/cli-cmds.c:277:71: warning: 'tracker' may be used uninitialized in this function [-Wmaybe-uninitialized]

     g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -c -o breakpoint.o -MT breakpoint.o -MMD -MP  -MF ./.deps/breakpoint.Tpo breakpoint.c
     breakpoint.c: In function 'void check_status_watchpoint(bpstat)':
     breakpoint.c:5079:4: warning: 'e' may be used uninitialized in this function [-Wmaybe-uninitialized]
	 switch (e)
	 ^~~~~~
     breakpoint.c:5056:20: note: 'e' was declared here
	 wp_check_result e;
                    ^

     g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -c -o infrun.o -MT infrun.o -MMD -MP  -MF ./.deps/infrun.Tpo infrun.c
     In file included from infrun.c:26:0:
     inferior.h: In function 'void handle_vfork_child_exec_or_exit(int)':
     inferior.h:537:39: warning: '*((void*)(& maybe_restore_inferior)+20).scoped_restore_current_inferior::m_saved_inf' may be used uninitialized in this function [-Wmaybe-uninitialized]
	{ set_current_inferior (m_saved_inf); }
					    ^
     infrun.c:927:6: note: '*((void*)(& maybe_restore_inferior)+20).scoped_restore_current_inferior::m_saved_inf' was declared here
	   maybe_restore_inferior;
	   ^~~~~~~~~~~~~~~~~~~~~~
     In file included from inferior.h:48:0,
		      from infrun.c:26:
     progspace.h:285:47: warning: '*((void*)(& maybe_restore_inferior)+16).scoped_restore_current_program_space::m_saved_pspace' may be used uninitialized in this function [-Wmaybe-uninitialized]
	{ set_current_program_space (m_saved_pspace); }
						    ^
     infrun.c:927:6: note: '*((void*)(& maybe_restore_inferior)+16).scoped_restore_current_program_space::m_saved_pspace' was declared here
	   maybe_restore_inferior;
	   ^~~~~~~~~~~~~~~~~~~~~~

     g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -c -o typeprint.o -MT typeprint.o -MMD -MP  -MF ./.deps/
     typeprint.Tpo typeprint.c
     typeprint.c: In function 'void whatis_exp(const char*, int)':
     typeprint.c:515:60: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]
       real_type = value_rtti_type (val, &full, &top, &using_enc);
                                                            ^

     g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -c -o compile/compile.o -MT compile/compile.o -MMD -MP -MF compile/.deps/compile.Tpo compile/compile.c
     compile/compile.c: In function 'void eval_compile_command(command_line*, const char*, compile_i_scope_types, void*)':
     compile/compile.c:549:20: warning: 'triplet_rx' may be used uninitialized in this function [-Wmaybe-uninitialized]
	      argc, argv);
			 ^
     compile/compile.c:466:9: note: 'triplet_rx' was declared here
	char *triplet_rx;
	      ^~~~~~~~~~

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Sergio Durigan Junior]

On Tuesday, January 16 2018, Eli Zaretskii wrote:

> I've built this pre-release with mingw.org's MinGW, and found a few
> problems.

[...]

> The other problems I saw are compilation warnings, see below.  Some of
> them sound like GCC doesn't understand our TRY/CATCH blocks, but others
> seem like real problems, at least from the POV of a naïve reader such
> as myself.  I wonder how come others don't see these warnings, and why
> weren't they fixed during development, when we use -Werror.  Here are
> the warnings I saw:
>
[...]
>      g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -c -o infrun.o -MT infrun.o -MMD -MP  -MF ./.deps/infrun.Tpo infrun.c
>      In file included from infrun.c:26:0:
>      inferior.h: In function 'void handle_vfork_child_exec_or_exit(int)':
>      inferior.h:537:39: warning: '*((void*)(& maybe_restore_inferior)+20).scoped_restore_current_inferior::m_saved_inf' may be used uninitialized in this function [-Wmaybe-uninitialized]
> 	{ set_current_inferior (m_saved_inf); }
> 					    ^
>      infrun.c:927:6: note: '*((void*)(& maybe_restore_inferior)+20).scoped_restore_current_inferior::m_saved_inf' was declared here
> 	   maybe_restore_inferior;
> 	   ^~~~~~~~~~~~~~~~~~~~~~
>      In file included from inferior.h:48:0,
> 		      from infrun.c:26:
>      progspace.h:285:47: warning: '*((void*)(& maybe_restore_inferior)+16).scoped_restore_current_program_space::m_saved_pspace' may be used uninitialized in this function [-Wmaybe-uninitialized]
> 	{ set_current_program_space (m_saved_pspace); }
> 						    ^
>      infrun.c:927:6: note: '*((void*)(& maybe_restore_inferior)+16).scoped_restore_current_program_space::m_saved_pspace' was declared here
> 	   maybe_restore_inferior;
> 	   ^~~~~~~~~~~~~~~~~~~~~~
>
>      g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -c -o typeprint.o -MT typeprint.o -MMD -MP  -MF ./.deps/
>      typeprint.Tpo typeprint.c
>      typeprint.c: In function 'void whatis_exp(const char*, int)':
>      typeprint.c:515:60: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]
>        real_type = value_rtti_type (val, &full, &top, &using_enc);
>                                                             ^
>
>      g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -c -o compile/compile.o -MT compile/compile.o -MMD -MP -MF compile/.deps/compile.Tpo compile/compile.c
>      compile/compile.c: In function 'void eval_compile_command(command_line*, const char*, compile_i_scope_types, void*)':
>      compile/compile.c:549:20: warning: 'triplet_rx' may be used uninitialized in this function [-Wmaybe-uninitialized]
> 	      argc, argv);
> 			 ^
>      compile/compile.c:466:9: note: 'triplet_rx' was declared here
> 	char *triplet_rx;
> 	      ^~~~~~~~~~

FWIW, I've seen these warnings when preparing a Fedora GDB release.
I'll take a look at them.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

[PATCH] Fix warning on gdb/compile/compile.c (C++-ify "triplet_rx")

[Sergio Durigan Junior]

This fixes a GCC warning that happens when compiling
gdb/compile/compile.c on some GCC versions (e.g., "gcc (GCC) 7.2.1
20180104 (Red Hat 7.2.1-6)").

It's a simple patch that converts "triplet_rx" from "char *" to
"std::string", thus guaranteeing that it will be always initialized.

I've regtested this patch and did not find any regressions.  OK to
apply on both master and 8.1 (after creating a bug for it)?

gdb/ChangeLog:
2018-01-16  Sergio Durigan Junior  <sergiodj@redhat.com>

	* compile/compile.c (compile_to_object): Convert "triplet_rx"
	to "std::string".
---
 gdb/compile/compile.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 2ee75930ac..47646169c8 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -463,7 +463,7 @@ compile_to_object (struct command_line *cmd, const char *cmd_string,
   char **argv;
   int ok;
   struct gdbarch *gdbarch = get_current_arch ();
-  char *triplet_rx;
+  std::string triplet_rx;
   char *error_message;
 
   if (!target_has_execution)
@@ -527,15 +527,15 @@ compile_to_object (struct command_line *cmd, const char *cmd_string,
     }
   else
     {
-      const char *os_rx = osabi_triplet_regexp (gdbarch_osabi (gdbarch));
-      const char *arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);
+      std::string os_rx = osabi_triplet_regexp (gdbarch_osabi (gdbarch));
+      std::string arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);
 
       /* Allow triplets with or without vendor set.  */
-      triplet_rx = concat (arch_rx, "(-[^-]*)?-", os_rx, (char *) NULL);
-      make_cleanup (xfree, triplet_rx);
+      triplet_rx = arch_rx + "(-[^-]*)?-" + os_rx;
 
       if (compiler->fe->ops->version >= GCC_FE_VERSION_1)
-	compiler->fe->ops->set_triplet_regexp (compiler->fe, triplet_rx);
+	compiler->fe->ops->set_triplet_regexp (compiler->fe,
+					       triplet_rx.c_str ());
     }
 
   /* Set compiler command-line arguments.  */
@@ -545,7 +545,8 @@ compile_to_object (struct command_line *cmd, const char *cmd_string,
   if (compiler->fe->ops->version >= GCC_FE_VERSION_1)
     error_message = compiler->fe->ops->set_arguments (compiler->fe, argc, argv);
   else
-    error_message = compiler->fe->ops->set_arguments_v0 (compiler->fe, triplet_rx,
+    error_message = compiler->fe->ops->set_arguments_v0 (compiler->fe,
+							 triplet_rx.c_str (),
 							 argc, argv);
   if (error_message != NULL)
     {
-- 
2.14.3

Re: [PATCH] Fix warning on gdb/compile/compile.c (C++-ify "triplet_rx")

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,	Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Tue, 16 Jan 2018 14:46:41 -0500
> 
> This fixes a GCC warning that happens when compiling
> gdb/compile/compile.c on some GCC versions (e.g., "gcc (GCC) 7.2.1
> 20180104 (Red Hat 7.2.1-6)").
> 
> It's a simple patch that converts "triplet_rx" from "char *" to
> "std::string", thus guaranteeing that it will be always initialized.
> 
> I've regtested this patch and did not find any regressions.  OK to
> apply on both master and 8.1 (after creating a bug for it)?
> 
> gdb/ChangeLog:
> 2018-01-16  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* compile/compile.c (compile_to_object): Convert "triplet_rx"
> 	to "std::string".

Thanks, this fixes the warning for me.

Re: [PATCH] Fix warning on gdb/compile/compile.c (C++-ify "triplet_rx")

[Simon Marchi]

On 2018-01-16 14:46, Sergio Durigan Junior wrote:
> This fixes a GCC warning that happens when compiling
> gdb/compile/compile.c on some GCC versions (e.g., "gcc (GCC) 7.2.1
> 20180104 (Red Hat 7.2.1-6)").
> 
> It's a simple patch that converts "triplet_rx" from "char *" to
> "std::string", thus guaranteeing that it will be always initialized.
> 
> I've regtested this patch and did not find any regressions.  OK to
> apply on both master and 8.1 (after creating a bug for it)?
> 
> gdb/ChangeLog:
> 2018-01-16  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* compile/compile.c (compile_to_object): Convert "triplet_rx"
> 	to "std::string".
> ---
>  gdb/compile/compile.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
> index 2ee75930ac..47646169c8 100644
> --- a/gdb/compile/compile.c
> +++ b/gdb/compile/compile.c
> @@ -463,7 +463,7 @@ compile_to_object (struct command_line *cmd, const
> char *cmd_string,
>    char **argv;
>    int ok;
>    struct gdbarch *gdbarch = get_current_arch ();
> -  char *triplet_rx;
> +  std::string triplet_rx;
>    char *error_message;
> 
>    if (!target_has_execution)
> @@ -527,15 +527,15 @@ compile_to_object (struct command_line *cmd,
> const char *cmd_string,
>      }
>    else
>      {
> -      const char *os_rx = osabi_triplet_regexp (gdbarch_osabi 
> (gdbarch));
> -      const char *arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);
> +      std::string os_rx = osabi_triplet_regexp (gdbarch_osabi 
> (gdbarch));
> +      std::string arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);

Making these std::string makes unnecessary copies.

You can write the line below like this:

   triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + os_rx;

Otherwise, LGTM.

Simon

Re: [PATCH] Fix warning on gdb/compile/compile.c (C++-ify "triplet_rx")

[Sergio Durigan Junior]

On Wednesday, January 17 2018, Simon Marchi wrote:

> On 2018-01-16 14:46, Sergio Durigan Junior wrote:
>> This fixes a GCC warning that happens when compiling
>> gdb/compile/compile.c on some GCC versions (e.g., "gcc (GCC) 7.2.1
>> 20180104 (Red Hat 7.2.1-6)").
>>
>> It's a simple patch that converts "triplet_rx" from "char *" to
>> "std::string", thus guaranteeing that it will be always initialized.
>>
>> I've regtested this patch and did not find any regressions.  OK to
>> apply on both master and 8.1 (after creating a bug for it)?
>>
>> gdb/ChangeLog:
>> 2018-01-16  Sergio Durigan Junior  <sergiodj@redhat.com>
>>
>> 	* compile/compile.c (compile_to_object): Convert "triplet_rx"
>> 	to "std::string".
>> ---
>>  gdb/compile/compile.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
>> index 2ee75930ac..47646169c8 100644
>> --- a/gdb/compile/compile.c
>> +++ b/gdb/compile/compile.c
>> @@ -463,7 +463,7 @@ compile_to_object (struct command_line *cmd, const
>> char *cmd_string,
>>    char **argv;
>>    int ok;
>>    struct gdbarch *gdbarch = get_current_arch ();
>> -  char *triplet_rx;
>> +  std::string triplet_rx;
>>    char *error_message;
>>
>>    if (!target_has_execution)
>> @@ -527,15 +527,15 @@ compile_to_object (struct command_line *cmd,
>> const char *cmd_string,
>>      }
>>    else
>>      {
>> -      const char *os_rx = osabi_triplet_regexp (gdbarch_osabi
>> (gdbarch));
>> -      const char *arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);
>> +      std::string os_rx = osabi_triplet_regexp (gdbarch_osabi
>> (gdbarch));
>> +      std::string arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);
>
> Making these std::string makes unnecessary copies.
>
> You can write the line below like this:
>
>   triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + os_rx;
>
> Otherwise, LGTM.

Thanks, Simon.

Pushed to master:

7d937cad0acdccd0ff485435fbe16f005e994c66

Is it OK to push this to the 8.1 branch as well?  If so, I'm not sure I
should create a bug for this, or just go ahead and push it.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Fix warning on gdb/compile/compile.c (C++-ify "triplet_rx")

[Simon Marchi]

On 2018-01-17 18:07, Sergio Durigan Junior wrote:
> On Wednesday, January 17 2018, Simon Marchi wrote:
> 
>> On 2018-01-16 14:46, Sergio Durigan Junior wrote:
>>> This fixes a GCC warning that happens when compiling
>>> gdb/compile/compile.c on some GCC versions (e.g., "gcc (GCC) 7.2.1
>>> 20180104 (Red Hat 7.2.1-6)").
>>> 
>>> It's a simple patch that converts "triplet_rx" from "char *" to
>>> "std::string", thus guaranteeing that it will be always initialized.
>>> 
>>> I've regtested this patch and did not find any regressions.  OK to
>>> apply on both master and 8.1 (after creating a bug for it)?
>>> 
>>> gdb/ChangeLog:
>>> 2018-01-16  Sergio Durigan Junior  <sergiodj@redhat.com>
>>> 
>>> 	* compile/compile.c (compile_to_object): Convert "triplet_rx"
>>> 	to "std::string".
>>> ---
>>>  gdb/compile/compile.c | 15 ++++++++-------
>>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
>>> index 2ee75930ac..47646169c8 100644
>>> --- a/gdb/compile/compile.c
>>> +++ b/gdb/compile/compile.c
>>> @@ -463,7 +463,7 @@ compile_to_object (struct command_line *cmd, 
>>> const
>>> char *cmd_string,
>>>    char **argv;
>>>    int ok;
>>>    struct gdbarch *gdbarch = get_current_arch ();
>>> -  char *triplet_rx;
>>> +  std::string triplet_rx;
>>>    char *error_message;
>>> 
>>>    if (!target_has_execution)
>>> @@ -527,15 +527,15 @@ compile_to_object (struct command_line *cmd,
>>> const char *cmd_string,
>>>      }
>>>    else
>>>      {
>>> -      const char *os_rx = osabi_triplet_regexp (gdbarch_osabi
>>> (gdbarch));
>>> -      const char *arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);
>>> +      std::string os_rx = osabi_triplet_regexp (gdbarch_osabi
>>> (gdbarch));
>>> +      std::string arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);
>> 
>> Making these std::string makes unnecessary copies.
>> 
>> You can write the line below like this:
>> 
>>   triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + os_rx;
>> 
>> Otherwise, LGTM.
> 
> Thanks, Simon.
> 
> Pushed to master:
> 
> 7d937cad0acdccd0ff485435fbe16f005e994c66
> 
> Is it OK to push this to the 8.1 branch as well?  If so, I'm not sure I
> should create a bug for this, or just go ahead and push it.

Yes, it is fine to go to the 8.1 branch.  A PR is not necessary right 
now.  They are only necessary after there was an initial release on the 
branch, to track what fixes were made on top of that initial version.

Simon

Re: [PATCH] Fix warning on gdb/compile/compile.c (C++-ify "triplet_rx")

[Sergio Durigan Junior]

On Wednesday, January 17 2018, Simon Marchi wrote:

> On 2018-01-17 18:07, Sergio Durigan Junior wrote:
>> On Wednesday, January 17 2018, Simon Marchi wrote:
>>
>>> On 2018-01-16 14:46, Sergio Durigan Junior wrote:
>>>> This fixes a GCC warning that happens when compiling
>>>> gdb/compile/compile.c on some GCC versions (e.g., "gcc (GCC) 7.2.1
>>>> 20180104 (Red Hat 7.2.1-6)").
>>>>
>>>> It's a simple patch that converts "triplet_rx" from "char *" to
>>>> "std::string", thus guaranteeing that it will be always initialized.
>>>>
>>>> I've regtested this patch and did not find any regressions.  OK to
>>>> apply on both master and 8.1 (after creating a bug for it)?
>>>>
>>>> gdb/ChangeLog:
>>>> 2018-01-16  Sergio Durigan Junior  <sergiodj@redhat.com>
>>>>
>>>> 	* compile/compile.c (compile_to_object): Convert "triplet_rx"
>>>> 	to "std::string".
>>>> ---
>>>>  gdb/compile/compile.c | 15 ++++++++-------
>>>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
>>>> index 2ee75930ac..47646169c8 100644
>>>> --- a/gdb/compile/compile.c
>>>> +++ b/gdb/compile/compile.c
>>>> @@ -463,7 +463,7 @@ compile_to_object (struct command_line *cmd,
>>>> const
>>>> char *cmd_string,
>>>>    char **argv;
>>>>    int ok;
>>>>    struct gdbarch *gdbarch = get_current_arch ();
>>>> -  char *triplet_rx;
>>>> +  std::string triplet_rx;
>>>>    char *error_message;
>>>>
>>>>    if (!target_has_execution)
>>>> @@ -527,15 +527,15 @@ compile_to_object (struct command_line *cmd,
>>>> const char *cmd_string,
>>>>      }
>>>>    else
>>>>      {
>>>> -      const char *os_rx = osabi_triplet_regexp (gdbarch_osabi
>>>> (gdbarch));
>>>> -      const char *arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);
>>>> +      std::string os_rx = osabi_triplet_regexp (gdbarch_osabi
>>>> (gdbarch));
>>>> +      std::string arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);
>>>
>>> Making these std::string makes unnecessary copies.
>>>
>>> You can write the line below like this:
>>>
>>>   triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + os_rx;
>>>
>>> Otherwise, LGTM.
>>
>> Thanks, Simon.
>>
>> Pushed to master:
>>
>> 7d937cad0acdccd0ff485435fbe16f005e994c66
>>
>> Is it OK to push this to the 8.1 branch as well?  If so, I'm not sure I
>> should create a bug for this, or just go ahead and push it.
>
> Yes, it is fine to go to the 8.1 branch.  A PR is not necessary right
> now.  They are only necessary after there was an initial release on
> the branch, to track what fixes were made on top of that initial
> version.

Awesome.  In this case, pushed to the 8.1 branch:

2a54d2158beeb2833cb3fb4da68e7c55e341159a

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

[PATCH] Fix unitialized warning on gdb/typeprint.c:whatis_exp

[Sergio Durigan Junior]

This simple patch initializes "struct value *val" to NULL, which
silences a when compiling GDB with certain GCC versions.

This warning is technically incorrect, because there is now way that
"val" will be used unitialized if you look at the code flow, but it's
a simple "fix" and doesn't do any harm.

Is it OK to push this to master and 8.1?  I believe I will still need
to create a bug with the 8.1 milestone set, even for this simple
patch.

gdb/ChangeLog:
2017-01-16  Sergio Durigan Junior  <sergiodj@redhat.com>

	* typeprint.c (whatis_exp): Initialize "val" to NULL.
---
 gdb/typeprint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/typeprint.c b/gdb/typeprint.c
index 9a125076a1..bf9aec5436 100644
--- a/gdb/typeprint.c
+++ b/gdb/typeprint.c
@@ -405,7 +405,7 @@ error_unknown_type (const char *sym_print_name)
 static void
 whatis_exp (const char *exp, int show)
 {
-  struct value *val;
+  struct value *val = NULL;
   struct cleanup *old_chain;
   struct type *real_type = NULL;
   struct type *type;
-- 
2.14.3

Re: [PATCH] Fix unitialized warning on gdb/typeprint.c:whatis_exp

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,
> 	Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Tue, 16 Jan 2018 15:32:39 -0500
> 
> This simple patch initializes "struct value *val" to NULL, which
> silences a when compiling GDB with certain GCC versions.
> 
> This warning is technically incorrect, because there is now way that
> "val" will be used unitialized if you look at the code flow, but it's
> a simple "fix" and doesn't do any harm.

This fixes the warning for me (of course).

Thanks.

Re: [PATCH] Fix unitialized warning on gdb/typeprint.c:whatis_exp

[Pedro Alves]

On 01/16/2018 08:32 PM, Sergio Durigan Junior wrote:
> This simple patch initializes "struct value *val" to NULL, which
> silences a when compiling GDB with certain GCC versions.

Please include a representative paste of warnings in git logs.
It makes it easier to see what a patch/commit is about.

> This warning is technically incorrect, because there is now way that
> "val" will be used unitialized if you look at the code flow, but it's
> a simple "fix" and doesn't do any harm.

No, it's not incorrect.  It's showing a real bug.  Try, e.g.:

(gdb) set print object on
(gdb) whatis some_structure_type

Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
0x00000000005dda90 in check_typedef (type=0x6120736573756170) at src/gdb/gdbtypes.c:2388
2388      int instance_flags = TYPE_INSTANCE_FLAGS (type);
(top-gdb) bt
#0  0x00000000005dda90 in check_typedef(type*) (type=0x6120736573756170) at src/gdb/gdbtypes.c:2388
#1  0x00000000005e63fb in gnuv3_rtti_type(value*, int*, LONGEST*, int*) (value=0xdf21b7, full_p=0x7fffffffd180, top_p=0x7fffffffd188, using_enc_p=0x7fffffffd184) at src/gdb/gnu-v3-abi.c:293
#2  0x000000000055f7d8 in value_rtti_type(value*, int*, long*, int*) (v=0xdf21b7, full=0x7fffffffd180, top=0x7fffffffd188, using_enc=0x7fffffffd184) at src/gdb/cp-abi.c:117
#3  0x00000000006cc61f in whatis_exp(char const*, int) (exp=<optimized out>, show=-1) at src/gdb/typeprint.c:515
#4  0x0000000000472462 in cmd_func(cmd_list_element*, char const*, int) (cmd=<optimized out>, args=<optimized out>, from_tty=<optimized out>)
    at src/gdb/cli/cli-decode.c:1886
#5  0x00000000006b833a in execute_command(char const*, int) (p=<optimized out>, from_tty=1) at src/gdb/top.c:630
#6  0x00000000005c0f8c in command_handler(char const*) (command=0xdf21b0 "whatis siginfo_t") at src/gdb/event-top.c:583
#7  0x00000000005c12d8 in command_line_handler(char*) (rl=<optimized out>) at /home/pedro/gdb/mygit/src/gdb/event-top.c:774
...

Thanks,
Pedro Alves

Re: [PATCH] Fix unitialized warning on gdb/typeprint.c:whatis_exp

[Sergio Durigan Junior]

On Wednesday, January 17 2018, Pedro Alves wrote:

> On 01/16/2018 08:32 PM, Sergio Durigan Junior wrote:
>> This simple patch initializes "struct value *val" to NULL, which
>> silences a when compiling GDB with certain GCC versions.
>
> Please include a representative paste of warnings in git logs.
> It makes it easier to see what a patch/commit is about.

Will do.

>> This warning is technically incorrect, because there is now way that
>> "val" will be used unitialized if you look at the code flow, but it's
>> a simple "fix" and doesn't do any harm.
>
> No, it's not incorrect.  It's showing a real bug.  Try, e.g.:
>
> (gdb) set print object on
> (gdb) whatis some_structure_type
>
> Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
> 0x00000000005dda90 in check_typedef (type=0x6120736573756170) at src/gdb/gdbtypes.c:2388
> 2388      int instance_flags = TYPE_INSTANCE_FLAGS (type);
> (top-gdb) bt
> #0  0x00000000005dda90 in check_typedef(type*) (type=0x6120736573756170) at src/gdb/gdbtypes.c:2388
> #1 0x00000000005e63fb in gnuv3_rtti_type(value*, int*, LONGEST*, int*)
> (value=0xdf21b7, full_p=0x7fffffffd180, top_p=0x7fffffffd188,
> using_enc_p=0x7fffffffd184) at src/gdb/gnu-v3-abi.c:293
> #2  0x000000000055f7d8 in value_rtti_type(value*, int*, long*, int*) (v=0xdf21b7, full=0x7fffffffd180, top=0x7fffffffd188, using_enc=0x7fffffffd184) at src/gdb/cp-abi.c:117
> #3  0x00000000006cc61f in whatis_exp(char const*, int) (exp=<optimized out>, show=-1) at src/gdb/typeprint.c:515
> #4  0x0000000000472462 in cmd_func(cmd_list_element*, char const*, int) (cmd=<optimized out>, args=<optimized out>, from_tty=<optimized out>)
>     at src/gdb/cli/cli-decode.c:1886
> #5  0x00000000006b833a in execute_command(char const*, int) (p=<optimized out>, from_tty=1) at src/gdb/top.c:630
> #6  0x00000000005c0f8c in command_handler(char const*) (command=0xdf21b0 "whatis siginfo_t") at src/gdb/event-top.c:583
> #7  0x00000000005c12d8 in command_line_handler(char*) (rl=<optimized out>) at /home/pedro/gdb/mygit/src/gdb/event-top.c:774
> ...

Ah, I totally missed that.  Sorry about jumping into the conclusion that
it was incorrect.

I'll try to investigate what's happening here.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

[PATCH] Fix segfault when using 'set print object on' + whatis <struct>

[Sergio Durigan Junior]

This problem was hidden behind a "maybe-uninitialized" warning
generated when compiling GDB with a recent GCC.  The warning is:

  ../../gdb/typeprint.c: In function 'void whatis_exp(const char*, int)':
  ../../gdb/typeprint.c:515:12: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]
    real_type = value_rtti_type (val, &full, &top, &using_enc);
    ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I submitted a patch fixing this by initializing "val" to NULL, but it
was the wrong fix, as Pedro pointed out on
<https://sourceware.org/ml/gdb-patches/2018-01/msg00346.html>:

  (gdb) set print object on
  (gdb) whatis some_structure_type

  Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
  0x00000000005dda90 in check_typedef (type=0x6120736573756170) at src/gdb/gdbtypes.c:2388
  2388      int instance_flags = TYPE_INSTANCE_FLAGS (type);
  ...

So I set off to find the cause of the problem.  It turns out that a
recent-ish refactoring of the code on 'whatis_exp', introduced by:

  commit c973d0aa4a2c737ab527ae44a617f1c357e07364
  Date:   Mon Aug 21 11:34:32 2017 +0100

      Fix type casts losing typedefs and reimplement "whatis" typedef stripping

was the reason of the failure.  After investigating what 'set print
object on' was supposed to do to the output of 'whatis', if made sense
initialize "val = evaluate_type (expr.get ());" all the time, not only
when we're dealing with the 'ptype' command.

I've regtested this on the BuildBot, without seeing any regressions.
I've also extended 'gdb.base/whatis.exp' to check if the segfault is
not there anymore.

gdb/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* typeprint.c (whatis_exp): Move initialization of "val"
	outside of "if".

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.base/whatis.exp: Test that 'set print object on' +
	'whatis <struct>' doesn't segfault.
---
 gdb/testsuite/gdb.base/whatis.exp | 7 +++++++
 gdb/typeprint.c                   | 3 ++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/whatis.exp b/gdb/testsuite/gdb.base/whatis.exp
index dd6aeb02f9..8207dab59c 100644
--- a/gdb/testsuite/gdb.base/whatis.exp
+++ b/gdb/testsuite/gdb.base/whatis.exp
@@ -566,3 +566,10 @@ gdb_test "whatis int (*)(void, int, int)" \
 gdb_test "whatis int (*)(int, void, int)" \
     "'void' invalid as parameter type" \
     "whatis applied to function with 'void' parameter type"
+
+# Test that 'set print object on' + whatis doesn't segfault.
+clean_restart $binfile
+gdb_test_no_output "set print object on"
+gdb_test "whatis v_struct1" \
+    "type = struct t_struct" \
+    "whatis + set print object on doesn't segfault"
diff --git a/gdb/typeprint.c b/gdb/typeprint.c
index 9a125076a1..dd6e75bd4f 100644
--- a/gdb/typeprint.c
+++ b/gdb/typeprint.c
@@ -471,6 +471,8 @@ whatis_exp (const char *exp, int show)
 
       expression_up expr = parse_expression (exp);
 
+      val = evaluate_type (expr.get ());
+
       /* The behavior of "whatis" depends on whether the user
 	 expression names a type directly, or a language expression
 	 (including variable names).  If the former, then "whatis"
@@ -495,7 +497,6 @@ whatis_exp (const char *exp, int show)
 	  /* The user expression names a type indirectly by naming an
 	     object or expression of that type.  Find that
 	     indirectly-named type.  */
-	  val = evaluate_type (expr.get ());
 	  type = value_type (val);
 	}
     }
-- 
2.14.3

[PATCH v2] Fix segfault when using 'set print object on' + whatis <struct> (Re: [PATCH] Fix segfault when using 'set print object on' + whatis <struct>)

[Pedro Alves]

Hi Sergio,

On 01/20/2018 01:03 AM, Sergio Durigan Junior wrote:
> This problem was hidden behind a "maybe-uninitialized" warning
> generated when compiling GDB with a recent GCC.  The warning is:
> 
>   ../../gdb/typeprint.c: In function 'void whatis_exp(const char*, int)':
>   ../../gdb/typeprint.c:515:12: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]
>     real_type = value_rtti_type (val, &full, &top, &using_enc);
>     ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> I submitted a patch fixing this by initializing "val" to NULL, but it
> was the wrong fix, as Pedro pointed out on
> <https://sourceware.org/ml/gdb-patches/2018-01/msg00346.html>:

IMHO, adding such history to the commit log directly doesn't really
add much here.  It's better IMO to leave that to "what changed in v2"
comments (that don't go in the commit log), and instead update
the commit log to go straight to the point.  (please keep reading.)

> 
>   (gdb) set print object on
>   (gdb) whatis some_structure_type
> 
>   Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
>   0x00000000005dda90 in check_typedef (type=0x6120736573756170) at src/gdb/gdbtypes.c:2388
>   2388      int instance_flags = TYPE_INSTANCE_FLAGS (type);
>   ...
> 
> So I set off to find the cause of the problem.  It turns out that a
> recent-ish refactoring of the code on 'whatis_exp', introduced by:
> 
>   commit c973d0aa4a2c737ab527ae44a617f1c357e07364
>   Date:   Mon Aug 21 11:34:32 2017 +0100
> 
>       Fix type casts losing typedefs and reimplement "whatis" typedef stripping
> 
> was the reason of the failure.  After investigating what 'set print
> object on' was supposed to do to the output of 'whatis', if made sense
> initialize "val = evaluate_type (expr.get ());" all the time, not only
> when we're dealing with the 'ptype' command.

The point of the c973d0aa4a2c change was to bypass evaluating the
expression if the opcode is OP_TYPE.  The proposed patch adds back
the evaluation, while the OP_TYPE shortcut is left in there too.  I
think calling allocate_value instead in the path that misses
initializing 'val' would make more sense, to end up with a dummy
not_lval value, like expression evaluation does when evaluating
EVAL_AVOID_SIDE_EFFECTS.  But even better still is to set VAL to NULL
in this case (only), and skip the "real type" printing.  It doesn't make
any sense to try to fetch the dynamic type of a type that didn't come
from an actual program value in the first place.  There's nothing
dynamic about a statically named type.

> 
> I've regtested this on the BuildBot, without seeing any regressions.
> I've also extended 'gdb.base/whatis.exp' to check if the segfault is
> not there anymore.

The "whatis struct" case was an example, but there's another similar
path in the code that also lead to a crash that is not covered by the
testcase.  The value_rtti_indirect_type path here:

  if (opts.objectprint)
    {
      if (((TYPE_CODE (type) == TYPE_CODE_PTR) || TYPE_IS_REFERENCE (type))
	  && (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_STRUCT))
        real_type = value_rtti_indirect_type (val, &full, &top, &using_enc);
      else if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
	real_type = value_rtti_type (val, &full, &top, &using_enc);
    }

> --- a/gdb/testsuite/gdb.base/whatis.exp
> +++ b/gdb/testsuite/gdb.base/whatis.exp
> @@ -566,3 +566,10 @@ gdb_test "whatis int (*)(void, int, int)" \
>  gdb_test "whatis int (*)(int, void, int)" \
>      "'void' invalid as parameter type" \
>      "whatis applied to function with 'void' parameter type"
> +
> +# Test that 'set print object on' + whatis doesn't segfault.
> +clean_restart $binfile
> +gdb_test_no_output "set print object on"
> +gdb_test "whatis v_struct1" \
> +    "type = struct t_struct" \
> +    "whatis + set print object on doesn't segfault"

At some point later on someone is going to read this test
name/message and wonder why is it talking about a segfault.
All the other 50k+ tests in the testsuite are indirectly checking
that gdb doesn't segfault either.  This one's not that special
in that regard.  Instead, we're testing that the
"set print object on" + "whatis <struct>" combination _works_ as
expected, which among other things obviously includes
not segfaulting, but also includes checking that the output
is reasonable.

As I was writing/experimenting the above, I ended up addressing
my own comments.  What do you think of this patch instead?

New in v2:

- set val to NULL and skip dynamic type printing in the "whatis <type>"
  case.
- rewrite the new tests to reuse preexisting tests, and add new tests
  for whatis with pointers and references to structs coupled with
  "set print object on".  Try both "set print object on/off".

From f86b4087d23be4cf20bf3dc2f8407cef3f28830d Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Mon, 22 Jan 2018 17:33:13 +0000
Subject: [PATCH] Fix segfault when using 'set print object on' + whatis
 <struct>

Compiling GDB with a recent GCC exposes a problem:

  ../../gdb/typeprint.c: In function 'void whatis_exp(const char*, int)':
  ../../gdb/typeprint.c:515:12: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]
    real_type = value_rtti_type (val, &full, &top, &using_enc);
    ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The warning is correct.  There are indeed code paths that use
uninitialized 'val', leading to crashes.  Inside the
value_rtti_indirect_type/value_rtti_type calls here in whatis_exp:

  if (opts.objectprint)
    {
      if (((TYPE_CODE (type) == TYPE_CODE_PTR) || TYPE_IS_REFERENCE (type))
	  && (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_STRUCT))
        real_type = value_rtti_indirect_type (val, &full, &top, &using_enc);
      else if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
	real_type = value_rtti_type (val, &full, &top, &using_enc);
    }

We reach those calls above with "set print object on", and then with
any of:

  (gdb) whatis struct some_structure_type
  (gdb) whatis struct some_structure_type *
  (gdb) whatis struct some_structure_type &

because "whatis" with a type argument enters this branch:

      /* The behavior of "whatis" depends on whether the user
	 expression names a type directly, or a language expression
	 (including variable names).  If the former, then "whatis"
	 strips one level of typedefs, only.  If an expression,
	 "whatis" prints the type of the expression without stripping
	 any typedef level.  "ptype" always strips all levels of
	 typedefs.  */
      if (show == -1 && expr->elts[0].opcode == OP_TYPE)
	{

which does not initialize VAL.  Trying the above lead to crashes like
this:

  (gdb) set print object on
  (gdb) whatis some_structure_type

  Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
  0x00000000005dda90 in check_typedef (type=0x6120736573756170) at src/gdb/gdbtypes.c:2388
  2388      int instance_flags = TYPE_INSTANCE_FLAGS (type);
  ...

This is a regression caused by a recent-ish refactoring of the code on
'whatis_exp', introduced by:

  commit c973d0aa4a2c737ab527ae44a617f1c357e07364
  Date:   Mon Aug 21 11:34:32 2017 +0100

      Fix type casts losing typedefs and reimplement "whatis" typedef stripping

Fix this by setting VAL to NULL in the "whatis TYPE" case, and
skipping fetching the dynamic type if there's no value to fetch it
from.

New tests included.

gdb/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	* typeprint.c (whatis_exp): Initialize "val" in the "whatis type"
	case.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	* gdb.base/whatis.exp: Add tests for 'set print object on' +
	'whatis <struct>' 'whatis <struct> *' and 'whatis <struct> &'.
---
 gdb/testsuite/gdb.base/whatis.exp | 25 +++++++++++++++++++++----
 gdb/typeprint.c                   |  6 +++++-
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.base/whatis.exp b/gdb/testsuite/gdb.base/whatis.exp
index dd6aeb02f91..509183e2ea4 100644
--- a/gdb/testsuite/gdb.base/whatis.exp
+++ b/gdb/testsuite/gdb.base/whatis.exp
@@ -282,14 +282,31 @@ gdb_test "whatis v_double_pointer" \
 
 
 # test whatis command with structure types
+
+# First with a type argument, with both "set print object" set to "on"
+# and "off", ending with "off" for the following tests.
+foreach_with_prefix print_object {"on" "off"} {
+    gdb_test_no_output "set print object $print_object"
+
+    gdb_test "whatis struct t_struct" \
+	"type = struct t_struct" \
+	"whatis named structure using type name"
+
+    gdb_test "whatis struct t_struct *" \
+	"type = struct t_struct \\*" \
+	"whatis named structure using type name and pointer"
+
+    gdb_test "whatis struct t_struct &" \
+	"type = struct t_struct &" \
+	"whatis named structure using type name and reference"
+}
+
+# Now with an expression argument.
+
 gdb_test "whatis v_struct1" \
     "type = struct t_struct" \
     "whatis named structure"
 
-gdb_test "whatis struct t_struct" \
-    "type = struct t_struct" \
-    "whatis named structure using type name"
-
 gdb_test "whatis v_struct2" \
     "type = struct \{\.\.\.\}" \
     "whatis unnamed structure"
diff --git a/gdb/typeprint.c b/gdb/typeprint.c
index 9a125076a1b..c098a3f4261 100644
--- a/gdb/typeprint.c
+++ b/gdb/typeprint.c
@@ -489,6 +489,10 @@ whatis_exp (const char *exp, int show)
 	  check_typedef (type);
 	  if (TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
 	    type = TYPE_TARGET_TYPE (type);
+
+	  /* If the expression is actually a type, then there's no
+	     value to fetch the dynamic type from.  */
+	  val = NULL;
 	}
       else
 	{
@@ -506,7 +510,7 @@ whatis_exp (const char *exp, int show)
     }
 
   get_user_print_options (&opts);
-  if (opts.objectprint)
+  if (val != NULL && opts.objectprint)
     {
       if (((TYPE_CODE (type) == TYPE_CODE_PTR) || TYPE_IS_REFERENCE (type))
 	  && (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_STRUCT))
-- 
2.14.3

Re: [PATCH v2] Fix segfault when using 'set print object on' + whatis <struct> (Re: [PATCH] Fix segfault when using 'set print object on' + whatis <struct>)

[Sergio Durigan Junior]

On Monday, January 22 2018, Pedro Alves wrote:

> Hi Sergio,

Hey,

/me prepares himself for the "this is completely wrong" e-mail

> On 01/20/2018 01:03 AM, Sergio Durigan Junior wrote:
>> This problem was hidden behind a "maybe-uninitialized" warning
>> generated when compiling GDB with a recent GCC.  The warning is:
>> 
>>   ../../gdb/typeprint.c: In function 'void whatis_exp(const char*, int)':
>>   ../../gdb/typeprint.c:515:12: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]
>>     real_type = value_rtti_type (val, &full, &top, &using_enc);
>>     ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 
>> I submitted a patch fixing this by initializing "val" to NULL, but it
>> was the wrong fix, as Pedro pointed out on
>> <https://sourceware.org/ml/gdb-patches/2018-01/msg00346.html>:
>
> IMHO, adding such history to the commit log directly doesn't really
> add much here.  It's better IMO to leave that to "what changed in v2"
> comments (that don't go in the commit log), and instead update
> the commit log to go straight to the point.  (please keep reading.)

OK.  I personally think that it's good to document the actions and
reactions that led me to writing this patch, but this is a small detail.

>> 
>>   (gdb) set print object on
>>   (gdb) whatis some_structure_type
>> 
>>   Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
>>   0x00000000005dda90 in check_typedef (type=0x6120736573756170) at src/gdb/gdbtypes.c:2388
>>   2388      int instance_flags = TYPE_INSTANCE_FLAGS (type);
>>   ...
>> 
>> So I set off to find the cause of the problem.  It turns out that a
>> recent-ish refactoring of the code on 'whatis_exp', introduced by:
>> 
>>   commit c973d0aa4a2c737ab527ae44a617f1c357e07364
>>   Date:   Mon Aug 21 11:34:32 2017 +0100
>> 
>>       Fix type casts losing typedefs and reimplement "whatis" typedef stripping
>> 
>> was the reason of the failure.  After investigating what 'set print
>> object on' was supposed to do to the output of 'whatis', if made sense
>> initialize "val = evaluate_type (expr.get ());" all the time, not only
>> when we're dealing with the 'ptype' command.
>
> The point of the c973d0aa4a2c change was to bypass evaluating the
> expression if the opcode is OP_TYPE.  The proposed patch adds back
> the evaluation, while the OP_TYPE shortcut is left in there too.  I
> think calling allocate_value instead in the path that misses
> initializing 'val' would make more sense, to end up with a dummy
> not_lval value, like expression evaluation does when evaluating
> EVAL_AVOID_SIDE_EFFECTS.  But even better still is to set VAL to NULL
> in this case (only), and skip the "real type" printing.  It doesn't make
> any sense to try to fetch the dynamic type of a type that didn't come
> from an actual program value in the first place.  There's nothing
> dynamic about a statically named type.

Ah, OK.  Thanks for explaining.  I wasn't really sure about the right
fix here, and somehow I felt like I was missing more context to make an
informed decision on what the right fix was.  That's why I asked
(internally) what "whatis" is supposed to print when 'set print object'
is on.  Anyway, now it makes more sense.

>> 
>> I've regtested this on the BuildBot, without seeing any regressions.
>> I've also extended 'gdb.base/whatis.exp' to check if the segfault is
>> not there anymore.
>
> The "whatis struct" case was an example, but there's another similar
> path in the code that also lead to a crash that is not covered by the
> testcase.  The value_rtti_indirect_type path here:
>
>   if (opts.objectprint)
>     {
>       if (((TYPE_CODE (type) == TYPE_CODE_PTR) || TYPE_IS_REFERENCE (type))
> 	  && (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_STRUCT))
>         real_type = value_rtti_indirect_type (val, &full, &top, &using_enc);
>       else if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
> 	real_type = value_rtti_type (val, &full, &top, &using_enc);
>     }

Right, I should have added one more test to cover this case as well,
sorry.

>> --- a/gdb/testsuite/gdb.base/whatis.exp
>> +++ b/gdb/testsuite/gdb.base/whatis.exp
>> @@ -566,3 +566,10 @@ gdb_test "whatis int (*)(void, int, int)" \
>>  gdb_test "whatis int (*)(int, void, int)" \
>>      "'void' invalid as parameter type" \
>>      "whatis applied to function with 'void' parameter type"
>> +
>> +# Test that 'set print object on' + whatis doesn't segfault.
>> +clean_restart $binfile
>> +gdb_test_no_output "set print object on"
>> +gdb_test "whatis v_struct1" \
>> +    "type = struct t_struct" \
>> +    "whatis + set print object on doesn't segfault"
>
> At some point later on someone is going to read this test
> name/message and wonder why is it talking about a segfault.
> All the other 50k+ tests in the testsuite are indirectly checking
> that gdb doesn't segfault either.  This one's not that special
> in that regard.  Instead, we're testing that the
> "set print object on" + "whatis <struct>" combination _works_ as
> expected, which among other things obviously includes
> not segfaulting, but also includes checking that the output
> is reasonable.
>
> As I was writing/experimenting the above, I ended up addressing
> my own comments.  What do you think of this patch instead?

The patch is yours now, so you should be listed as the author, and you
will probably want to rewrite the subject line to make it more correct.
Other than that, and after your e-mail, I don't really have the
expertise to criticize anything.

Thanks for explaining things and taking care of v2.

> New in v2:
>
> - set val to NULL and skip dynamic type printing in the "whatis <type>"
>   case.
> - rewrite the new tests to reuse preexisting tests, and add new tests
>   for whatis with pointers and references to structs coupled with
>   "set print object on".  Try both "set print object on/off".
>
> From f86b4087d23be4cf20bf3dc2f8407cef3f28830d Mon Sep 17 00:00:00 2001
> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Mon, 22 Jan 2018 17:33:13 +0000
> Subject: [PATCH] Fix segfault when using 'set print object on' + whatis
>  <struct>
>
> Compiling GDB with a recent GCC exposes a problem:
>
>   ../../gdb/typeprint.c: In function 'void whatis_exp(const char*, int)':
>   ../../gdb/typeprint.c:515:12: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]
>     real_type = value_rtti_type (val, &full, &top, &using_enc);
>     ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> The warning is correct.  There are indeed code paths that use
> uninitialized 'val', leading to crashes.  Inside the
> value_rtti_indirect_type/value_rtti_type calls here in whatis_exp:
>
>   if (opts.objectprint)
>     {
>       if (((TYPE_CODE (type) == TYPE_CODE_PTR) || TYPE_IS_REFERENCE (type))
> 	  && (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_STRUCT))
>         real_type = value_rtti_indirect_type (val, &full, &top, &using_enc);
>       else if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
> 	real_type = value_rtti_type (val, &full, &top, &using_enc);
>     }
>
> We reach those calls above with "set print object on", and then with
> any of:
>
>   (gdb) whatis struct some_structure_type
>   (gdb) whatis struct some_structure_type *
>   (gdb) whatis struct some_structure_type &
>
> because "whatis" with a type argument enters this branch:
>
>       /* The behavior of "whatis" depends on whether the user
> 	 expression names a type directly, or a language expression
> 	 (including variable names).  If the former, then "whatis"
> 	 strips one level of typedefs, only.  If an expression,
> 	 "whatis" prints the type of the expression without stripping
> 	 any typedef level.  "ptype" always strips all levels of
> 	 typedefs.  */
>       if (show == -1 && expr->elts[0].opcode == OP_TYPE)
> 	{
>
> which does not initialize VAL.  Trying the above lead to crashes like
> this:
>
>   (gdb) set print object on
>   (gdb) whatis some_structure_type
>
>   Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
>   0x00000000005dda90 in check_typedef (type=0x6120736573756170) at src/gdb/gdbtypes.c:2388
>   2388      int instance_flags = TYPE_INSTANCE_FLAGS (type);
>   ...
>
> This is a regression caused by a recent-ish refactoring of the code on
> 'whatis_exp', introduced by:
>
>   commit c973d0aa4a2c737ab527ae44a617f1c357e07364
>   Date:   Mon Aug 21 11:34:32 2017 +0100
>
>       Fix type casts losing typedefs and reimplement "whatis" typedef stripping
>
> Fix this by setting VAL to NULL in the "whatis TYPE" case, and
> skipping fetching the dynamic type if there's no value to fetch it
> from.
>
> New tests included.
>
> gdb/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
> 	    Pedro Alves  <palves@redhat.com>
>
> 	* typeprint.c (whatis_exp): Initialize "val" in the "whatis type"
> 	case.
>
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
> 	    Pedro Alves  <palves@redhat.com>
>
> 	* gdb.base/whatis.exp: Add tests for 'set print object on' +
> 	'whatis <struct>' 'whatis <struct> *' and 'whatis <struct> &'.
> ---
>  gdb/testsuite/gdb.base/whatis.exp | 25 +++++++++++++++++++++----
>  gdb/typeprint.c                   |  6 +++++-
>  2 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/whatis.exp b/gdb/testsuite/gdb.base/whatis.exp
> index dd6aeb02f91..509183e2ea4 100644
> --- a/gdb/testsuite/gdb.base/whatis.exp
> +++ b/gdb/testsuite/gdb.base/whatis.exp
> @@ -282,14 +282,31 @@ gdb_test "whatis v_double_pointer" \
>  
>  
>  # test whatis command with structure types
> +
> +# First with a type argument, with both "set print object" set to "on"
> +# and "off", ending with "off" for the following tests.
> +foreach_with_prefix print_object {"on" "off"} {
> +    gdb_test_no_output "set print object $print_object"
> +
> +    gdb_test "whatis struct t_struct" \
> +	"type = struct t_struct" \
> +	"whatis named structure using type name"
> +
> +    gdb_test "whatis struct t_struct *" \
> +	"type = struct t_struct \\*" \
> +	"whatis named structure using type name and pointer"
> +
> +    gdb_test "whatis struct t_struct &" \
> +	"type = struct t_struct &" \
> +	"whatis named structure using type name and reference"
> +}
> +
> +# Now with an expression argument.
> +
>  gdb_test "whatis v_struct1" \
>      "type = struct t_struct" \
>      "whatis named structure"
>  
> -gdb_test "whatis struct t_struct" \
> -    "type = struct t_struct" \
> -    "whatis named structure using type name"
> -
>  gdb_test "whatis v_struct2" \
>      "type = struct \{\.\.\.\}" \
>      "whatis unnamed structure"
> diff --git a/gdb/typeprint.c b/gdb/typeprint.c
> index 9a125076a1b..c098a3f4261 100644
> --- a/gdb/typeprint.c
> +++ b/gdb/typeprint.c
> @@ -489,6 +489,10 @@ whatis_exp (const char *exp, int show)
>  	  check_typedef (type);
>  	  if (TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
>  	    type = TYPE_TARGET_TYPE (type);
> +
> +	  /* If the expression is actually a type, then there's no
> +	     value to fetch the dynamic type from.  */
> +	  val = NULL;
>  	}
>        else
>  	{
> @@ -506,7 +510,7 @@ whatis_exp (const char *exp, int show)
>      }
>  
>    get_user_print_options (&opts);
> -  if (opts.objectprint)
> +  if (val != NULL && opts.objectprint)
>      {
>        if (((TYPE_CODE (type) == TYPE_CODE_PTR) || TYPE_IS_REFERENCE (type))
>  	  && (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_STRUCT))
> -- 
> 2.14.3

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH v2] Fix segfault when using 'set print object on' + whatis <struct> (Re: [PATCH] Fix segfault when using 'set print object on' + whatis <struct>)

[Pedro Alves]

On 01/22/2018 06:04 PM, Sergio Durigan Junior wrote:
> On Monday, January 22 2018, Pedro Alves wrote:
> 
> /me prepares himself for the "this is completely wrong" e-mail
> 

:-)

>> On 01/20/2018 01:03 AM, Sergio Durigan Junior wrote:
>>> This problem was hidden behind a "maybe-uninitialized" warning
>>> generated when compiling GDB with a recent GCC.  The warning is:
>>>
>>>   ../../gdb/typeprint.c: In function 'void whatis_exp(const char*, int)':
>>>   ../../gdb/typeprint.c:515:12: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]
>>>     real_type = value_rtti_type (val, &full, &top, &using_enc);
>>>     ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> I submitted a patch fixing this by initializing "val" to NULL, but it
>>> was the wrong fix, as Pedro pointed out on
>>> <https://sourceware.org/ml/gdb-patches/2018-01/msg00346.html>:
>>
>> IMHO, adding such history to the commit log directly doesn't really
>> add much here.  It's better IMO to leave that to "what changed in v2"
>> comments (that don't go in the commit log), and instead update
>> the commit log to go straight to the point.  (please keep reading.)
> 
> OK.  I personally think that it's good to document the actions and
> reactions that led me to writing this patch, but this is a small detail.

It's good to describe approaches that are
seemingly-obvious-but-then-don't-actually-work.  That spares the
reader/reviewer from going through the same thought process, and/or
immediately shooting back with a "why didn't you do it in the other
seemingly obvious way" question.   In this case, just explaining that
the warning is valid obviously switches the previous approach to
the "really-obviously-not-correct/incomplete" department, so there's
no need to explain it.  Commit messages are meant to help future
readers understand why changes were made, and being concise and to
the point helps.  The thing that led to the discovery of the issue
is the GCC warning, and that is still mentioned.

> Right, I should have added one more test to cover this case as well,
> sorry.

No worries.  This is team work.  :-)

>>
>> As I was writing/experimenting the above, I ended up addressing
>> my own comments.  What do you think of this patch instead?
> 
> The patch is yours now, so you should be listed as the author, and you
> will probably want to rewrite the subject line to make it more correct.
> Other than that, and after your e-mail, I don't really have the
> expertise to criticize anything.

I don't mind either way, but I've pushed the patch in respecting that.

And now I realize that this should be needed on the 8.1 branch too.
I'll take care of that.

Thanks,
Pedro Alves

Re: [PATCH v2] Fix segfault when using 'set print object on' + whatis <struct> (Re: [PATCH] Fix segfault when using 'set print object on' + whatis <struct>)

[Sergio Durigan Junior]

On Monday, January 22 2018, Pedro Alves wrote:

>>> On 01/20/2018 01:03 AM, Sergio Durigan Junior wrote:
>>>> This problem was hidden behind a "maybe-uninitialized" warning
>>>> generated when compiling GDB with a recent GCC.  The warning is:
>>>>
>>>>   ../../gdb/typeprint.c: In function 'void whatis_exp(const char*, int)':
>>>>   ../../gdb/typeprint.c:515:12: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]
>>>>     real_type = value_rtti_type (val, &full, &top, &using_enc);
>>>>     ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> I submitted a patch fixing this by initializing "val" to NULL, but it
>>>> was the wrong fix, as Pedro pointed out on
>>>> <https://sourceware.org/ml/gdb-patches/2018-01/msg00346.html>:
>>>
>>> IMHO, adding such history to the commit log directly doesn't really
>>> add much here.  It's better IMO to leave that to "what changed in v2"
>>> comments (that don't go in the commit log), and instead update
>>> the commit log to go straight to the point.  (please keep reading.)
>> 
>> OK.  I personally think that it's good to document the actions and
>> reactions that led me to writing this patch, but this is a small detail.
>
> It's good to describe approaches that are
> seemingly-obvious-but-then-don't-actually-work.  That spares the
> reader/reviewer from going through the same thought process, and/or
> immediately shooting back with a "why didn't you do it in the other
> seemingly obvious way" question.   In this case, just explaining that
> the warning is valid obviously switches the previous approach to
> the "really-obviously-not-correct/incomplete" department, so there's
> no need to explain it.  Commit messages are meant to help future
> readers understand why changes were made, and being concise and to
> the point helps.  The thing that led to the discovery of the issue
> is the GCC warning, and that is still mentioned.

I understand your point.  To the experienced reader, the final commit
message (written by you) is indeed better because it removes the
previous attempt I made to fix this issue by just initializing VAL to
NULL.  Since I was the one who made the mistake of not fully
understanding the consequences of the warning, I thought it made sense
to explain everything, from the very beginning, when I thought this was
just a silly warning) to the point where you pointed me to the real bug.
While this commit message made sense to me, and can probably be
considered "too much" for the experienced reader, I still think it would
be a nice addition.  But I don't want to create a confusion/mess/issue
out of this specific part, so I'm totally fine with your final message.

>>> As I was writing/experimenting the above, I ended up addressing
>>> my own comments.  What do you think of this patch instead?
>> 
>> The patch is yours now, so you should be listed as the author, and you
>> will probably want to rewrite the subject line to make it more correct.
>> Other than that, and after your e-mail, I don't really have the
>> expertise to criticize anything.
>
> I don't mind either way, but I've pushed the patch in respecting that.
>
> And now I realize that this should be needed on the 8.1 branch too.
> I'll take care of that.

Yes, thanks for doing, I was going to send a message reminding you about
the 8.1 branch.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Sergio Durigan Junior]

On Tuesday, January 16 2018, I wrote:

>>      g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -c -o infrun.o -MT infrun.o -MMD -MP  -MF ./.deps/infrun.Tpo infrun.c
>>      In file included from infrun.c:26:0:
>>      inferior.h: In function 'void handle_vfork_child_exec_or_exit(int)':
>>      inferior.h:537:39: warning: '*((void*)(& maybe_restore_inferior)+20).scoped_restore_current_inferior::m_saved_inf' may be used uninitialized in this function [-Wmaybe-uninitialized]
>> 	{ set_current_inferior (m_saved_inf); }
>> 					    ^
>>      infrun.c:927:6: note: '*((void*)(& maybe_restore_inferior)+20).scoped_restore_current_inferior::m_saved_inf' was declared here
>> 	   maybe_restore_inferior;
>> 	   ^~~~~~~~~~~~~~~~~~~~~~
>>      In file included from inferior.h:48:0,
>> 		      from infrun.c:26:
>>      progspace.h:285:47: warning: '*((void*)(& maybe_restore_inferior)+16).scoped_restore_current_program_space::m_saved_pspace' may be used uninitialized in this function [-Wmaybe-uninitialized]
>> 	{ set_current_program_space (m_saved_pspace); }
>> 						    ^
>>      infrun.c:927:6: note: '*((void*)(& maybe_restore_inferior)+16).scoped_restore_current_program_space::m_saved_pspace' was declared here
>> 	   maybe_restore_inferior;
>> 	   ^~~~~~~~~~~~~~~~~~~~~~

I'm not sure why this one is happening.  I think it has something to do
with the fact that we're declaring "maybe_restore_inferior" as
gdb::optional, because scoped_restore_current_inferior's constructor
already takes care of initializing "m_saved_inf" (same goes for
scoped_restore_current_program_space).

If that is the case, then maybe we can mark the "uninitialized variable"
warning for "m_saved_inf" and "m_saved_pspace".

WDYT?

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Tue, 16 Jan 2018 15:36:22 -0500
> 
> I'm not sure why this one is happening.  I think it has something to do
> with the fact that we're declaring "maybe_restore_inferior" as
> gdb::optional, because scoped_restore_current_inferior's constructor
> already takes care of initializing "m_saved_inf" (same goes for
> scoped_restore_current_program_space).
> 
> If that is the case, then maybe we can mark the "uninitialized variable"
> warning for "m_saved_inf" and "m_saved_pspace".

Sorry, I'm not sure I understand what does your suggestion here mean
in practice.  Can you elaborate, or show a proposed patch?

Thanks.

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Sergio Durigan Junior]

On Tuesday, January 16 2018, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: gdb-patches@sourceware.org
>> Date: Tue, 16 Jan 2018 15:36:22 -0500
>> 
>> I'm not sure why this one is happening.  I think it has something to do
>> with the fact that we're declaring "maybe_restore_inferior" as
>> gdb::optional, because scoped_restore_current_inferior's constructor
>> already takes care of initializing "m_saved_inf" (same goes for
>> scoped_restore_current_program_space).
>> 
>> If that is the case, then maybe we can mark the "uninitialized variable"
>> warning for "m_saved_inf" and "m_saved_pspace".
>
> Sorry, I'm not sure I understand what does your suggestion here mean
> in practice.  Can you elaborate, or show a proposed patch?

Eli, with Pedro's reply, mentioning:

  https://sourceware.org/ml/gdb-patches/2017-05/msg00130.html

I think there's not much we can do about the warning.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Pedro Alves]

On 01/16/2018 08:36 PM, Sergio Durigan Junior wrote:

> I'm not sure why this one is happening.  I think it has something to do
> with the fact that we're declaring "maybe_restore_inferior" as
> gdb::optional, because scoped_restore_current_inferior's constructor
> already takes care of initializing "m_saved_inf" (same goes for
> scoped_restore_current_program_space).

https://sourceware.org/ml/gdb-patches/2017-05/msg00130.html

Thanks,
Pedro Alves

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Sergio Durigan Junior]

On Wednesday, January 17 2018, Pedro Alves wrote:

> On 01/16/2018 08:36 PM, Sergio Durigan Junior wrote:
>
>> I'm not sure why this one is happening.  I think it has something to do
>> with the fact that we're declaring "maybe_restore_inferior" as
>> gdb::optional, because scoped_restore_current_inferior's constructor
>> already takes care of initializing "m_saved_inf" (same goes for
>> scoped_restore_current_program_space).
>
> https://sourceware.org/ml/gdb-patches/2017-05/msg00130.html

Ah, there you go.  Thanks, Pedro.  I had missed that exchange of
messages.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  gdb-patches@sourceware.org
> Date: Wed, 17 Jan 2018 11:38:08 -0500
> 
> > https://sourceware.org/ml/gdb-patches/2017-05/msg00130.html
> 
> Ah, there you go.  Thanks, Pedro.  I had missed that exchange of
> messages.

How about using GCC pragma to shut up this warning in that particular
place?

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Pedro Alves]

On 01/17/2018 04:46 PM, Eli Zaretskii wrote:
>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  gdb-patches@sourceware.org
>> Date: Wed, 17 Jan 2018 11:38:08 -0500
>>
>>> https://sourceware.org/ml/gdb-patches/2017-05/msg00130.html
>>
>> Ah, there you go.  Thanks, Pedro.  I had missed that exchange of
>> messages.
> 
> How about using GCC pragma to shut up this warning in that particular
> place?
IIRC, last I tried, it make the code exceedingly ugly.  See:

 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635#c9

Thanks,
Pedro Alves

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Wed, 17 Jan 2018 16:49:58 +0000
> 
> > How about using GCC pragma to shut up this warning in that particular
> > place?
> IIRC, last I tried, it make the code exceedingly ugly.  See:
> 
>  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635#c9

It doesn't look too ugly to me, FWIW.  Its advantage is that it
documents exactly what is the problem being worked around here, and
makes finding the affected places easy.  By contrast, leaving the
warnings around and keeping the information about the problem in some
mailing list posting makes its discoverability much lower, and the
probability that someone else will spend time trying to understand it
higher.  And the warning is quite cryptic, at least to my naïve eyes.

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Eli Zaretskii]

Ping!  Is it OK to install the patch proposed below?

> Date: Tue, 16 Jan 2018 19:31:37 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
>      g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -fno-strict-aliasing -DNDEBUG -fwrapv -c -o python/py-arch.o -MT python/py-arch.o -MMD -MP  -MF python/.deps/py-arch.Tpo python/py-arch.c
>      In file included from d:\usr\lib\gcc\mingw32\6.3.0\include\c++\math.h:36:0,
> 		      from build-gnulib/import/math.h:27,
> 		      from d:/usr/Python26/include/pyport.h:235,
> 		      from d:/usr/Python26/include/Python.h:58,
> 		      from python/python-internal.h:94,
> 		      from python/py-arch.c:24:
>      d:\usr\lib\gcc\mingw32\6.3.0\include\c++\cmath:1157:11: error: '::hypot' has not been declared
> 	using ::hypot;
> 		^~~~~
>      Makefile:1618: recipe for target `python/py-arch.o' failed
> 
> Googling suggests the following solution; is it okay to push this
> (with the necessary logs and after doing the "paperwork" required for
> branch changes)?  Or does someone have better ideas?  (Does this work
> in MinGW64?)
> 
> --- gdb/python/python-internal.h~0	2018-01-12 05:31:04.000000000 +0200
> +++ gdb/python/python-internal.h	2018-01-16 08:56:10.717759900 +0200
> @@ -85,6 +85,12 @@
>  #define HAVE_SNPRINTF 1
>  #endif
>  
> +/* Another kludge to avoid compilation errors because MinGW defines
> +   'hypot' to '_hypot', but the C++ headers says "using ::hypot".  */
> +#if defined(__MINGW32__) && defined(__cplusplus)
> +# define _hypot hypot
> +#endif
> +
>  /* Request clean size types from Python.  */
>  #define PY_SSIZE_T_CLEAN
>  
> 

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Eli Zaretskii]

> Date: Thu, 18 Jan 2018 17:52:53 +0200
> From: Eli Zaretskii <eliz@gnu.org>

Ping!  Ping!  OK to push this?

> Ping!  Is it OK to install the patch proposed below?
> 
> > Date: Tue, 16 Jan 2018 19:31:37 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > 
> >      g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -fno-strict-aliasing -DNDEBUG -fwrapv -c -o python/py-arch.o -MT python/py-arch.o -MMD -MP  -MF python/.deps/py-arch.Tpo python/py-arch.c
> >      In file included from d:\usr\lib\gcc\mingw32\6.3.0\include\c++\math.h:36:0,
> > 		      from build-gnulib/import/math.h:27,
> > 		      from d:/usr/Python26/include/pyport.h:235,
> > 		      from d:/usr/Python26/include/Python.h:58,
> > 		      from python/python-internal.h:94,
> > 		      from python/py-arch.c:24:
> >      d:\usr\lib\gcc\mingw32\6.3.0\include\c++\cmath:1157:11: error: '::hypot' has not been declared
> > 	using ::hypot;
> > 		^~~~~
> >      Makefile:1618: recipe for target `python/py-arch.o' failed
> > 
> > Googling suggests the following solution; is it okay to push this
> > (with the necessary logs and after doing the "paperwork" required for
> > branch changes)?  Or does someone have better ideas?  (Does this work
> > in MinGW64?)
> > 
> > --- gdb/python/python-internal.h~0	2018-01-12 05:31:04.000000000 +0200
> > +++ gdb/python/python-internal.h	2018-01-16 08:56:10.717759900 +0200
> > @@ -85,6 +85,12 @@
> >  #define HAVE_SNPRINTF 1
> >  #endif
> >  
> > +/* Another kludge to avoid compilation errors because MinGW defines
> > +   'hypot' to '_hypot', but the C++ headers says "using ::hypot".  */
> > +#if defined(__MINGW32__) && defined(__cplusplus)
> > +# define _hypot hypot
> > +#endif
> > +
> >  /* Request clean size types from Python.  */
> >  #define PY_SSIZE_T_CLEAN
> >  
> > 
> 

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Eli Zaretskii]

Ping! Ping! Ping!  This is delaying the release of GDB 8.1.

Should I just stop waiting for approval and push this?

> Date: Thu, 25 Jan 2018 18:58:30 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > Date: Thu, 18 Jan 2018 17:52:53 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> 
> Ping!  Ping!  OK to push this?
> 
> > Ping!  Is it OK to install the patch proposed below?
> > 
> > > Date: Tue, 16 Jan 2018 19:31:37 +0200
> > > From: Eli Zaretskii <eliz@gnu.org>
> > > 
> > >      g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -fno-strict-aliasing -DNDEBUG -fwrapv -c -o python/py-arch.o -MT python/py-arch.o -MMD -MP  -MF python/.deps/py-arch.Tpo python/py-arch.c
> > >      In file included from d:\usr\lib\gcc\mingw32\6.3.0\include\c++\math.h:36:0,
> > > 		      from build-gnulib/import/math.h:27,
> > > 		      from d:/usr/Python26/include/pyport.h:235,
> > > 		      from d:/usr/Python26/include/Python.h:58,
> > > 		      from python/python-internal.h:94,
> > > 		      from python/py-arch.c:24:
> > >      d:\usr\lib\gcc\mingw32\6.3.0\include\c++\cmath:1157:11: error: '::hypot' has not been declared
> > > 	using ::hypot;
> > > 		^~~~~
> > >      Makefile:1618: recipe for target `python/py-arch.o' failed
> > > 
> > > Googling suggests the following solution; is it okay to push this
> > > (with the necessary logs and after doing the "paperwork" required for
> > > branch changes)?  Or does someone have better ideas?  (Does this work
> > > in MinGW64?)
> > > 
> > > --- gdb/python/python-internal.h~0	2018-01-12 05:31:04.000000000 +0200
> > > +++ gdb/python/python-internal.h	2018-01-16 08:56:10.717759900 +0200
> > > @@ -85,6 +85,12 @@
> > >  #define HAVE_SNPRINTF 1
> > >  #endif
> > >  
> > > +/* Another kludge to avoid compilation errors because MinGW defines
> > > +   'hypot' to '_hypot', but the C++ headers says "using ::hypot".  */
> > > +#if defined(__MINGW32__) && defined(__cplusplus)
> > > +# define _hypot hypot
> > > +#endif
> > > +
> > >  /* Request clean size types from Python.  */
> > >  #define PY_SSIZE_T_CLEAN
> > >  
> > > 
> > 
> 

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Simon Marchi]

On 2018-01-26 09:18 AM, Eli Zaretskii wrote:
> Ping! Ping! Ping!  This is delaying the release of GDB 8.1.
> 
> Should I just stop waiting for approval and push this?

Hi Eli,

I didn't reply to your pings because I don't feel qualified to review changes related
to mingw (you are probably the most qualified person here, since you use it).  Given
the lack of response, I'd say you can go ahead with the change, tt seems relatively safe.

I have one question below.

>> Date: Thu, 25 Jan 2018 18:58:30 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>>
>>> Date: Thu, 18 Jan 2018 17:52:53 +0200
>>> From: Eli Zaretskii <eliz@gnu.org>
>>
>> Ping!  Ping!  OK to push this?
>>
>>> Ping!  Is it OK to install the patch proposed below?
>>>
>>>> Date: Tue, 16 Jan 2018 19:31:37 +0200
>>>> From: Eli Zaretskii <eliz@gnu.org>
>>>>
>>>>      g++ -x c++  -O2 -gdwarf-4 -g3    -I. -I. -I./common -I./config  -DLOCALEDIR="\"d:/usr/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../opcodes/.. -I./../readline/..   -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber   -I./gnulib/import -Ibuild-gnulib/import   -DTUI=1   -Id:/usr/include -Id:/usr/include/guile/2.0 -Id:/usr/include   -Id:/usr/Python26/include -Id:/usr/Python26/include -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized -Wno-format  -fno-strict-aliasing -DNDEBUG -fwrapv -c -o python/py-arch.o -MT python/py-arch.o -MMD -MP  -MF python/.deps/py-arch.Tpo python/py-arch.c
>>>>      In file included from d:\usr\lib\gcc\mingw32\6.3.0\include\c++\math.h:36:0,
>>>> 		      from build-gnulib/import/math.h:27,
>>>> 		      from d:/usr/Python26/include/pyport.h:235,
>>>> 		      from d:/usr/Python26/include/Python.h:58,
>>>> 		      from python/python-internal.h:94,
>>>> 		      from python/py-arch.c:24:
>>>>      d:\usr\lib\gcc\mingw32\6.3.0\include\c++\cmath:1157:11: error: '::hypot' has not been declared
>>>> 	using ::hypot;
>>>> 		^~~~~
>>>>      Makefile:1618: recipe for target `python/py-arch.o' failed
>>>>
>>>> Googling suggests the following solution; is it okay to push this
>>>> (with the necessary logs and after doing the "paperwork" required for
>>>> branch changes)?  Or does someone have better ideas?  (Does this work
>>>> in MinGW64?)
>>>>
>>>> --- gdb/python/python-internal.h~0	2018-01-12 05:31:04.000000000 +0200
>>>> +++ gdb/python/python-internal.h	2018-01-16 08:56:10.717759900 +0200
>>>> @@ -85,6 +85,12 @@
>>>>  #define HAVE_SNPRINTF 1
>>>>  #endif
>>>>  
>>>> +/* Another kludge to avoid compilation errors because MinGW defines
>>>> +   'hypot' to '_hypot', but the C++ headers says "using ::hypot".  */
>>>> +#if defined(__MINGW32__) && defined(__cplusplus)

Do we need "defined(__cplusplus)", since we are always building GDB as a C++ program?

Thanks,

Simon

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Eli Zaretskii]

> From: Simon Marchi <simon.marchi@ericsson.com>
> Date: Fri, 26 Jan 2018 10:36:56 -0500
> >>>>  
> >>>> +/* Another kludge to avoid compilation errors because MinGW defines
> >>>> +   'hypot' to '_hypot', but the C++ headers says "using ::hypot".  */
> >>>> +#if defined(__MINGW32__) && defined(__cplusplus)
> 
> Do we need "defined(__cplusplus)", since we are always building GDB as a C++ program?

I'm okay with dropping it.

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Eli Zaretskii]

> Date: Fri, 26 Jan 2018 20:52:56 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: gdb-patches@sourceware.org
> 
> > From: Simon Marchi <simon.marchi@ericsson.com>
> > Date: Fri, 26 Jan 2018 10:36:56 -0500
> > >>>>  
> > >>>> +/* Another kludge to avoid compilation errors because MinGW defines
> > >>>> +   'hypot' to '_hypot', but the C++ headers says "using ::hypot".  */
> > >>>> +#if defined(__MINGW32__) && defined(__cplusplus)
> > 
> > Do we need "defined(__cplusplus)", since we are always building GDB as a C++ program?
> 
> I'm okay with dropping it.

No further comments, so I went ahead and pushed the change, without
the __cplusplus part, to master, and then cherry-picked to
gdb-8.1-branch.

Thanks for the feedback, Simon.

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Yao Qi]

Eli Zaretskii <eliz@gnu.org> writes:

> No further comments, so I went ahead and pushed the change, without
> the __cplusplus part, to master, and then cherry-picked to
> gdb-8.1-branch.

Hi Eli,
Is it intended to include error messages in the changelog entry?  If
not, can we remove it? in the patch below,

2018-01-27  Eli Zaretskii  <eliz@gnu.org>

	Avoid compilation errors in MinGW native builds

	The error is triggered by including python-internal.h, and the
	error message is:

	     In file included from d:\usr\lib\gcc\mingw32\6.3.0\include\c++\math.h:36:0,
		      from build-gnulib/import/math.h:27,
		      from d:/usr/Python26/include/pyport.h:235,
		      from d:/usr/Python26/include/Python.h:58,
		      from python/python-internal.h:94,
		      from python/py-arch.c:24:
	     d:\usr\lib\gcc\mingw32\6.3.0\include\c++\cmath:1157:11: error: '::hypot' has not been declared
	using ::hypot;
		^~~~~

	This happens because Python headers define 'hypot' to expand t
	'_hypot' in the Windows builds.
	* python/python-internal.h (_hypot) [__MINGW32__]: Define back to
	'hypoth'.  This avoids a compilation error.

-- 
Yao (齐尧)
From 65951173ddd802b5c26f3f706ad845f41dc78f8b Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Thu, 1 Feb 2018 15:11:46 +0000
Subject: [PATCH] Remove unnecessary bits from ChangeLog

Remove something shouldn't be put in ChangeLog.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5c3338f..40f4008 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -144,23 +144,6 @@
 
 2018-01-27  Eli Zaretskii  <eliz@gnu.org>
 
-	Avoid compilation errors in MinGW native builds
-
-	The error is triggered by including python-internal.h, and the
-	error message is:
-
-	     In file included from d:\usr\lib\gcc\mingw32\6.3.0\include\c++\math.h:36:0,
-		      from build-gnulib/import/math.h:27,
-		      from d:/usr/Python26/include/pyport.h:235,
-		      from d:/usr/Python26/include/Python.h:58,
-		      from python/python-internal.h:94,
-		      from python/py-arch.c:24:
-	     d:\usr\lib\gcc\mingw32\6.3.0\include\c++\cmath:1157:11: error: '::hypot' has not been declared
-	using ::hypot;
-		^~~~~
-
-	This happens because Python headers define 'hypot' to expand t
-	'_hypot' in the Windows builds.
 	* python/python-internal.h (_hypot) [__MINGW32__]: Define back to
 	'hypoth'.  This avoids a compilation error.
 

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Eli Zaretskii]

> From: Yao Qi <qiyaoltc@gmail.com>
> Cc: simon.marchi@ericsson.com,  gdb-patches@sourceware.org
> Date: Thu, 01 Feb 2018 15:12:47 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > No further comments, so I went ahead and pushed the change, without
> > the __cplusplus part, to master, and then cherry-picked to
> > gdb-8.1-branch.
> 
> Hi Eli,
> Is it intended to include error messages in the changelog entry?  If
> not, can we remove it? in the patch below,

Maybe I'm missing something, but isn't the commit log entry supposed
to have the same text as the ChangeLog for that change?

And what's wrong with having error messages in ChangeLog's, anyway?

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Yao Qi]

On Thu, Feb 1, 2018 at 4:27 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> Maybe I'm missing something, but isn't the commit log entry supposed
> to have the same text as the ChangeLog for that change?

My understanding is that ChangeLog entry should be copied at the end
of commit log

https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_formatted_commit_messages

>
> And what's wrong with having error messages in ChangeLog's, anyway?

Each entry in ChangeLog describes the changes to the code,
https://www.gnu.org/prep/standards/standards.html#Change-Log-Concepts
IMO, error messages are out of the scope of what ChangeLog is intended
to describe.  Also, I've never seen such ChangeLog entry (with error
messages) before.

-- 
Yao (齐尧)

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Eli Zaretskii]

> From: Yao Qi <qiyaoltc@gmail.com>
> Date: Thu, 1 Feb 2018 16:51:36 +0000
> Cc: Simon Marchi <simon.marchi@ericsson.com>, GDB Patches <gdb-patches@sourceware.org>
> 
> On Thu, Feb 1, 2018 at 4:27 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> > Maybe I'm missing something, but isn't the commit log entry supposed
> > to have the same text as the ChangeLog for that change?
> 
> My understanding is that ChangeLog entry should be copied at the end
> of commit log

I believe I did that, except that I moved the motivation for the
change before the ChangeLog header, as it belongs to the preamble of
the Git log.  (In ChangeLog, there's no subdivision of the entry.)

> > And what's wrong with having error messages in ChangeLog's, anyway?
> 
> Each entry in ChangeLog describes the changes to the code,
> https://www.gnu.org/prep/standards/standards.html#Change-Log-Concepts
> IMO, error messages are out of the scope of what ChangeLog is intended
> to describe.

This is changing, and some GNU projects already do it differently.  I
believe the next version of standards.texi will change the
recommendations to be more lax.  And using different style for each
project is a pain.  Is GDB really so rigid as to not tolerate this?

Please note that the error message I cited is just part of the
motivation for the change, so we are really talking about including
the motivation in the ChangeLog.

> Also, I've never seen such ChangeLog entry (with error messages)
> before.

That's not necessarily a reason for rejecting such entries.

So: what is the project's take on this, if there is one?

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Yao Qi]

On Thu, Feb 1, 2018 at 5:34 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Yao Qi <qiyaoltc@gmail.com>
>> Date: Thu, 1 Feb 2018 16:51:36 +0000
>> Cc: Simon Marchi <simon.marchi@ericsson.com>, GDB Patches <gdb-patches@sourceware.org>
>>
>> On Thu, Feb 1, 2018 at 4:27 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> > Maybe I'm missing something, but isn't the commit log entry supposed
>> > to have the same text as the ChangeLog for that change?
>>
>> My understanding is that ChangeLog entry should be copied at the end
>> of commit log
>
> I believe I did that, except that I moved the motivation for the
> change before the ChangeLog header, as it belongs to the preamble of
> the Git log.  (In ChangeLog, there's no subdivision of the entry.)
>

It is absolutely fine to put the reason of the change in the commit
log, but it is questionable to put commit log into ChangeLog.  At
least, nowadays, "commit log" is more than ChangeLog entry.  This
is the convention we followed for several years.

>> > And what's wrong with having error messages in ChangeLog's, anyway?
>>
>> Each entry in ChangeLog describes the changes to the code,
>> https://www.gnu.org/prep/standards/standards.html#Change-Log-Concepts
>> IMO, error messages are out of the scope of what ChangeLog is intended
>> to describe.
>
> This is changing, and some GNU projects already do it differently.  I
> believe the next version of standards.texi will change the
> recommendations to be more lax.  And using different style for each
> project is a pain.  Is GDB really so rigid as to not tolerate this?
>

GDB is evolving, so I am completely fine to changes, but you
made the change of the way of writing changelog, which is not
discussed before.

> Please note that the error message I cited is just part of the
> motivation for the change, so we are really talking about including
> the motivation in the ChangeLog.
>

Yes, do we want to change the rule of writing ChangeLog, so that
we can/should put the motivation for the change?  Is the motivation
mandatory in the changelog or optional?

>> Also, I've never seen such ChangeLog entry (with error messages)
>> before.
>
> That's not necessarily a reason for rejecting such entries.
>
> So: what is the project's take on this, if there is one?

We need a clear rule on writing changelog, otherwise how do we
write changelog or review patches?

-- 
Yao (齐尧)

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Eli Zaretskii]

> From: Yao Qi <qiyaoltc@gmail.com>
> Date: Thu, 1 Feb 2018 21:32:13 +0000
> Cc: Simon Marchi <simon.marchi@ericsson.com>, GDB Patches <gdb-patches@sourceware.org>
> 
> > Please note that the error message I cited is just part of the
> > motivation for the change, so we are really talking about including
> > the motivation in the ChangeLog.
> >
> 
> Yes, do we want to change the rule of writing ChangeLog, so that
> we can/should put the motivation for the change?  Is the motivation
> mandatory in the changelog or optional?
> 
> >> Also, I've never seen such ChangeLog entry (with error messages)
> >> before.
> >
> > That's not necessarily a reason for rejecting such entries.
> >
> > So: what is the project's take on this, if there is one?
> 
> We need a clear rule on writing changelog, otherwise how do we
> write changelog or review patches?

I agree.  I hope Someone(TM) will answer these questions.

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Joel Brobecker]

> > We need a clear rule on writing changelog, otherwise how do we
> > write changelog or review patches?
> 
> I agree.  I hope Someone(TM) will answer these questions.

Well, if it were me, I would stop using ChangeLog files with
immediate effect.

The purpose of ChangeLog files, as I understand it, is to describe
the "what changed", and is aimed at people who download a given
release and want to know what changed (because they do not have
access to the version control information).

While the above might have been essential 20 or 25 years ago,
it is trivial for anyone today to have access to the change
history of our program. Two options:
  - git clone our public repository; or...
  - ... if they don't want to use git, they can use the gitweb interface.

I think that this is an acceptable way for us to provide the log
of changes that were made.

Currently, the situation is that we all pay a price in term of time
wasted populating those files, and when we add the time collectively
spent on those, I cannot imagine the few people's convenience would
be worth that much effort.

I vote for we stop using those ChangeLog files right now. We can
still have them as part of the ChangeLog if people want -- although,
I am also open to considering the pros and cons of keeping it,
but I think they might not be worth the investiment either.

-- 
Joel

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Simon Marchi]

On 2018-02-02 10:53, Joel Brobecker wrote:
> Well, if it were me, I would stop using ChangeLog files with
> immediate effect.
> 
> The purpose of ChangeLog files, as I understand it, is to describe
> the "what changed", and is aimed at people who download a given
> release and want to know what changed (because they do not have
> access to the version control information).
> 
> While the above might have been essential 20 or 25 years ago,
> it is trivial for anyone today to have access to the change
> history of our program. Two options:
>   - git clone our public repository; or...
>   - ... if they don't want to use git, they can use the gitweb 
> interface.
> 
> I think that this is an acceptable way for us to provide the log
> of changes that were made.
> 
> Currently, the situation is that we all pay a price in term of time
> wasted populating those files, and when we add the time collectively
> spent on those, I cannot imagine the few people's convenience would
> be worth that much effort.
> 
> I vote for we stop using those ChangeLog files right now. We can
> still have them as part of the ChangeLog if people want -- although,
> I am also open to considering the pros and cons of keeping it,
> but I think they might not be worth the investiment either.

I agree about the time consumed producing ChangeLog entries.  I have to 
admit that it sometimes discourages me to do structural changes that 
would affect a lot files/functions.  I also feel like I'm filling some 
administrative paperwork, which is not my favorite way of spending my 
time.

Maintaining an internal port of GDB, one use I have for the ChangeLog is 
to find what commit removed a given function.  Let's say I rebase our 
code and find out a function we use was removed, grepping for that 
function would find the ChangeLog entry, leading me to the commit, which 
helps in fixing the code.  However, I recently discovered "git blame 
--reverse", which allows to ask git about which commit removed a 
particular line.  So the ChangeLog is not really required for that use 
case.

Since with git the whole history is available offline, I think the 
usefulness of this kind of ChangeLogs is not as big as it used to be.

To be fair I'll also state a pro I see for the ChangeLog.  When I write 
the ChangeLog entry, it makes me look at my change a last time in 
details and sometimes find some additional issues I might not have found 
otherwise.

It might be worth starting a new thread to get the attention of 
everybody to discuss this issue.

Simon

Re: [ANNOUNCEMENT] GDB 8.1 release branch created!

[Joseph Myers]

On Fri, 2 Feb 2018, Joel Brobecker wrote:

> > > We need a clear rule on writing changelog, otherwise how do we
> > > write changelog or review patches?
> > 
> > I agree.  I hope Someone(TM) will answer these questions.
> 
> Well, if it were me, I would stop using ChangeLog files with
> immediate effect.

Feel free to join the discussion on bug-standards (which has been going 
for several months) of eliminating the GNU Coding Standards requirement 
for the ChangeLog *format*.  (It's already been allowed by the GCS for 
over 20 years to generate the ChangeLog *file* at release time for the 
release tarball from version control history rather than having it checked 
in, but then of course you need to make sure that commit messages are 
appropriately formatted to derive ChangeLog entries - so I consider that 
the format, not the file, is the main issue.)

-- 
Joseph S. Myers
joseph@codesourcery.com