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 <palves@redhat.com>]

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