[patch] Fix "ambiguous linespec" regression: break lineno

[patch] Fix "ambiguous linespec" regression: break lineno

[Jan Kratochvil]

Hi,

"list" should set context for the "break barelinenumber" command, this is
a usability regression reported by Jakub Jelinek.

I have just reverted a part of code removed by the patch:

d2fae92c9d78b49086182385a5bbd086b7a030b6 is the first bad commit
Author: Tom Tromey <tromey@redhat.com>
    the "ambiguous linespec" series

It seems somehow clear to me, OK to check it in?

tested on {x86_64,x86_64-m32}-fedora18pre-linux-gnu.


Thanks,
Jan


gdb/
2012-06-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix regression by the "ambiguous linespec" series.
	* breakpoint.c (parse_breakpoint_sals): New variable cursal.  Use
	get_last_displayed_symtab and get_last_displayed_line and depending
	on CURSAL.

gdb/testsuite/
2012-06-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix regression by the "ambiguous linespec" series.
	* gdb.base/break.exp (list marker1, break lineno, delete $bpnum): New
	tests.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 12db39b..9c4348f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -8802,19 +8802,26 @@ parse_breakpoint_sals (char **address,
     }
   else
     {
+      struct symtab_and_line cursal = get_current_source_symtab_and_line ();
+
       /* Force almost all breakpoints to be in terms of the
          current_source_symtab (which is decode_line_1's default).
          This should produce the results we want almost all of the
-         time while leaving default_breakpoint_* alone.  */
-      if (last_displayed_sal_is_valid ())
+         time while leaving default_breakpoint_* alone.
+
+	 ObjC: However, don't match an Objective-C method name which
+	 may have a '+' or '-' succeeded by a '['.  */
+      if (last_displayed_sal_is_valid ()
+	  && (!cursal.symtab
+	      || ((strchr ("+-", (*address)[0]) != NULL)
+		  && ((*address)[1] != '['))))
 	decode_line_full (address, DECODE_LINE_FUNFIRSTLINE,
 			  get_last_displayed_symtab (),
 			  get_last_displayed_line (),
 			  canonical, NULL, NULL);
       else
 	decode_line_full (address, DECODE_LINE_FUNFIRSTLINE,
-			  (struct symtab *) NULL, 0,
-			  canonical, NULL, NULL);
+			  cursal.symtab, cursal.line, canonical, NULL, NULL);
     }
 }
 
diff --git a/gdb/testsuite/gdb.base/break.exp b/gdb/testsuite/gdb.base/break.exp
index a203364..4254eca 100644
--- a/gdb/testsuite/gdb.base/break.exp
+++ b/gdb/testsuite/gdb.base/break.exp
@@ -371,6 +371,13 @@ gdb_expect {
     }
 }
 
+# Test the 'list' commands sets current file for the 'break LINENO' command.
+set bp_marker1 [gdb_get_line_number "set breakpoint 16 here" ${srcfile1}]
+gdb_test "list marker1" ".*"
+gdb_test "break $bp_marker1" "Breakpoint \[0-9\]+ at 0x\[0-9a-f\]+: file .*${srcfile1}, line ${bp_marker1}\\." \
+         "break lineno"
+gdb_test_no_output {delete $bpnum}
+
 #
 # run until the breakpoint at a line number
 #

Re: [patch] Fix "ambiguous linespec" regression: break lineno

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> It seems somehow clear to me, OK to check it in?

I agree it is an improvement.

However, why should this apply to linespecs used by 'break' but not by
other ones?

Jan> +      if (last_displayed_sal_is_valid ()

linespec.c:initialize_defaults has:

      struct symtab_and_line cursal = 
	get_current_source_symtab_and_line ();

It seems like we have two similar notions here -- the "current" source
line and the "last displayed" source line.

This doesn't make sense to me.  Can we not just have a single notion and
use it everywhere?

If we really need two, can we do the processing in linespec.c?

I realize you're just reverting a bit of code - but is that ObjC hack
really needed?  I'd like us to get away from this kind of thing.

Tom

Re: [patch] Fix "ambiguous linespec" regression: break lineno

[Matt Rice]

On Fri, Jun 8, 2012 at 1:40 PM, Tom Tromey <tromey@redhat.com> wrote:

> I realize you're just reverting a bit of code - but is that ObjC hack
> really needed?  I'd like us to get away from this kind of thing.

I have failed to find any justification for it now, or when it was
removed from your linespec branch, it's possibly related to the old
ambiguous method name support, which seems to work fine without it
now.

Re: [patch] Fix "ambiguous linespec" regression: break lineno

[Jan Kratochvil]

On Fri, 08 Jun 2012 22:40:55 +0200, Tom Tromey wrote:
> However, why should this apply to linespecs used by 'break' but not by
> other ones?
+
> If we really need two, can we do the processing in linespec.c?

Because the values were called default_breakpoint_* before the patch
	commit eb1a2e1ef3957213a420bbeedff9c045016e3aa0
	Author: Justin Lebar <justin.lebar@gmail.com>
	skip/blacklist patch
renamed default_breakpoint_* into *_last_displayed_ functions.

So I find OK to use default_breakpoint_* in breakpoint.c and not to use
default_breakpoint_* in other .c files.


> Jan> +      if (last_displayed_sal_is_valid ()
> 
> linespec.c:initialize_defaults has:
> 
>       struct symtab_and_line cursal = 
> 	get_current_source_symtab_and_line ();
> 
> It seems like we have two similar notions here -- the "current" source
> line and the "last displayed" source line.

default_breakpoint_* vs. current_source_* locations were always duplicate this
way.


> This doesn't make sense to me.  Can we not just have a single notion and
> use it everywhere?

I have some draft patch almost without regressions but those two locations had
some logic.  "break" should put breakpoint to the current frame and not to
some last arbitrary line listed.

But that seems as definitely a cleanup patch outside of the scope of this one.


> I realize you're just reverting a bit of code - but is that ObjC hack
> really needed?  I'd like us to get away from this kind of thing.

It needs to detect the pattern ^[+-] (for 'break +5', 'break -3' etc.) but it
should not get confused by the ObjC breakpoints for -[func] or +[func].

I agree if the two locations get unified it is no longer needed.


Thanks,
Jan

Re: [patch] Fix "ambiguous linespec" regression: break lineno

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I have some draft patch almost without regressions but those two
Jan> locations had some logic.  "break" should put breakpoint to the
Jan> current frame and not to some last arbitrary line listed.

Ok, I see.

>> I realize you're just reverting a bit of code - but is that ObjC hack
>> really needed?  I'd like us to get away from this kind of thing.

Jan> It needs to detect the pattern ^[+-] (for 'break +5', 'break -3'
Jan> etc.) but it should not get confused by the ObjC breakpoints for
Jan> -[func] or +[func].

Jan> I agree if the two locations get unified it is no longer needed.

I wonder if it would be cleaner as a flag to linespec.
That's just speculation, please don't consider it an objection.
I think you can go ahead as you like.

Tom

Re: [patch] Fix "ambiguous linespec" regression: break lineno

[Jan Kratochvil]

On Sat, 09 Jun 2012 00:18:57 +0200, Matt Rice wrote:
> On Fri, Jun 8, 2012 at 1:40 PM, Tom Tromey <tromey@redhat.com> wrote:
> > I realize you're just reverting a bit of code - but is that ObjC hack
> > really needed?  I'd like us to get away from this kind of thing.
> 
> I have failed to find any justification for it now, or when it was
> removed from your linespec branch, it's possibly related to the old
> ambiguous method name support, which seems to work fine without it
> now.

I do not fully understand this comment, I hope it has been answered in the
thread now.


Thanks,
Jan

[commit] [patch] Fix "ambiguous linespec" regression: break lineno

[Jan Kratochvil]

On Mon, 11 Jun 2012 17:14:37 +0200, Tom Tromey wrote:
> I wonder if it would be cleaner as a flag to linespec.
> That's just speculation, please don't consider it an objection.
> I think you can go ahead as you like.

When I already wrote and hopefully finish the unification patch I just kept
this one to be "revert".

Checked in:
	http://sourceware.org/ml/gdb-cvs/2012-06/msg00077.html


Thanks,
Jan