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

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

[Joel Brobecker]

Hello everyone,

As far as I can tell, we still have a couple of issues open, with
unfortunately no progress that I can tell for the past week or so.
At this point, the GDB 9.1 release has really been a long time coming,
we really should try to get it out the door soon. Here are the
remaining known issue, with my comments on how I propose we proceed:

  - [IanB/PedroA] output out of order / issue with filtered/unfiltered output order
    https://sourceware.org/ml/gdb-patches/2019-11/msg00859.html

    Affects cgdb via annotations:
    https://sourceware.org/bugzilla/show_bug.cgi?id=25190

    These are the patches that have been proposed:
    (a) https://sourceware.org/ml/gdb-patches/2019-11/msg01120.html
    (b) https://sourceware.org/ml/gdb-patches/2019-11/msg01121.html

    As far as I can tell: For (a), the patch doesn't look unreasonable
    and is pretty simple. Andrew sent some comments, but we didn't get
    an updated version.

    For (b), Simon mentioned that he was trying to apply it, and was
    then able to. But I haven't seen any comments on it. So I took
    a look, and sent my first impressions on the patch:
    https://www.sourceware.org/ml/gdb-patches/2020-01/msg00842.html

        => At this point, I am not confident that we are going to
           be able to solve that issue right away. Since there is
           an easy work around for this issue, I propose we consider
           this issue as *not* blocking for 9.1.

  - [Eliz/NickA/JoelB] libtcf fails to build on MinGW
    https://sourceware.org/bugzilla/show_bug.cgi?id=25155

    The only issue left to fix is the issue with errno.

        => Still waiting for Nick. Rather than putting more pressure
           on Nick, I propose we go with Eli's patch of providing
           fallback errno values. When Nick is able to confirm
           his final patch, we'll decide whether to backport or
           stay with the simpler patch from Eli.

Pending comments, I will work on the GDB 9.1 release during
the weekend of Feb 01-02.

Thank you!
-- 
Joel

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

[Eli Zaretskii]

> Date: Sun, 26 Jan 2020 15:40:33 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: palves@redhat.com, Iain Buclaw <ibuclaw@gdcproject.org>,	Nick Alcock <nick.alcock@oracle.com>, Eli Zaretskii <eliz@gnu.org>
> 
>   - [Eliz/NickA/JoelB] libtcf fails to build on MinGW
>     https://sourceware.org/bugzilla/show_bug.cgi?id=25155
> 
>     The only issue left to fix is the issue with errno.
> 
>         => Still waiting for Nick. Rather than putting more pressure
>            on Nick, I propose we go with Eli's patch of providing
>            fallback errno values. When Nick is able to confirm
>            his final patch, we'll decide whether to backport or
>            stay with the simpler patch from Eli.

Fine with me (obviously).

Thanks.

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

[Nick Alcock]

On 26 Jan 2020, Joel Brobecker outgrape:
>   - [Eliz/NickA/JoelB] libtcf fails to build on MinGW
>     https://sourceware.org/bugzilla/show_bug.cgi?id=25155
>
>     The only issue left to fix is the issue with errno.
>
>         => Still waiting for Nick. Rather than putting more pressure
>            on Nick, I propose we go with Eli's patch of providing
>            fallback errno values. When Nick is able to confirm
>            his final patch, we'll decide whether to backport or
>            stay with the simpler patch from Eli.

FYI: I'm just knocking some last work madness away and will grab a bit
to look at this tomorrow.

-- 
NULL && (void)

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

[Hannes Domani via gdb-patches]

 Am Sonntag, 26. Januar 2020, 12:40:48 MEZ hat Joel Brobecker <brobecker@adacore.com> Folgendes geschrieben:

> Hello everyone,
>
> As far as I can tell, we still have a couple of issues open, with
> unfortunately no progress that I can tell for the past week or so.
> At this point, the GDB 9.1 release has really been a long time coming,
> we really should try to get it out the door soon. Here are the
> remaining known issue, with my comments on how I propose we proceed:

I just noticed that gdbserver is a bit broken for x86_64-mingw (and I think
it always was).

Reason for it is this part in gdbserver/server.c:

    document += string_printf
      ("  <library name=\"%s\"><segment address=\"0x%lx\"/></library>\n",
       dll.name.c_str (), (long) dll.base_addr);

(long) is always 32bit on Windows, so the upper bits are cut off.

Fix seems simple:

     document += string_printf
-      ("  <library name=\"%s\"><segment address=\"0x%lx\"/></library>\n",
-       dll.name.c_str (), (long) dll.base_addr);
+      ("  <library name=\"%s\"><segment address=\"0x%s\"/></library>\n",
+       dll.name.c_str (), paddress (dll.base_addr));

I just tested it and this improves the debugging experience a lot.

But I guess this is probably too late for the release.


Regards
Hannes Domani

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

[Hannes Domani via gdb-patches]

 Am Dienstag, 28. Januar 2020, 18:13:15 MEZ hat Simon Marchi <simark@simark.ca> Folgendes geschrieben:

> On 2020-01-28 11:53 a.m., Hannes Domani via gdb-patches wrote:
> > I just noticed that gdbserver is a bit broken for x86_64-mingw (and I think
> > it always was).>
> > Reason for it is this part in gdbserver/server.c:
> >
> >     document += string_printf
> >       ("  <library name=\"%s\"><segment address=\"0x%lx\"/></library>\n",
> >        dll.name.c_str (), (long) dll.base_addr);
> >
> > (long) is always 32bit on Windows, so the upper bits are cut off.
>
> Indeed.  That code went through several refactors, but originally it was introduced
> by this 2007 commit:
>
>   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=255e7678a93693bd4d16cc3246442a1b8e11064e
>
> which has this line:
>
>   sprintf (p, "0x%lx", (long) dll->base_addr);
>
> So I think the bug has been there forever.
>
>
> > Fix seems simple:
> >
> >      document += string_printf
> > -      ("  <library name=\"%s\"><segment address=\"0x%lx\"/></library>\n",
> > -       dll.name.c_str (), (long) dll.base_addr);
> > +      ("  <library name=\"%s\"><segment address=\"0x%s\"/></library>\n",
> > +       dll.name.c_str (), paddress (dll.base_addr));
> >
> > I just tested it and this improves the debugging experience a lot.
> >
> > But I guess this is probably too late for the release.
>
>
> I don't think it so, we can always push fixes to the release branch.  This one
> seems desirable and not risky at all, so I think it would be fine.  Would you
> be willing to submit a complete patch for this?

Yes, I will do that.


Regards
Hannes Domani

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

[Jonah Graham]

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.

Jonah

Re: 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

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

[Joel Brobecker]

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

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

[Jonah Graham]

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

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

[Joel Brobecker]

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

FWIW, in terms of reverting, I was only thinking of reverting
in the gdb-9-branch. It's possible we might decide to revert
in master as well, but, at the moment, it's looking like we have
a decent chance of fixing the issue in master withing a reasonable
time frame. So I'm not advocating for a revert in master just yet.
Open to people's thoughts, though, as always!

-- 
Joel

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

[Simon Marchi]

On 2020-02-01 7:17 a.m., Joel Brobecker wrote:
> 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.

Yeah, I think it would be better to revert Tom's patch for the release.

For master, we can take the time to think about what we want long term, and if
a patch like the one I proposed useful.

Simon

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

[Simon Marchi]

On 2020-02-01 7:57 a.m., Jonah Graham wrote:
> 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.

The MI documentation doesn't say anything about what that fullname field should
contain, in fact it's just shown in examples and never formally described.

But given that it has contained the realpath for years, this is what the IDEs/users
expect, so I indeed think we should make sure it stays that way.

> 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

Thanks, that will be useful.

Simon

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

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> FWIW, in terms of reverting, I was only thinking of reverting
Joel> in the gdb-9-branch.

I sent a reversion patch.  It's pretty straightforward, but take a quick
look anyway.

Joel> It's possible we might decide to revert
Joel> in master as well, but, at the moment, it's looking like we have
Joel> a decent chance of fixing the issue in master withing a reasonable
Joel> time frame. So I'm not advocating for a revert in master just yet.
Joel> Open to people's thoughts, though, as always!

I wasn't sure what to do here, either.

For trunk I think we should at least have a regression test for the
bug exposed in PR breakpoints/24915.

I suppose right now the name to search by and the name to display are
entangled in the code.  So maybe another approach to fixing the original
problem would be to untangle them somehow.  I recall wanting to move
this information into the source cache, and out of the symtabs; but I
looked at this and it was a little complicated because, IIRC, the search
can depend on the path to the objfile.  Maybe we could have a unified
table per objfile though, the idea being to consolidate and cache the
lookups, so that (1) it's shared across symtabs and psymtabs, and (2)
things like the name to print can be computed lazily if possible.

Hopefully this makes sense,
Tom