From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 98681 invoked by alias); 1 Feb 2020 12:17: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 98672 invoked by uid 89); 1 Feb 2020 12:17:28 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=emergency X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 01 Feb 2020 12:17:26 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id D61D2117616; Sat, 1 Feb 2020 07:17:24 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 3Ffd8Y8RHQqB; Sat, 1 Feb 2020 07:17:24 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 5341B1175FB; Sat, 1 Feb 2020 07:17:24 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 14EDF82C47; Sat, 1 Feb 2020 16:17:20 +0400 (+04) Date: Sat, 01 Feb 2020 12:17:00 -0000 From: Joel Brobecker To: Simon Marchi , tom@tromey.com Cc: Jonah Graham , gdb-patches@sourceware.org, Pedro Alves , Iain Buclaw , Nick Alcock , Eli Zaretskii Subject: Re: Propose we release GDB 9.1 next weekend (Feb 01-02) Message-ID: <20200201121720.GA30012@adacore.com> References: <20200126114033.GA20733@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-SW-Source: 2020-02/txt/msg00014.txt.bz2 Hi Jonah, Thank you for reporting... In time before we created the release! This is the kind of bug that I think would have forced us to issue an emergency fixup release. Hi Simon, Thanks very much for looking into it. On Sat, Feb 01, 2020 at 03:00:05AM -0500, Simon Marchi wrote: > On 2020-01-31 10:20 p.m., Jonah Graham wrote: > > On Sun, 26 Jan 2020 at 06:40, Joel Brobecker wrote: > >> > >> Pending comments, I will work on the GDB 9.1 release during > >> the weekend of Feb 01-02. > >> > >> > > > > Hi folks, > > > > Sorry to be coming to the party a little late on this - but GDB 9.1 > > has a serious regression related to path handling. This affects > > Eclipse CDT users because the default build system in CDT does not > > pass absolute paths to GCC when compiling. Thanks to some efforts of > > users who have been exposed early to this thanks to fc31 pushing > > pre-releases of 9.1 out we have detected it before release, but just > > barely :-) > > > > The Bug is https://sourceware.org/bugzilla/show_bug.cgi?id=24915 and > > it has been bouncing around for a while, with it seemingly fixed for a > > while, but it turns out that there are lots of cases that it is not > > fixed. The patch that causes the problem is > > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a0c1ffedcf1988bc13fc5b6d57d3b74a17b60299 > > > > The summary is that GDB is often failing to insert breakpoints when > > the absolute path to the file is passed to GDB if it was compiled with > > a different path, such as a path with .. in the compilation path, or > > symlinks. This would require IDEs to know the compilation path of a > > file it has open when a breakpoint is inserted, rather than only > > having to know the absolute path to the file that is actually open. > > > > Setting basenames-may-differ to on is a workaround as the new code in > > the commit is guarded by that. However GDB is failing even when > > basenames don't differ. > > > > I have added two short traces in Comment 17 with all the commands > > (https://sourceware.org/bugzilla/show_bug.cgi?id=24915#c17), but I > > want to show you the part of the trace that indicates this is a bug. > > The breakpoint fails to insert on the first instance - but then works > > on the second try with the intervening break with no path. > > > > $ /scratch/gdb/build-gdb-9/gdb/gdb --quiet main > > Reading symbols from main... > > (gdb) b /scratch/gdb/test/src/main2.c:1 ## Breakpoint fails here > > No source file named /scratch/gdb/test/src/main2.c. > > Make breakpoint pending on future shared library load? (y or [n]) n > > (gdb) b main2.c:1 > > Breakpoint 1 at 0x609: file ../src/main2.c, line 2. > > (gdb) b /scratch/gdb/test/src/main2.c:1 ## same command works here > > Note: breakpoint 1 also set at pc 0x609. > > Breakpoint 2 at 0x609: file ../src/main2.c, line 2. > > (gdb) quit > > $ > > > > Sorry for bringing this up so late in the dev cycle - I thought this > > was fixed, but it was only fixed for a limited number of cases. > > Thanks for reporting, I think it's a quite serious problem that we should > address before the release. I looked into it a bit, here's my take on it. > > The commit that introduced the regression has an impact when we reach this > point: > > #0 find_and_open_source (filename=0x6210000c6dc0 "../src/main2.c", dirname=0x60b000062c4e "/tmp/test/build", fullname=0x7ffc78d15d50) at /home/simark/src/binutils-gdb/gdb/source.c:1034 > #1 0x0000560b5146e58a in psymtab_to_fullname (ps=0x60b000062e30) at /home/simark/src/binutils-gdb/gdb/psymtab.c:1149 > #2 0x0000560b51468b0a in psym_map_symtabs_matching_filename(objfile *, const char *, const char *, gdb::function_view) (objfile=0x614000007640, name=0x6030000412c0 "/tmp/test/src/main2.c", real_path=0x621000102d00 "/tmp/test/src/main2.c", callback=...) at /home/simark/src/binutils-gdb/gdb/psymtab.c:182 > #3 0x0000560b5188b4ea in iterate_over_symtabs(char const*, gdb::function_view) (name=0x6030000412c0 "/tmp/test/src/main2.c", callback=...) at /home/simark/src/binutils-gdb/gdb/symtab.c:558 > #4 0x0000560b510c1351 in collect_symtabs_from_filename (file=0x6030000412c0 "/tmp/test/src/main2.c", search_pspace=0x0) at /home/simark/src/binutils-gdb/gdb/linespec.c:3798 > #5 0x0000560b510c15e2 in symtabs_from_filename (filename=0x6030000412c0 "/tmp/test/src/main2.c", search_pspace=0x0) at /home/simark/src/binutils-gdb/gdb/linespec.c:3818 > #6 0x0000560b510b7df5 in parse_linespec (parser=0x7ffc78d16b50, arg=0x603000041290 "/tmp/test/src/main2.c:2", match_type=symbol_name_match_type::WILD) at /home/simark/src/binutils-gdb/gdb/linespec.c:2615 > #7 0x0000560b510bbdcd in event_location_to_sals (parser=0x7ffc78d16b50, location=0x60600007fdc0) at /home/simark/src/binutils-gdb/gdb/linespec.c:3152 > #8 0x0000560b510bc655 in decode_line_full (location=0x60600007fdc0, flags=1, search_pspace=0x0, default_symtab=0x0, default_line=0, canonical=0x7ffc78d17390, select_mode=0x0, filter=0x0) at /home/simark/src/binutils-gdb/gdb/linespec.c:3232 > > In psym_map_symtabs_matching_filename, we go through all partial symtabs and > try to find those that match the given search name in various ways. The function > psymtab_to_fullname gives us the "full name" of the partial symtab (it gets it from > find_and_open_source), but it's not really clear whether that means just any absolute > path that resolves to that file, or if it means the real/canonical path. > > Before the mentioned commit, we always returned the real/canonical path, after the > commit, we return an absolute path that may not be the canonical path. So in my case, > we end up calling compare_filenames_for_search with "/tmp/test/src/main2.c" as the > search name and "/tmp/test/build/../src/main2.c" as the psymtab's "fullname", and it > returns false. > > My understanding is that Tom's patch meant to change the way the symtab's paths are > displayed, showing the version with symlinks and possibly some ".." in it, rather than > the real path. That represents "how the user compiled the file". But doing so, it also > changed the symtab's full name that is expected to be a real/canonical path, for search > purposes. > > [The change of the way the symtab's path is displayed probably explains why you now see > non-real/canonical paths in the MI output, as mentioned in the bug.] > > If we want to keep both behaviors, I think we need to separate that "fullname" field/concept > in absolute_fullname and realpath_fullname. The former being what Tom's patch wants, where we > keep symlinks and "..", and the latter being the canonical path, which the search wants. Doing > so would hopefully make things clearer. I think that just "fullname" is confusing: we are left > wondering if that field contains a canonical path or not. > > I have a patch that does that, I'll start a test run and see the results tomorrow. It's perhaps > a bit too invasive for the stable branch though. Here it is in the mean time: > > https://github.com/simark/binutils-gdb/tree/symtab-realpath I haven't reviewed the patch super carefully, but I like the idea! For the release, it's indeed a bit on the fence. Given the nature of the patch, With a bit of careful review, we should be able to convince ourselves that this patch is sufficiently safe, particularly for a .1. On the other hand, I think the patch that triggered the regression is a fairly minor enhancement (name of the file printed by annotations). So perhaps the best way forward is to revert the triggering patch from the gdb-9-branch. I've added Tom to way in on this... -- Joel