Re: Propose we release GDB 9.1 next weekend (Feb 01-02)

[Jonah Graham <jonah@kichwacoders.com>]

~~~
Jonah Graham
Kichwa Coders
www.kichwacoders.com

On Sat, 1 Feb 2020 at 07:17, Joel Brobecker <brobecker@adacore.com> wrote:
>
> 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 Joel,
Thank you for all the hard work. I would like to help in testing a fix
- I am mostly not around this weekend so can't do anything useful
until next week.

>
> Hi Simon,
>
> Thanks very much for looking into it.

I second that!

>
> 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 <brobecker@adacore.com> 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<bool(symtab*)>) (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<bool (symtab*)>) (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.

Can fullname field (at least in MI output) continue to be
realpath_fullname concept and if needed in MI a new field added for
absolute_fullname? In the CLI output users see just file
(../src/main2.c), but that is not generally useful in the IDE case as
it is display only.

> >
> > 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...

Tom If you decide to revert (which would be my preference) I will
happily work with you to get this back in. I previously wrote a
testcase for the simple case that did already get fixed, I can work on
a testcase for the new case too for the suite if it helps.
https://sourceware.org/ml/gdb-patches/2019-08/msg00421.html

> --
> Joel