From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22444 invoked by alias); 27 Nov 2019 23:00:34 -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 22409 invoked by uid 89); 27 Nov 2019 23:00:33 -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=opposing 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, 27 Nov 2019 23:00:31 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id 43DE62046F; Wed, 27 Nov 2019 18:00:30 -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 05A3F20362; Wed, 27 Nov 2019 18:00:27 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id D732020AF6; Wed, 27 Nov 2019 18:00:27 -0500 (EST) X-Gerrit-PatchSet: 1 Date: Wed, 27 Nov 2019 23:00:00 -0000 From: "Simon Marchi (Code Review)" To: Kevin Buettner , gdb-patches@sourceware.org Cc: 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, 27 Nov 2019 18:00:27 -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: <20191127230027.D732020AF6@gnutoolchain-gerrit.osci.io> X-SW-Source: 2019-11/txt/msg01078.txt.bz2 Simon Marchi has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/682 ...................................................................... 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. -- 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, 27 Nov 2019 23:00:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment