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

  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(-)
  

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
  

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
  

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,