[patch] physname regression: Non-matching type false breakpoint

[patch] physname regression: Non-matching type false breakpoint

[Jan Kratochvil]

Hi,

FSF GDB HEAD can now successfully break on a function which does not match the
specified type.

This is a physname regression.


Thanks,
Jan


gdb/
2011-06-05  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* linespec.c (find_method): Accept the function type automatically only
	if it was specified with parameter types.

gdb/testsuite/
2011-06-05  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.cp/paren-type.cc: New files.
	* gdb.cp/paren-type.exp: New files.

--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -1732,7 +1732,10 @@ find_method (int funfirstline, struct linespec_result *canonical,
   i1 = find_methods (t, copy, SYMBOL_LANGUAGE (sym_class), sym_arr,
 		     file_symtab);
 
-  if (i1 == 1)
+  /* The strchr check is there if any types were specified verify the single
+     found found parameter types really match those specified by user in COPY.
+     */
+  if (i1 == 1 && strchr (copy, '(') == NULL)
     {
       /* There is exactly one field with that name.  */
       sym = sym_arr[0];
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/paren-type.cc
@@ -0,0 +1,33 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+class C
+{
+public:
+  static void f (short x);
+};
+
+void
+C::f (short x)
+{
+}
+
+int
+main ()
+{
+  C::f(1);
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/paren-type.exp
@@ -0,0 +1,24 @@
+# Copyright (C) 2011 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set testfile paren-type
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${testfile}.cc {c++ debug}] } {
+    return -1
+}
+
+gdb_test "break C::f(long)" {the class `C' does not have any method instance named f\(long\)}
+
+# Sanity check the breakpoints work for the correct type.
+gdb_test "break C::f(short)" "Breakpoint 1 at \[^\r\n\]*"

Re: [patch] physname regression: Non-matching type false breakpoint

[Mark Kettenis]

> Date: Sun, 5 Jun 2011 17:34:19 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> +++ b/gdb/linespec.c
> @@ -1732,7 +1732,10 @@ find_method (int funfirstline, struct linespec_result *canonical,
>    i1 = find_methods (t, copy, SYMBOL_LANGUAGE (sym_class), sym_arr,
>  		     file_symtab);
>  
> -  if (i1 == 1)
> +  /* The strchr check is there if any types were specified verify the single
> +     found found parameter types really match those specified by user in COPY.
> +     */

That lone */ looks weird.

comments formatting [Re: [patch] physname regression: Non-matching type false breakpoint]

[Jan Kratochvil]

On Sun, 05 Jun 2011 17:57:31 +0200, Mark Kettenis wrote:
> > From: Jan Kratochvil <jan.kratochvil@redhat.com>
> > +  /* The strchr check is there if any types were specified verify the single
> > +     found found parameter types really match those specified by user in COPY.
> > +     */
> 
> That lone */ looks weird.

I agree, this commonly happens, I do not know what is the right way to do with it.

 * exceeding 80 columns is forbidden: COPY.   */

 * Non-standard formatting of paragraph is not good - as the next modification
   will possibly reformat it a bit differently, creating false diffs confusing
   diff readers: by user in\n
   COPY.  */

 * I usually reword the sentence to fit the formatting better but TBH I do not
   find that a too much usefully spent time of my life.


Thanks,
Jan

Re: comments formatting [Re: [patch] physname regression: Non-matching type false breakpoint]

[Andreas Schwab]

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

> On Sun, 05 Jun 2011 17:57:31 +0200, Mark Kettenis wrote:
>> > From: Jan Kratochvil <jan.kratochvil@redhat.com>
>> > +  /* The strchr check is there if any types were specified verify the single
>> > +     found found parameter types really match those specified by user in COPY.
>> > +     */
>> 
>> That lone */ looks weird.
>
> I agree, this commonly happens, I do not know what is the right way to do with it.

M-q (c-fill-paragraph)

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: comments formatting [Re: [patch] physname regression: Non-matching type false breakpoint]

[Jan Kratochvil]

On Sun, 05 Jun 2011 18:23:40 +0200, Andreas Schwab wrote:
> Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> > I agree, this commonly happens, I do not know what is the right way to do with it.
> 
> M-q (c-fill-paragraph)

What does it do?  I use VIM.


Thanks,
Jan

Re: comments formatting [Re: [patch] physname regression: Non-matching type false breakpoint]

[Alfred M. Szmidt]

   > > I agree, this commonly happens, I do not know what is the right
   > > way to do with it.
   > 
   > M-q (c-fill-paragraph)

   What does it do?  I use VIM.

It fills (reformats) a paragraph to fit with-in 70 columns; this:

/* The strchr check is there if any types were specified verify the single found found parameter types really match those specified by user in COPY.  */

becomes

/* The strchr check is there if any types were specified verify the
   single found found parameter types really match those specified by
   user in COPY.  */

Re: comments formatting [Re: [patch] physname regression: Non-matching type false breakpoint]

[Jan Kratochvil]

On Sun, 05 Jun 2011 18:49:39 +0200, Alfred M. Szmidt wrote:
> It fills (reformats) a paragraph to fit with-in 70 columns; this:

OK (in VIM it is gq} or gqip ).  I use 79 columns instead of 70.
So in the case of 70 columns what would be the right formatting of:

/* The strchr check is there if any verify the single found found parameter foo types really match those specified by user in COPY.  */

The 70 columns formatting produces:

/* The strchr check is there if any verify the single found found
   parameter foo types really match those specified by user in COPY.
   */


Thanks,
Jan

Re: comments formatting [Re: [patch] physname regression: Non-matching type false breakpoint]

[Alfred M. Szmidt]

   > It fills (reformats) a paragraph to fit with-in 70 columns; this:

   OK (in VIM it is gq} or gqip ).  I use 79 columns instead of 70.
   So in the case of 70 columns what would be the right formatting of:

   /* The strchr check is there if any types were specified verify the single found found parameter types really match those specified by user in COPY.  */

It would be the one I produced in the previous email:

/* The strchr check is there if any types were specified verify the
   single found found parameter types really match those specified by
   user in COPY.  */

Re: comments formatting [Re: [patch] physname regression: Non-matching type false breakpoint]

[Jan Kratochvil]

On Sun, 05 Jun 2011 19:15:32 +0200, Alfred M. Szmidt wrote:
>    OK (in VIM it is gq} or gqip ).  I use 79 columns instead of 70.
>    So in the case of 70 columns what would be the right formatting of:
> 
>    /* The strchr check is there if any types were specified verify the single found found parameter types really match those specified by user in COPY.  */

This is not a quote of my text, I wrote:
     /* The strchr check is there if any verify the single found found parameter foo types really match those specified by user in COPY.  */

See:
	http://sourceware.org/ml/gdb-patches/2011-06/msg00074.html


> It would be the one I produced in the previous email:

The text I provided was specially crafted to hit the same problem with 70
columns as I faced with the 79 columns and as I / others will face in the
future.

When the GNU Coding Standard does describe the comments formatting rules one
has to unfortunately reverse engineer them from Emacs.


Thanks,
Jan

Re: comments formatting [Re: [patch] physname regression: Non-matching type false breakpoint]

[Mark Kettenis]

> Date: Sun, 5 Jun 2011 18:35:06 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> On Sun, 05 Jun 2011 18:23:40 +0200, Andreas Schwab wrote:
> > Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> > > I agree, this commonly happens, I do not know what is the right way to do with it.
> > 
> > M-q (c-fill-paragraph)
> 
> What does it do?  I use VIM.

See, there is your problem ;).

Seriosuly though, given that people use different tools some
reformatting of comments simply can't be avoided.  If I make changes
to a comment I'll make some effort to minimize the diff, but if the
changes are in the middle of the comment, or if the end-result simply
looks wrong I'll simply hit M-q and be done with it.  And */ on a line
of its own will almost certainly make me hit M-q.  Which is why I
brought this up.

In this case I'd simply move the last word of the previous line to the
next line to avoid the dangling */.

Cheers,

Mark

Re: comments formatting [Re: [patch] physname regression: Non-matching type false breakpoint]

[Alfred M. Szmidt]

   >    OK (in VIM it is gq} or gqip ).  I use 79 columns instead of 70.
   >    So in the case of 70 columns what would be the right formatting of:
   > 
   >    /* The strchr check is there if any types were specified verify the single found found parameter types really match those specified by user in COPY.  */

   This is not a quote of my text, I wrote:
	/* The strchr check is there if any verify the single found found parameter foo types really match those specified by user in COPY.  */

It (c-fill-paragraph) will reformat it correctly, comment starting at
column 0:

/* The strchr check is there if any verify the single found found
   parameter foo types really match those specified by user in
   COPY.  */

Starting at column 8:

        /* The strchr check is there if any verify the single found
           found parameter foo types really match those specified by
           user in COPY.  */


This applies only to the C mode though, since it takes care to look at
how comments start and end; the normal text function, fill-paragraph,
does not do that and pretends that a closing comment signature is just
a normal word.  In which case it would format it like so (column 0):

/* The strchr check is there if any verify the single found found
   parameter foo types really match those specified by user in COPY.
   */

   > It would be the one I produced in the previous email:

   The text I provided was specially crafted to hit the same problem
   with 70 columns as I faced with the 79 columns and as I / others
   will face in the future.

   When the GNU Coding Standard does describe the comments formatting
   rules one has to unfortunately reverse engineer them from Emacs.

I think that the dangling */ on a single line is a cosmetic thing that
hasn't been thought of that much; it looks ugly to have it on a single
line.

Re: [patch] physname regression: Non-matching type false breakpoint

[Pedro Alves]

On Sunday 05 June 2011 16:34:19, Jan Kratochvil wrote:
> +  /* The strchr check is there if any types were specified verify the single
> +     found found parameter types really match those specified by user in COPY.
> +     */

I have trouble parsing this sentence.  The "The strchr check is there"
part doesn't glue nicely with the other part to my ears.  I suggest
dropping it.  Is a comma or colon missing between specified/verify,
perhaps?  Is double-"found" a typo?  Hmm, looking at the code,
how about this alternative comment?

+  /* If we found a single field with that name, and we were not given
+     a specific overload instance in COPY, accept the field, if it's
+     really a method.  */
+  if (i1 == 1 && strchr (copy, '(') == NULL)

-- 
Pedro Alves

Re: [patch] physname regression: Non-matching type false breakpoint

[Jan Kratochvil]

On Mon, 06 Jun 2011 11:21:30 +0200, Pedro Alves wrote:
> I have trouble parsing this sentence.

I agree the original sentence by me was bogus.


> +  /* If we found a single field with that name, and we were not given
> +     a specific overload instance in COPY, accept the field, if it's
> +     really a method.  */
> +  if (i1 == 1 && strchr (copy, '(') == NULL)

The point was IMO not when to accept the field but rather when to reject it
- that an additional comparison is needed.  Adjusted the text.


Thanks,
Jan


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

	* linespec.c (find_method): Accept the function type automatically only
	if it was specified with parameter types.

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

	* gdb.cp/paren-type.cc: New files.
	* gdb.cp/paren-type.exp: New files.

--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -1697,7 +1697,10 @@ find_method (int funfirstline, struct linespec_result *canonical,
   i1 = find_methods (t, copy, SYMBOL_LANGUAGE (sym_class), sym_arr,
 		     file_symtab);
 
-  if (i1 == 1)
+  /* If we were given a specific overload instance in COPY defer the field
+     acceptance till the strcmp_iw verification below even if we found just
+     a single field with that name.  */
+  if (i1 == 1 && strchr (copy, '(') == NULL)
     {
       /* There is exactly one field with that name.  */
       sym = sym_arr[0];
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/paren-type.cc
@@ -0,0 +1,33 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+class C
+{
+public:
+  static void f (short x);
+};
+
+void
+C::f (short x)
+{
+}
+
+int
+main ()
+{
+  C::f(1);
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/paren-type.exp
@@ -0,0 +1,24 @@
+# Copyright (C) 2011 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set testfile paren-type
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${testfile}.cc {c++ debug}] } {
+    return -1
+}
+
+gdb_test "break C::f(long)" {the class `C' does not have any method instance named f\(long\)}
+
+# Sanity check the breakpoints work for the correct type.
+gdb_test "break C::f(short)" "Breakpoint 1 at \[^\r\n\]*"

Re: [patch] physname regression: Non-matching type false breakpoint

[Pedro Alves]

On Monday 06 June 2011 22:19:43, Jan Kratochvil wrote:
> On Mon, 06 Jun 2011 11:21:30 +0200, Pedro Alves wrote:
> > I have trouble parsing this sentence.
> 
> I agree the original sentence by me was bogus.
> 
> 
> > +  /* If we found a single field with that name, and we were not given
> > +     a specific overload instance in COPY, accept the field, if it's
> > +     really a method.  */
> > +  if (i1 == 1 && strchr (copy, '(') == NULL)
> 
> The point was IMO not when to accept the field but rather when to reject it
> - that an additional comparison is needed.  Adjusted the text.

That justifies the patch/change, but looking at the resulting code, it
looked to me a bit better to justify when can the i1 == 1 special case
be applied (the code directly under the comment).
Anyway, glass half full vs glass half empty.  :-)  Your version is fine
with me.

> -  if (i1 == 1)
> +  /* If we were given a specific overload instance in COPY defer the field
> +     acceptance till the strcmp_iw verification below even if we found just
> +     a single field with that name.  */

I suggest a comma after "COPY", and another after "below".

-- 
Pedro Alves

[commit] Re: [patch] physname regression: Non-matching type false breakpoint

[Jan Kratochvil]

On Tue, 07 Jun 2011 18:58:27 +0200, Pedro Alves wrote:
> On Monday 06 June 2011 22:19:43, Jan Kratochvil wrote:
> > -  if (i1 == 1)
> > +  /* If we were given a specific overload instance in COPY defer the field
> > +     acceptance till the strcmp_iw verification below even if we found just
> > +     a single field with that name.  */
> 
> I suggest a comma after "COPY", and another after "below".

Done.

Checked in.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2011-07/msg00031.html

--- src/gdb/ChangeLog	2011/07/01 20:18:59	1.13153
+++ src/gdb/ChangeLog	2011/07/01 20:27:58	1.13154
@@ -1,5 +1,10 @@
 2011-07-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
+	* linespec.c (find_method): Accept the function type automatically only
+	if it was specified with parameter types.
+
+2011-07-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
 	Stop on first linespec terminator instead of eating what we can.
 	* linespec.c (is_linespec_boundary): New function.
 	(name_end): Remove function.
--- src/gdb/linespec.c	2011/07/01 20:19:03	1.124
+++ src/gdb/linespec.c	2011/07/01 20:27:58	1.125
@@ -1697,7 +1697,10 @@
   i1 = find_methods (t, copy, SYMBOL_LANGUAGE (sym_class), sym_arr,
 		     file_symtab);
 
-  if (i1 == 1)
+  /* If we were given a specific overload instance in COPY, defer the field
+     acceptance till the strcmp_iw verification below, even if we found just
+     a single field with that name.  */
+  if (i1 == 1 && strchr (copy, '(') == NULL)
     {
       /* There is exactly one field with that name.  */
       sym = sym_arr[0];
--- src/gdb/testsuite/ChangeLog	2011/07/01 20:19:03	1.2776
+++ src/gdb/testsuite/ChangeLog	2011/07/01 20:27:59	1.2777
@@ -1,5 +1,10 @@
 2011-07-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
+	* gdb.cp/paren-type.cc: New files.
+	* gdb.cp/paren-type.exp: New files.
+
+2011-07-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
 	Stop on first linespec terminator instead of eating what we can.
 	* gdb.cp/minsym-fallback-main.cc (main): Call also C::operator ().
 	* gdb.cp/minsym-fallback.cc (C::operator ()): Define.
--- src/gdb/testsuite/gdb.cp/paren-type.cc
+++ src/gdb/testsuite/gdb.cp/paren-type.cc	2011-07-01 20:28:47.786329000 +0000
@@ -0,0 +1,33 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+class C
+{
+public:
+  static void f (short x);
+};
+
+void
+C::f (short x)
+{
+}
+
+int
+main ()
+{
+  C::f(1);
+}
--- src/gdb/testsuite/gdb.cp/paren-type.exp
+++ src/gdb/testsuite/gdb.cp/paren-type.exp	2011-07-01 20:28:48.069315000 +0000
@@ -0,0 +1,26 @@
+# Copyright (C) 2011 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set testfile paren-type
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${testfile}.cc {c++ debug}] } {
+    return -1
+}
+
+gdb_test_no_output "set breakpoint pending off"
+
+gdb_test "break C::f(long)" {Function "C::f\(long\)" not defined\.}
+
+# Sanity check the breakpoints work for the correct type.
+gdb_test "break C::f(short)" "Breakpoint 1 at \[^\r\n\]*"