From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 84442 invoked by alias); 4 Dec 2019 22:04:53 -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 84432 invoked by uid 89); 4 Dec 2019 22:04:53 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-0.4 required=5.0 tests=BAYES_00,KAM_STOCKGEN autolearn=no version=3.3.1 spammy=sk:kevinb@, Buettner, U*kevinb, chances X-HELO: mx1.osci.io Received: from polly.osci.io (HELO mx1.osci.io) (8.43.85.229) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 04 Dec 2019 22:04:51 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id EE7D920422; Wed, 4 Dec 2019 17:04:49 -0500 (EST) Received: from gnutoolchain-gerrit.osci.io (gnutoolchain-gerrit.osci.io [IPv6:2620:52:3:1:5054:ff:fe06:16ca]) by mx1.osci.io (Postfix) with ESMTP id 09B0320172; Wed, 4 Dec 2019 17:04:45 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id D2E432816F; Wed, 4 Dec 2019 17:04:44 -0500 (EST) X-Gerrit-PatchSet: 1 Date: Wed, 04 Dec 2019 22:04:00 -0000 From: "Kevin Buettner (Code Review)" To: gdb-patches@sourceware.org Cc: Simon Marchi , Luis Machado Auto-Submitted: auto-generated X-Gerrit-MessageType: comment Subject: [review] Avoid infinite recursion in find_pc_sect_line X-Gerrit-Change-Id: I595470be6ab5f61ca7e4e9e70c61a252c0deaeaa X-Gerrit-Change-Number: 682 X-Gerrit-ChangeURL: X-Gerrit-Commit: 23229a89912a2e567f1d1a4ebb7ecc5c12f86c0c In-Reply-To: References: X-Gerrit-Comment-Date: Wed, 4 Dec 2019 17:04:44 -0500 Reply-To: gnutoolchain-gerrit@osci.io MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/3.0.3-79-g83ff7f88f1 Content-Type: text/plain; charset=UTF-8 Message-Id: <20191204220444.D2E432816F@gnutoolchain-gerrit.osci.io> X-SW-Source: 2019-12/txt/msg00169.txt.bz2 Kevin Buettner has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/682 ...................................................................... Patch Set 1: > Patch Set 1: > > I'm not 100% comfortable in merging fixes like this, in an area that's already very tricky, without reproducer. I'm not strongly opposing either, but just sharing my opinion. > > It just doesn't sound logical. To reach this "else", we need to: > > 1. lookup a minsym for PC, that minsym is a trampoline. It means that this trampoline minsym is the closest (smaller or equal) minsym from PC, all minsym types considered. > 2. lookup a _text_ minsym by name, we find one that is exactly equal to pc > > If that text minsym we have found in #2 is equal to PC, why did we not find it in #1? The only way I see is for the two minsyms (the trampoline one and the text one) to have the same value. The lookup function would have returned the trampoline one, because it can only return one symbol, so it has to choose, and the trampoline one happens to come before. Then, we search specifically text symbols by name, we find the text one. But that case would already handled with the existing > > else if (BMSYMBOL_VALUE_ADDRESS (mfunsym) > == BMSYMBOL_VALUE_ADDRESS (msymbol)) > > So the only explanation I can imagine is that there was another GDB bug, causing it to mess up the minsym creation or lookup. And either: > > - that bug was fixed, and this fix is no longer needed > - that bug still isn't fixed, in which case this fix just covers up the problem. > > I think you are already conscious of this, since you added a warning to tell the user that this is not expected. But if this is a condition we expect to never legitimately happen, I would suggest using an assertion instead: > > gdb_assert (BMSYMBOL_VALUE_ADDRESS (mfunsym) != pc); > return find_pc_line (BMSYMBOL_VALUE_ADDRESS (mfunsym), 0); > > If this ever happens again in the field, we have more chances to hear about it than if it's a warning that users can easily overlook. As a GDB user, I'd prefer that GDB do its best to provide a reasonable result. However, as I developer, I too, really want a bug report. I don't have a strong preference either way, so I went (mostly) with your suggestion, but the new patch calls internal_error instead of using an assert. This way, I could include a plea to file a bug report. (Though that may be obvious if you get an assertion failure.) Anyway, the new patch can be found here: https://www.sourceware.org/ml/gdb-patches/2019-12/msg00168.html (I didn't use gerrit to post it.) -- Gerrit-Project: binutils-gdb Gerrit-Branch: master Gerrit-Change-Id: I595470be6ab5f61ca7e4e9e70c61a252c0deaeaa Gerrit-Change-Number: 682 Gerrit-PatchSet: 1 Gerrit-Owner: Kevin Buettner Gerrit-Reviewer: Kevin Buettner Gerrit-CC: Luis Machado Gerrit-CC: Simon Marchi Gerrit-Comment-Date: Wed, 04 Dec 2019 22:04:44 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment