[PATCH v2] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[PATCH v2] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Enze Li]

The clear command shouldn't delete momentary and internal breakpoints,
nor internal breakpoints created via Python's gdb.Breakpoint.

This patch fixes this issue and adds a testcase.

Regression tested on x86_64 openSUSE Tumbleweed.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=7161
---
 gdb/breakpoint.c                  |  3 +-
 gdb/testsuite/gdb.base/pr7161.c   | 25 +++++++++++++
 gdb/testsuite/gdb.base/pr7161.exp | 59 +++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/pr7161.c
 create mode 100644 gdb/testsuite/gdb.base/pr7161.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index a3cfeea6989..ee1ce793593 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10979,7 +10979,8 @@ clear_command (const char *arg, int from_tty)
 	{
 	  int match = 0;
 	  /* Are we going to delete b?  */
-	  if (b->type != bp_none && !is_watchpoint (b))
+	  if (b->type != bp_none && !is_watchpoint (b)
+	      && user_breakpoint_p (b))
 	    {
 	      for (bp_location *loc : b->locations ())
 		{
diff --git a/gdb/testsuite/gdb.base/pr7161.c b/gdb/testsuite/gdb.base/pr7161.c
new file mode 100644
index 00000000000..b5a3a8ccb30
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr7161.c
@@ -0,0 +1,25 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2022 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/>.  */
+
+#include <stdio.h>
+
+int
+main ()
+{
+  printf("Hello,world!\n"); /* break here */
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/pr7161.exp b/gdb/testsuite/gdb.base/pr7161.exp
new file mode 100644
index 00000000000..533e61d1795
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr7161.exp
@@ -0,0 +1,59 @@
+# Copyright 2022 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/>.
+
+proc get_maint_info_bp { var } {
+    global expect_out
+    global gdb_prompt
+
+    gdb_test_multiple "maint info break -1" "find address of internal bp $var" {
+	-re ".*(0x\[0-9a-f\]+).*$gdb_prompt $" {
+            return $expect_out(1,string)
+	}
+        timeout { 
+	    perror "couldn't find address of $var"
+	    return ""
+        }
+    }
+    return ""
+}
+
+standard_testfile .c
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested "failed to compile"
+     return -1
+}
+
+# Start with a fresh gdb.
+clean_restart ${binfile}
+
+if ![runto_main] then {
+    return 0
+}
+
+gdb_test "break [gdb_get_line_number "break here"]" \
+	".*Breakpoint.* at .*" \
+	"set breakpoint"
+
+set bp_addr [get_maint_info_bp "-1"]
+
+gdb_test "maint info break -1" \
+    "-1.*shlib events.*keep y.*$bp_addr.*" \
+    "maint info breakpoint -1 error"
+
+gdb_test "clear *$bp_addr" \
+    "No breakpoint at \\*$bp_addr." \
+    "clear internal breakpoint error"
+
-- 
2.35.1

Re: [PATCH v2] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Enze Li]

Kindly Ping.

The previous version is here,
https://sourceware.org/pipermail/gdb-patches/2022-March/186607.html

Thanks,
Enze

On Thu, 2022-03-17 at 22:24 +0800, Enze Li via Gdb-patches wrote:
> The clear command shouldn't delete momentary and internal
> breakpoints,
> nor internal breakpoints created via Python's gdb.Breakpoint.
> 
> This patch fixes this issue and adds a testcase.
> 
> Regression tested on x86_64 openSUSE Tumbleweed.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=7161
> ---
>  gdb/breakpoint.c                  |  3 +-
>  gdb/testsuite/gdb.base/pr7161.c   | 25 +++++++++++++
>  gdb/testsuite/gdb.base/pr7161.exp | 59
> +++++++++++++++++++++++++++++++
>  3 files changed, 86 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.base/pr7161.c
>  create mode 100644 gdb/testsuite/gdb.base/pr7161.exp
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index a3cfeea6989..ee1ce793593 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -10979,7 +10979,8 @@ clear_command (const char *arg, int from_tty)
>         {
>           int match = 0;
>           /* Are we going to delete b?  */
> -         if (b->type != bp_none && !is_watchpoint (b))
> +         if (b->type != bp_none && !is_watchpoint (b)
> +             && user_breakpoint_p (b))
>             {
>               for (bp_location *loc : b->locations ())
>                 {
> diff --git a/gdb/testsuite/gdb.base/pr7161.c
> b/gdb/testsuite/gdb.base/pr7161.c
> new file mode 100644
> index 00000000000..b5a3a8ccb30
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/pr7161.c
> @@ -0,0 +1,25 @@
> +/* This test is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 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/>.  */
> +
> +#include <stdio.h>
> +
> +int
> +main ()
> +{
> +  printf("Hello,world!\n"); /* break here */
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/pr7161.exp
> b/gdb/testsuite/gdb.base/pr7161.exp
> new file mode 100644
> index 00000000000..533e61d1795
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/pr7161.exp
> @@ -0,0 +1,59 @@
> +# Copyright 2022 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/>.
> +
> +proc get_maint_info_bp { var } {
> +    global expect_out
> +    global gdb_prompt
> +
> +    gdb_test_multiple "maint info break -1" "find address of
> internal bp $var" {
> +       -re ".*(0x\[0-9a-f\]+).*$gdb_prompt $" {
> +            return $expect_out(1,string)
> +       }
> +        timeout { 
> +           perror "couldn't find address of $var"
> +           return ""
> +        }
> +    }
> +    return ""
> +}
> +
> +standard_testfile .c
> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
> executable {debug}] != "" } {
> +     untested "failed to compile"
> +     return -1
> +}
> +
> +# Start with a fresh gdb.
> +clean_restart ${binfile}
> +
> +if ![runto_main] then {
> +    return 0
> +}
> +
> +gdb_test "break [gdb_get_line_number "break here"]" \
> +       ".*Breakpoint.* at .*" \
> +       "set breakpoint"
> +
> +set bp_addr [get_maint_info_bp "-1"]
> +
> +gdb_test "maint info break -1" \
> +    "-1.*shlib events.*keep y.*$bp_addr.*" \
> +    "maint info breakpoint -1 error"
> +
> +gdb_test "clear *$bp_addr" \
> +    "No breakpoint at \\*$bp_addr." \
> +    "clear internal breakpoint error"
> +

[PING^2][PATCH v2] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Enze Li]

Ping^2.  :-)

Thanks,
Enze

On Thu, 2022-03-17 at 22:24 +0800, Enze Li via Gdb-patches wrote:
> The clear command shouldn't delete momentary and internal
> breakpoints,
> nor internal breakpoints created via Python's gdb.Breakpoint.
> 
> This patch fixes this issue and adds a testcase.
> 
> Regression tested on x86_64 openSUSE Tumbleweed.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=7161
> ---
>  gdb/breakpoint.c                  |  3 +-
>  gdb/testsuite/gdb.base/pr7161.c   | 25 +++++++++++++
>  gdb/testsuite/gdb.base/pr7161.exp | 59
> +++++++++++++++++++++++++++++++
>  3 files changed, 86 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.base/pr7161.c
>  create mode 100644 gdb/testsuite/gdb.base/pr7161.exp
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index a3cfeea6989..ee1ce793593 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -10979,7 +10979,8 @@ clear_command (const char *arg, int from_tty)
>  	{
>  	  int match = 0;
>  	  /* Are we going to delete b?  */
> -	  if (b->type != bp_none && !is_watchpoint (b))
> +	  if (b->type != bp_none && !is_watchpoint (b)
> +	      && user_breakpoint_p (b))
>  	    {
>  	      for (bp_location *loc : b->locations ())
>  		{
> diff --git a/gdb/testsuite/gdb.base/pr7161.c
> b/gdb/testsuite/gdb.base/pr7161.c
> new file mode 100644
> index 00000000000..b5a3a8ccb30
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/pr7161.c
> @@ -0,0 +1,25 @@
> +/* This test is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 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/>;.  */
> +
> +#include <stdio.h>
> +
> +int
> +main ()
> +{
> +  printf("Hello,world!\n"); /* break here */
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/pr7161.exp
> b/gdb/testsuite/gdb.base/pr7161.exp
> new file mode 100644
> index 00000000000..533e61d1795
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/pr7161.exp
> @@ -0,0 +1,59 @@
> +# Copyright 2022 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/>;.
> +
> +proc get_maint_info_bp { var } {
> +    global expect_out
> +    global gdb_prompt
> +
> +    gdb_test_multiple "maint info break -1" "find address of
> internal bp $var" {
> +	-re ".*(0x\[0-9a-f\]+).*$gdb_prompt $" {
> +            return $expect_out(1,string)
> +	}
> +        timeout { 
> +	    perror "couldn't find address of $var"
> +	    return ""
> +        }
> +    }
> +    return ""
> +}
> +
> +standard_testfile .c
> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
> executable {debug}] != "" } {
> +     untested "failed to compile"
> +     return -1
> +}
> +
> +# Start with a fresh gdb.
> +clean_restart ${binfile}
> +
> +if ![runto_main] then {
> +    return 0
> +}
> +
> +gdb_test "break [gdb_get_line_number "break here"]" \
> +	".*Breakpoint.* at .*" \
> +	"set breakpoint"
> +
> +set bp_addr [get_maint_info_bp "-1"]
> +
> +gdb_test "maint info break -1" \
> +    "-1.*shlib events.*keep y.*$bp_addr.*" \
> +    "maint info breakpoint -1 error"
> +
> +gdb_test "clear *$bp_addr" \
> +    "No breakpoint at \\*$bp_addr." \
> +    "clear internal breakpoint error"
> +

Re: [PATCH v2] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Tom Tromey]

>>>>> Enze Li via Gdb-patches <gdb-patches@sourceware.org> writes:

> The clear command shouldn't delete momentary and internal breakpoints,
> nor internal breakpoints created via Python's gdb.Breakpoint.

> This patch fixes this issue and adds a testcase.

Thank you for the patch.

> +int
> +main ()
> +{
> +  printf("Hello,world!\n"); /* break here */
> +  return 0;
> +}

There's may be an existing main.c you can just reuse instead of adding a
new file.

> diff --git a/gdb/testsuite/gdb.base/pr7161.exp b/gdb/testsuite/gdb.base/pr7161.exp

We normally don't name tests after bugs, but instead give them a
descriptive name.

Tom

[PATCH v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Enze Li]

The clear command shouldn't delete momentary and internal breakpoints,
nor internal breakpoints created via Python's gdb.Breakpoint.

This patch fixes this issue and adds a testcase.

Regression tested on x86_64 openSUSE Tumbleweed(VERSION_ID="20220413").

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=7161
---
 gdb/breakpoint.c                             |  3 +-
 gdb/testsuite/gdb.base/clear_non_user_bp.exp | 64 ++++++++++++++++++++
 2 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/clear_non_user_bp.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1a227e5d120..ff27174b91f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10921,7 +10921,8 @@ clear_command (const char *arg, int from_tty)
 	{
 	  int match = 0;
 	  /* Are we going to delete b?  */
-	  if (b->type != bp_none && !is_watchpoint (b))
+	  if (b->type != bp_none && !is_watchpoint (b)
+	      && user_breakpoint_p (b))
 	    {
 	      for (bp_location *loc : b->locations ())
 		{
diff --git a/gdb/testsuite/gdb.base/clear_non_user_bp.exp b/gdb/testsuite/gdb.base/clear_non_user_bp.exp
new file mode 100644
index 00000000000..4f0731e06f6
--- /dev/null
+++ b/gdb/testsuite/gdb.base/clear_non_user_bp.exp
@@ -0,0 +1,64 @@
+# Copyright 2022 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/>.
+
+# Regression test for PR gdb/7161.  Test that GDB cannot delete non-user
+# breakpoints with clear command.
+
+proc get_maint_info_bp { var } {
+    global expect_out
+    global gdb_prompt
+
+    gdb_test_multiple "maint info break -1" "find address of internal bp $var" {
+	-re ".*(0x\[0-9a-f\]+).*$gdb_prompt $" {
+            return $expect_out(1,string)
+	}
+	timeout {
+	    perror "couldn't find address of $var"
+	    return ""
+	}
+    }
+    return ""
+}
+
+standard_testfile .c
+
+# this testcase just needs a "Hello world" source file, reuse
+# gdb.server/sysroot.c instead of adding a new one.
+if  { [gdb_compile "${srcdir}/gdb.server/sysroot.c" "${binfile}" executable {debug}] != "" } {
+     untested "failed to compile"
+     return -1
+}
+
+# Start with a fresh gdb.
+clean_restart ${binfile}
+
+if ![runto_main] then {
+    return 0
+}
+
+gdb_test "break sysroot.c:23" \
+	".*Breakpoint.* at .*" \
+	"set breakpoint"
+
+set bp_addr [get_maint_info_bp "-1"]
+
+gdb_test "maint info break -1" \
+    "-1.*shlib events.*keep y.*$bp_addr.*" \
+    "maint info breakpoint -1 error"
+
+gdb_test "clear *$bp_addr" \
+    "No breakpoint at \\*$bp_addr." \
+    "clear internal breakpoint error"
+
-- 
2.35.1

[PATCH RESEND v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Enze Li]

The clear command shouldn't delete momentary and internal breakpoints,
nor internal breakpoints created via Python's gdb.Breakpoint.

This patch fixes this issue and adds a testcase.

Regression tested on x86_64 openSUSE Tumbleweed(VERSION_ID="20220413").

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=7161
---
 gdb/breakpoint.c                             |  3 +-
 gdb/testsuite/gdb.base/clear_non_user_bp.exp | 64 ++++++++++++++++++++
 2 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/clear_non_user_bp.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1a227e5d120..ff27174b91f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10921,7 +10921,8 @@ clear_command (const char *arg, int from_tty)
 	{
 	  int match = 0;
 	  /* Are we going to delete b?  */
-	  if (b->type != bp_none && !is_watchpoint (b))
+	  if (b->type != bp_none && !is_watchpoint (b)
+	      && user_breakpoint_p (b))
 	    {
 	      for (bp_location *loc : b->locations ())
 		{
diff --git a/gdb/testsuite/gdb.base/clear_non_user_bp.exp b/gdb/testsuite/gdb.base/clear_non_user_bp.exp
new file mode 100644
index 00000000000..3ac037e8a43
--- /dev/null
+++ b/gdb/testsuite/gdb.base/clear_non_user_bp.exp
@@ -0,0 +1,64 @@
+# Copyright 2022 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/>.
+
+# Regression test for PR gdb/7161.  Test that GDB cannot delete non-user
+# breakpoints with clear command.
+
+proc get_maint_info_bp { var } {
+    global expect_out
+    global gdb_prompt
+
+    gdb_test_multiple "maint info break -1" "find address of internal bp $var" {
+	-re ".*(0x\[0-9a-f\]+).*$gdb_prompt $" {
+	    return $expect_out(1,string)
+	}
+	timeout {
+	    perror "couldn't find address of $var"
+	    return ""
+	}
+    }
+    return ""
+}
+
+standard_testfile .c
+
+# This testcase just needs a "Hello world" source file, reuse
+# gdb.base/main.c instead of adding a new one.
+if  { [gdb_compile "${srcdir}/${subdir}/main.c" "${binfile}" executable {debug}] != "" } {
+    untested "failed to compile"
+    return -1
+}
+
+# Start with a fresh gdb.
+clean_restart ${binfile}
+
+if ![runto_main] then {
+    return 0
+}
+
+gdb_test "break main.c:21" \
+    ".*Breakpoint.* at .*" \
+    "set breakpoint"
+
+set bp_addr [get_maint_info_bp "-1"]
+
+gdb_test "maint info break -1" \
+    "-1.*shlib events.*keep y.*$bp_addr.*" \
+    "maint info breakpoint -1 error"
+
+gdb_test "clear *$bp_addr" \
+    "No breakpoint at \\*$bp_addr." \
+    "clear internal breakpoint error"
+
-- 
2.35.1

Re: [PATCH v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Tom Tromey]

>>>>> Enze Li <lienze2010@hotmail.com> writes:

> The clear command shouldn't delete momentary and internal breakpoints,
> nor internal breakpoints created via Python's gdb.Breakpoint.
> This patch fixes this issue and adds a testcase.
> Regression tested on x86_64 openSUSE Tumbleweed(VERSION_ID="20220413").
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=7161

Thank you.  This looks good to me.

Tom

Re: [PATCH RESEND v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Lancelot SIX]

Hi,

I think Tom already OK-ed this patch, but I have minor comments, if you do not
mind.

> +
> +proc get_maint_info_bp { var } {
> +    global expect_out
> +    global gdb_prompt
> +
> +    gdb_test_multiple "maint info break -1" "find address of internal bp $var" {

In this proc, the `var` param is only used in the messages, not in the
actual command.  Did you mean "maint info break $var" here?

> +	-re ".*(0x\[0-9a-f\]+).*$gdb_prompt $" {
> +	    return $expect_out(1,string)
> +	}
> +	timeout {
> +	    perror "couldn't find address of $var"
> +	    return ""
> +	}
> +    }
> +    return ""
> +}

> +
> +standard_testfile .c
> +
> +# This testcase just needs a "Hello world" source file, reuse
> +# gdb.base/main.c instead of adding a new one.
> +if  { [gdb_compile "${srcdir}/${subdir}/main.c" "${binfile}" executable {debug}] != "" } {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +# Start with a fresh gdb.
> +clean_restart ${binfile}
> +
> +if ![runto_main] then {
> +    return 0
> +}
> +
> +gdb_test "break main.c:21" \
> +    ".*Breakpoint.* at .*" \
> +    "set breakpoint"

If I run this test, I have:

    (gdb) break -qualified main
    Breakpoint 1 at 0x401020: file .../gdb/testsuite/gdb.base/main.c, line 21.
    (gdb) run 
    Starting program: .../gdb/testsuite/outputs/gdb.base/clear_non_user_bp/clear_non_user_bp 
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library ".../lib/libthread_db.so.1".

    Breakpoint 1, main () at .../gdb/testsuite/gdb.base/main.c:21
    21        return 0;
    (gdb) break main.c:21
    Note: breakpoint 1 also set at pc 0x401020.
    Breakpoint 2 at 0x401020: file .../gdb/testsuite/gdb.base/main.c, line 21.
    (gdb) PASS: gdb.base/clear_non_user_bp.exp: set breakpoint

So you already have a user breakpoint inserted (which also happens to be
at the same address as the breakpoint you want to insert).  I am not sure
this adds much to your testcase (but I might be wrong).

Also, I do not know how maintainers feel about this (I am not one), but
I feel that using gdb.base/main.c which is not directly linked to this
testcase makes it easy to have main.c accidentally edited with this file
not being touched.  With a plain line number, we might end up with
unexpected test changes.  I really do not expect this file to change any
time soon, but who knows, an update in the license header might lead to
the line you are looking for not being 21 anymore.

If you really need this breakpoint, I guess I would go for something
like:

    gdb_break [gdb_get_line_number "return 0;"]

> +
> +set bp_addr [get_maint_info_bp "-1"]
> +
> +gdb_test "maint info break -1" \
> +    "-1.*shlib events.*keep y.*$bp_addr.*" \
> +    "maint info breakpoint -1 error"

The last param is the name of the test, not an error message.  If the
test was to fail, the output would be:

    FAIL: main info breakpoint -1 error

"FAIL" and "error" would be somewhat redundant.  I would suggest
dropping the last argument altogether since, if missing, the framework
uses the 1st parameter (the command) as default value.

Also, I feel that both main

> +
> +gdb_test "clear *$bp_addr" \
> +    "No breakpoint at \\*$bp_addr." \
> +    "clear internal breakpoint error"

Here, I would suggest just dropping the " error" part of the testname.
"$bp_addr" is likely to change from one configuration to another,
making diffing .sum files difficult.

Best,
Lancelot.

Re: [PATCH v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Pedro Alves]

On 2022-04-16 04:42, Enze Li via Gdb-patches wrote:

> diff --git a/gdb/testsuite/gdb.base/clear_non_user_bp.exp b/gdb/testsuite/gdb.base/clear_non_user_bp.exp
> new file mode 100644
> index 00000000000..4f0731e06f6
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/clear_non_user_bp.exp
> @@ -0,0 +1,64 @@
> +# Copyright 2022 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/>.
> +
> +# Regression test for PR gdb/7161.  Test that GDB cannot delete non-user
> +# breakpoints with clear command.
> +
> +proc get_maint_info_bp { var } {
> +    global expect_out
> +    global gdb_prompt
> +
> +    gdb_test_multiple "maint info break -1" "find address of internal bp $var" {
> +	-re ".*(0x\[0-9a-f\]+).*$gdb_prompt $" {
> +            return $expect_out(1,string)
> +	}
> +	timeout {
> +	    perror "couldn't find address of $var"
> +	    return ""
> +	}

You shouldn't need a timeout match, gdb_test_multiple already has one internally.

> +    }
> +    return ""
> +}
> +
> +standard_testfile .c
> +
> +# this testcase just needs a "Hello world" source file, reuse

Uppercase "this" -> "This".

> +# gdb.server/sysroot.c instead of adding a new one.
> +if  { [gdb_compile "${srcdir}/gdb.server/sysroot.c" "${binfile}" executable {debug}] != "" } {
> +     untested "failed to compile"
> +     return -1
> +}
> +
> +# Start with a fresh gdb.
> +clean_restart ${binfile}
> +
> +if ![runto_main] then {
> +    return 0
> +}
> +
> +gdb_test "break sysroot.c:23" \
> +	".*Breakpoint.* at .*" \
> +	"set breakpoint"
> +
> +set bp_addr [get_maint_info_bp "-1"]
> +

It's not portable to assume that breakpoint -1 is the shlib_events breakpoint.
Some targets don't even use such a breakpoint.  E.g., Windows.  It would be
better to instead make get_maint_info_bp look for the first internal breakpoint and
return its number.

> +gdb_test "maint info break -1" \
> +    "-1.*shlib events.*keep y.*$bp_addr.*" \
> +    "maint info breakpoint -1 error"

Why is this test called "... error" ?

> +
> +gdb_test "clear *$bp_addr" \
> +    "No breakpoint at \\*$bp_addr." \
> +    "clear internal breakpoint error"
> +

It would be good to issue the "maint info break" command again after the clear,
to make sure that GDB really didn't delete the breakpoint, even if the clear command
errored out.

Not a strong requirement, but it would be great if we also had a test trying to clear an internal
python breakpoint.  Did you look into adding that?  If it's too complicated to add it here or to
a new file, maybe add a "clear" test to gdb.python/py-breakpoint.exp ?

Re: [PATCH v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Tom Tromey]

>>>>> Enze Li via Gdb-patches <gdb-patches@sourceware.org> writes:

> diff --git a/gdb/testsuite/gdb.base/clear_non_user_bp.exp b/gdb/testsuite/gdb.base/clear_non_user_bp.exp

Unfortunately this fails for me:

FAIL: gdb.base/clear_non_user_bp.exp: maint info breakpoint -1 error

Tom

Re: [PATCH v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Simon Marchi]

On 2022-04-15 23:42, Enze Li via Gdb-patches wrote:
> The clear command shouldn't delete momentary and internal breakpoints,
> nor internal breakpoints created via Python's gdb.Breakpoint.
> 
> This patch fixes this issue and adds a testcase.
> 
> Regression tested on x86_64 openSUSE Tumbleweed(VERSION_ID="20220413").
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=7161

Hi,

I see this test failing on Ubuntu 20.04:

 65 maint info break -1^M
 66 Num     Type           Disp Enb Address            What^M
 67 -1      shlib events   keep n   0x00007ffff7fd2fd1 <dl_main+6641> inf 1^M
 68 -1.1                        y   0x00007ffff7fd2fd1 <dl_main+6641> inf 1^M
 69 (gdb) maint info break -1^M
 70 Num     Type           Disp Enb Address            What^M
 71 -1      shlib events   keep n   0x00007ffff7fd2fd1 <dl_main+6641> inf 1^M
 72 -1.1                        y   0x00007ffff7fd2fd1 <dl_main+6641> inf 1^M
 73 (gdb) FAIL: gdb.base/clear_non_user_bp.exp: maint info breakpoint -1 error

Simon

Re: [PATCH v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Enze Li]

Oops!  I've already pushed it, should I revert it first?


On Mon, 2022-04-18 at 10:12 -0600, Tom Tromey wrote:
> > > > > > Enze Li via Gdb-patches <gdb-patches@sourceware.org>
> > > > > > writes:
> 
> > diff --git a/gdb/testsuite/gdb.base/clear_non_user_bp.exp
> > b/gdb/testsuite/gdb.base/clear_non_user_bp.exp
> 
> Unfortunately this fails for me:
> 
> FAIL: gdb.base/clear_non_user_bp.exp: maint info breakpoint -1 error
> 
> Tom

Re: [PATCH v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Tom Tromey]

>>>>> Enze Li <lienze2010@hotmail.com> writes:

> Oops!  I've already pushed it, should I revert it first?

If you can fix it, it's fine to just move forward and not do a backout.
I tend to reserve backouts for situations where the bug is in gdb itself
(and not just bug in the test, as this appears to be), and a fix isn't
so easy to come up with.

Tom

Re: [PATCH v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Simon Marchi]

On 2022-04-18 19:49, Simon Marchi via Gdb-patches wrote:
> 
> 
> On 2022-04-15 23:42, Enze Li via Gdb-patches wrote:
>> The clear command shouldn't delete momentary and internal breakpoints,
>> nor internal breakpoints created via Python's gdb.Breakpoint.
>>
>> This patch fixes this issue and adds a testcase.
>>
>> Regression tested on x86_64 openSUSE Tumbleweed(VERSION_ID="20220413").
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=7161
> 
> Hi,
> 
> I see this test failing on Ubuntu 20.04:
> 
>  65 maint info break -1^M
>  66 Num     Type           Disp Enb Address            What^M
>  67 -1      shlib events   keep n   0x00007ffff7fd2fd1 <dl_main+6641> inf 1^M
>  68 -1.1                        y   0x00007ffff7fd2fd1 <dl_main+6641> inf 1^M
>  69 (gdb) maint info break -1^M
>  70 Num     Type           Disp Enb Address            What^M
>  71 -1      shlib events   keep n   0x00007ffff7fd2fd1 <dl_main+6641> inf 1^M
>  72 -1.1                        y   0x00007ffff7fd2fd1 <dl_main+6641> inf 1^M
>  73 (gdb) FAIL: gdb.base/clear_non_user_bp.exp: maint info breakpoint -1 error
> 
> Simon

Sorry for the noise, I didn't see that Tom had already reported that failure.

Simon

Re: [PATCH v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Enze Li]

On Mon, 2022-04-18 at 23:37 -0400, Simon Marchi wrote:
> 
> 
> On 2022-04-18 19:49, Simon Marchi via Gdb-patches wrote:
> > 
> > 
> > On 2022-04-15 23:42, Enze Li via Gdb-patches wrote:
> > > The clear command shouldn't delete momentary and internal
> > > breakpoints,
> > > nor internal breakpoints created via Python's gdb.Breakpoint.
> > > 
> > > This patch fixes this issue and adds a testcase.
> > > 
> > > Regression tested on x86_64 openSUSE
> > > Tumbleweed(VERSION_ID="20220413").
> > > 
> > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=7161
> > 
> > Hi,
> > 
> > I see this test failing on Ubuntu 20.04:
> > 
> >  65 maint info break -1^M
> >  66 Num     Type           Disp Enb Address            What^M
> >  67 -1      shlib events   keep n   0x00007ffff7fd2fd1
> > <dl_main+6641> inf 1^M
> >  68 -1.1                        y   0x00007ffff7fd2fd1
> > <dl_main+6641> inf 1^M
> >  69 (gdb) maint info break -1^M
> >  70 Num     Type           Disp Enb Address            What^M
> >  71 -1      shlib events   keep n   0x00007ffff7fd2fd1
> > <dl_main+6641> inf 1^M
> >  72 -1.1                        y   0x00007ffff7fd2fd1
> > <dl_main+6641> inf 1^M
> >  73 (gdb) FAIL: gdb.base/clear_non_user_bp.exp: maint info
> > breakpoint -1 error
> > 
> > Simon
> 
> Sorry for the noise, I didn't see that Tom had already reported that
> failure.
> 
> Simon

Hi Simon,

Any feedback is welcome.  I reproduced the issue on Ubuntu 20.04 you
reported to me.  Thanks a lot.  :)

Enze

Re: [PATCH v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Enze Li]

Hi Pedro,

Thank you for the review.

On Mon, 2022-04-18 at 17:04 +0100, Pedro Alves wrote:
> On 2022-04-16 04:42, Enze Li via Gdb-patches wrote:
> 

> 
<snip>

> > +proc get_maint_info_bp { var } {
> > +    global expect_out
> > +    global gdb_prompt
> > +
> > +    gdb_test_multiple "maint info break -1" "find address of
> > internal bp $var" {
> > +       -re ".*(0x\[0-9a-f\]+).*$gdb_prompt $" {
> > +            return $expect_out(1,string)
> > +       }
> > +       timeout {
> > +           perror "couldn't find address of $var"
> > +           return ""
> > +       }
> 
> You shouldn't need a timeout match, gdb_test_multiple already has one
> internally.

Fixed.

> 
> > +    }
> > +    return ""
> > +}
> > +
> > +standard_testfile .c
> > +
> > +# this testcase just needs a "Hello world" source file, reuse
> 
> Uppercase "this" -> "This".
> 

Fixed.

> > +# gdb.server/sysroot.c instead of adding a new one.
> > +if  { [gdb_compile "${srcdir}/gdb.server/sysroot.c" "${binfile}"
> > executable {debug}] != "" } {
> > +     untested "failed to compile"
> > +     return -1
> > +}
> > +
> > +# Start with a fresh gdb.
> > +clean_restart ${binfile}
> > +
> > +if ![runto_main] then {
> > +    return 0
> > +}
> > +
> > +gdb_test "break sysroot.c:23" \
> > +       ".*Breakpoint.* at .*" \
> > +       "set breakpoint"
> > +
> > +set bp_addr [get_maint_info_bp "-1"]
> > +
> 
> It's not portable to assume that breakpoint -1 is the shlib_events
> breakpoint.
> Some targets don't even use such a breakpoint.  E.g., Windows.  It
> would be
> better to instead make get_maint_info_bp look for the first internal
> breakpoint and
> return its number.
> 

Thanks, this helps me a lot.

> > +gdb_test "maint info break -1" \
> > +    "-1.*shlib events.*keep y.*$bp_addr.*" \
> > +    "maint info breakpoint -1 error"
> 
> Why is this test called "... error" ?

Drop the word "error"? Done.

> > +
> > +gdb_test "clear *$bp_addr" \
> > +    "No breakpoint at \\*$bp_addr." \
> > +    "clear internal breakpoint error"
> > +
> 
> It would be good to issue the "maint info break" command again after
> the clear,
> to make sure that GDB really didn't delete the breakpoint, even if
> the clear command
> errored out.

Fixed.

> 
> Not a strong requirement, but it would be great if we also had a test
> trying to clear an internal
> python breakpoint.  Did you look into adding that?  If it's too
> complicated to add it here or to
> a new file, maybe add a "clear" test to gdb.python/py-breakpoint.exp
> ?

If this is a requirement, I'd like to implement it.  Is this something
that can be done later?

I've sent a fix patch to the gdb-patch@list and I posted it here:
https://sourceware.org/pipermail/gdb-patches/2022-April/188044.html


Thanks,
Enze

Re: [PATCH RESEND v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Enze Li]

Hi Lancelot,

Thank you for the review.

I'm very sorry for the long wait, I dunno what's going on with my
email.  I didn't receive this email from you and gdb-patches@list until
I happened to see your reply on the gdb-patches web page[1].  I copied
the content from the web page and replied to you here, please see below.

[1] https://sourceware.org/pipermail/gdb-patches/2022-April/187959.html

>Lancelot SIX lsix@lancelotsix.com
>Sun Apr 17 22:29:29 GMT 2022
>
>Hi,
>
>I think Tom already OK-ed this patch, but I have minor comments, if
>you do not mind.
>
>> +
>> +proc get_maint_info_bp { var } {
>> +    global expect_out
>> +    global gdb_prompt
>> +
>> +    gdb_test_multiple "maint info break -1" "find address of
>> internal bp $var" {
>
>In this proc, the `var` param is only used in the messages, not in the
>actual command.  Did you mean "maint info break $var" here?
>

Yes, I didn't notice the problem at the time.

>> +	-re ".*(0x\[0-9a-f\]+).*$gdb_prompt $" {
>> +	    return $expect_out(1,string)
>> +	}
>> +	timeout {
>> +	    perror "couldn't find address of $var"
>> +	    return ""
>> +	}
>> +    }
>> +    return ""
>> +}
>
>> +
>> +standard_testfile .c
>> +
>> +# This testcase just needs a "Hello world" source file, reuse
>> +# gdb.base/main.c instead of adding a new one.
>> +if  { [gdb_compile "${srcdir}/${subdir}/main.c" "${binfile}"
>> executable {debug}] != "" } {
>> +    untested "failed to compile"
>> +    return -1
>> +}
>> +
>> +# Start with a fresh gdb.
>> +clean_restart ${binfile}
>> +
>> +if ![runto_main] then {
>> +    return 0
>> +}
>> +
>> +gdb_test "break main.c:21" \
>> +    ".*Breakpoint.* at .*" \
>> +    "set breakpoint"
>
>If I run this test, I have:
>
>    (gdb) break -qualified main
>    Breakpoint 1 at 0x401020: file .../gdb/testsuite/gdb.base/main.c,
> line 21. (gdb) run 
>    Starting program:
> .../gdb/testsuite/outputs/gdb.base/clear_non_user_bp/clear_non_user_bp
> [Thread debugging using libthread_db enabled] Using host libthread_db
> library ".../lib/libthread_db.so.1".
>
>    Breakpoint 1, main () at .../gdb/testsuite/gdb.base/main.c:21
>    21        return 0;
>    (gdb) break main.c:21
>    Note: breakpoint 1 also set at pc 0x401020.
>    Breakpoint 2 at 0x401020: file .../gdb/testsuite/gdb.base/main.c,
> line 21. (gdb) PASS: gdb.base/clear_non_user_bp.exp: set breakpoint
>
>So you already have a user breakpoint inserted (which also happens to
>be at the same address as the breakpoint you want to insert).  I am
>not sure this adds much to your testcase (but I might be wrong).
>
>Also, I do not know how maintainers feel about this (I am not one), but
>I feel that using gdb.base/main.c which is not directly linked to this
>testcase makes it easy to have main.c accidentally edited with this
>file not being touched.  With a plain line number, we might end up with
>unexpected test changes.  I really do not expect this file to change
>any time soon, but who knows, an update in the license header might
>lead to the line you are looking for not being 21 anymore.
>
>If you really need this breakpoint, I guess I would go for something
>like:
>
>    gdb_break [gdb_get_line_number "return 0;"]
>

I also noticed this "Note" message a few hours ago, and I was thinking
of deleting this piece of code, which is actually possible.

>> +
>> +set bp_addr [get_maint_info_bp "-1"]
>> +
>> +gdb_test "maint info break -1" \
>> +    "-1.*shlib events.*keep y.*$bp_addr.*" \
>> +    "maint info breakpoint -1 error"
>
>The last param is the name of the test, not an error message.  If the
>test was to fail, the output would be:
>
>    FAIL: main info breakpoint -1 error
>
>"FAIL" and "error" would be somewhat redundant.  I would suggest
>dropping the last argument altogether since, if missing, the framework
>uses the 1st parameter (the command) as default value.
>
>Also, I feel that both main
>
>> +
>> +gdb_test "clear *$bp_addr" \
>> +    "No breakpoint at \\*$bp_addr." \
>> +    "clear internal breakpoint error"
>
>Here, I would suggest just dropping the " error" part of the testname.
>"$bp_addr" is likely to change from one configuration to another,
>making diffing .sum files difficult.
>

Thank you so much for providing such detailed instructions and advice,
it was really helpful.  I'll cc you when the next version is ready if
you don't mind.

Thanks again,
Enze

Re: [PATCH v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Pedro Alves]

On 2022-04-19 15:07, Enze Li wrote:
>> Not a strong requirement, but it would be great if we also had a test
>> trying to clear an internal
>> python breakpoint.  Did you look into adding that?  If it's too
>> complicated to add it here or to
>> a new file, maybe add a "clear" test to gdb.python/py-breakpoint.exp
>> ?
> If this is a requirement, I'd like to implement it.  Is this something
> that can be done later?

Yes, certainly.

> 
> I've sent a fix patch to the gdb-patch@list and I posted it here:
> https://sourceware.org/pipermail/gdb-patches/2022-April/188044.html