From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 33457 invoked by alias); 22 Jan 2018 20:11:28 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 33441 invoked by uid 89); 22 Jan 2018 20:11:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 22 Jan 2018 20:11:25 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 946F8883D8; Mon, 22 Jan 2018 20:11:24 +0000 (UTC) Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 443F15D6A3; Mon, 22 Jan 2018 20:11:24 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches , Eli Zaretskii Subject: Re: [PATCH v2] Fix segfault when using 'set print object on' + whatis (Re: [PATCH] Fix segfault when using 'set print object on' + whatis ) References: <20180116203239.27787-1-sergiodj@redhat.com> <20180120010334.7694-1-sergiodj@redhat.com> <87tvvdbwho.fsf@redhat.com> <89e94b33-96dd-474f-cfd6-56125aca7a6d@redhat.com> Date: Mon, 22 Jan 2018 20:11:00 -0000 In-Reply-To: <89e94b33-96dd-474f-cfd6-56125aca7a6d@redhat.com> (Pedro Alves's message of "Mon, 22 Jan 2018 19:52:55 +0000") Message-ID: <87fu6xbqms.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2018-01/txt/msg00450.txt.bz2 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 >>>> : >>> >>> 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/