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

[Simon Marchi]

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

Simon