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

[Joel Brobecker <brobecker@adacore.com>]

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