[PATCH] gdb/breakpoint: make a copy of the "commands" command's argument

[PATCH] gdb/breakpoint: make a copy of the "commands" command's argument

[Tankut Baris Aktemur]

When GDB reads commands from the input, its internal buffer is re-used
for each line.  This is usually just fine because commands are
executed in order; by the time we read the next line, we are already
done with the current line.  However, a problematic case is breakpoint
commands that are input from a script.  The header (e.g. commands 1 2)
is overwritten with the next line before the breakpoint numbers are
processed completely.

For example, suppose we have the following script:

  break main
  break main
  commands 1 2
    print 100123
  end

and source this script:

  (gdb) source script.gdb
  Breakpoint 1 at 0x1245: file main.cpp, line 27.
  Breakpoint 2 at 0x1245: file main.cpp, line 27.
  No breakpoint number 123.

Note the "No breakpoint number 123." error message.  This happens
because GDB first reads "commands 1 2" into its internal buffer

  buffer -> "commands 1 2"

and then starts parsing the breakpoint numbers.  After parsing the first
token, the "next token" pointer is as below:

  buffer -> "commands 1 2"
  next-token -----------^

So, if we continue parsing, we would tokenize "2" correctly.  However,
before parsing the next number, GDB reads the commands to attach them
to breakpoint 1.  Reading the commands causes the buffer to be
overwritten:

  buffer -> "  print 100123"
  next-token -----------^

So, the next time we parse the breakpoint number, we read "123".

To fix, simply create a copy of the arguments of the header.

gdb/ChangeLog:
2020-09-11  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* breakpoint.c (commands_command_1): Make a copy of the 'arg'
	argument.

gdb/testsuite/ChangeLog:
2020-09-11  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* gdb.base/bp-cmds-sourced-script.c: New file.
	* gdb.base/bp-cmds-sourced-script.exp: New test.
	* gdb.base/bp-cmds-sourced-script.gdb: New file.

Signed-off-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
---
 gdb/breakpoint.c                              | 12 +++++-
 .../gdb.base/bp-cmds-sourced-script.c         | 22 +++++++++++
 .../gdb.base/bp-cmds-sourced-script.exp       | 39 +++++++++++++++++++
 .../gdb.base/bp-cmds-sourced-script.gdb       | 20 ++++++++++
 4 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/bp-cmds-sourced-script.c
 create mode 100644 gdb/testsuite/gdb.base/bp-cmds-sourced-script.exp
 create mode 100644 gdb/testsuite/gdb.base/bp-cmds-sourced-script.gdb

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index fff80ff907e..3fb259ff409 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1229,13 +1229,23 @@ commands_command_1 (const char *arg, int from_tty,
 
   if (arg == NULL || !*arg)
     {
+      /* Argument not explicitly given.  Synthesize it.  */
       if (breakpoint_count - prev_breakpoint_count > 1)
 	new_arg = string_printf ("%d-%d", prev_breakpoint_count + 1,
 				 breakpoint_count);
       else if (breakpoint_count > 0)
 	new_arg = string_printf ("%d", breakpoint_count);
-      arg = new_arg.c_str ();
     }
+  else
+    {
+      /* Create a copy of ARG.  This is needed because the "commands"
+	 command may be coming from a script.  In that case, the read
+	 line buffer is going to be overwritten in the lambda of
+	 'map_breakpoint_numbers' below when reading the next line
+	 before we are are done parsing the breakpoint numbers.  */
+      new_arg = arg;
+    }
+  arg = new_arg.c_str ();
 
   map_breakpoint_numbers
     (arg, [&] (breakpoint *b)
diff --git a/gdb/testsuite/gdb.base/bp-cmds-sourced-script.c b/gdb/testsuite/gdb.base/bp-cmds-sourced-script.c
new file mode 100644
index 00000000000..f4825c8a7c1
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bp-cmds-sourced-script.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 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/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/bp-cmds-sourced-script.exp b/gdb/testsuite/gdb.base/bp-cmds-sourced-script.exp
new file mode 100644
index 00000000000..db551f19180
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bp-cmds-sourced-script.exp
@@ -0,0 +1,39 @@
+# Copyright 2020 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/>.  */
+
+# Test that breakpoint commands entered in a GDB script work as
+# expected when the commands are defined for multiple breakpoints.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+set script_file ${srcdir}/${subdir}/$testfile.gdb
+
+gdb_test "source $script_file" \
+    "Breakpoint 1\[^\r\n\]*\r\nBreakpoint 2\[^\r\n\]*" \
+    "source the script"
+
+gdb_run_cmd
+
+gdb_test_multiple "" "commands executed twice" {
+    -re "\\$${decimal} = 100123\r\n\\$${decimal} = 100123\r\n$gdb_prompt $" {
+	pass $gdb_test_name
+    }
+}
+
+gdb_continue_to_end
diff --git a/gdb/testsuite/gdb.base/bp-cmds-sourced-script.gdb b/gdb/testsuite/gdb.base/bp-cmds-sourced-script.gdb
new file mode 100644
index 00000000000..228fa388db1
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bp-cmds-sourced-script.gdb
@@ -0,0 +1,20 @@
+# Copyright 2020 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/>.  */
+
+break main
+break main
+commands 1 2
+  print 100123
+end
-- 
2.17.1

Re: [PATCH] gdb/breakpoint: make a copy of the "commands" command's argument

[Tom Tromey]

>>>>> "Tankut" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:

Tankut> When GDB reads commands from the input, its internal buffer is re-used
Tankut> for each line.

I wonder if this could be changed instead.

At least when reading from the user, a copy is already made.  For
instance, readline returns an allocated string.

I recall thinking that this was hard for some reason, but I don't
remember why any more :(

Tankut> gdb/ChangeLog:
Tankut> 2020-09-11  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

Tankut> 	* breakpoint.c (commands_command_1): Make a copy of the 'arg'
Tankut> 	argument.

This is ok.  Thank you.

Tom

Re: [PATCH] gdb/breakpoint: make a copy of the "commands" command's argument

[Tom Tromey]

Tankut> gdb/ChangeLog:
Tankut> 2020-09-11  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

Tankut> * breakpoint.c (commands_command_1): Make a copy of the 'arg'
Tankut> argument.

Tom> This is ok.  Thank you.

I forgot to mention -- this seems like a good candidate to apply to the
gdb 10 branch as well.

Tom

RE: [PATCH] gdb/breakpoint: make a copy of the "commands" command's argument

[Aktemur, Tankut Baris]

> >>>>> "Tankut" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tankut> When GDB reads commands from the input, its internal buffer is re-used
> Tankut> for each line.
> 
> I wonder if this could be changed instead.

This sounds much reasonable, however I'm not able to comment on how hard or intrusive
it would be, particularly because it involves readline and you already recalled
it being hard.

> At least when reading from the user, a copy is already made.  For
> instance, readline returns an allocated string.
> 
> I recall thinking that this was hard for some reason, but I don't
> remember why any more :(
>
> Tankut> gdb/ChangeLog:
> Tankut> 2020-09-11  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> 
> Tankut> 	* breakpoint.c (commands_command_1): Make a copy of the 'arg'
> Tankut> 	argument.
> 
> This is ok.  Thank you.

Thanks, I pushed the patch.
 
[from the second email]
> 
> I forgot to mention -- this seems like a good candidate to apply to the
> gdb 10 branch as well.
> 
> Tom

What is the policy for a release branch?  Should I simply cherry-pick the patch from
master and apply to the gdb-10 branch?

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH] gdb/breakpoint: make a copy of the "commands" command's argument

[Aktemur, Tankut Baris via Gdb-patches]

Hi Tom,

On Wednesday, September 16, 2020 4:45 PM, Aktemur, Tankut Baris wrote:
> > >>>>> "Tankut" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org>
> writes:
> >
> > Tankut> When GDB reads commands from the input, its internal buffer is re-used
> > Tankut> for each line.
> >
> > I wonder if this could be changed instead.
> 
> This sounds much reasonable, however I'm not able to comment on how hard or intrusive
> it would be, particularly because it involves readline and you already recalled
> it being hard.
> 
> > At least when reading from the user, a copy is already made.  For
> > instance, readline returns an allocated string.
> >
> > I recall thinking that this was hard for some reason, but I don't
> > remember why any more :(
> >
> > Tankut> gdb/ChangeLog:
> > Tankut> 2020-09-11  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> >
> > Tankut> 	* breakpoint.c (commands_command_1): Make a copy of the 'arg'
> > Tankut> 	argument.
> >
> > This is ok.  Thank you.
> 
> Thanks, I pushed the patch.
> 
> [from the second email]
> >
> > I forgot to mention -- this seems like a good candidate to apply to the
> > gdb 10 branch as well.
> >
> > Tom
> 
> What is the policy for a release branch?  Should I simply cherry-pick the patch from
> master and apply to the gdb-10 branch?

Shall I apply the patch to the gdb-10-branch, too?

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH] gdb/breakpoint: make a copy of the "commands" command's argument

[Tom Tromey]

>> > I forgot to mention -- this seems like a good candidate to apply to the
>> > gdb 10 branch as well.

>> What is the policy for a release branch?  Should I simply cherry-pick the patch from
>> master and apply to the gdb-10 branch?

>> Shall I apply the patch to the gdb-10-branch, too?

Oops, I'm sorry I didn't respond.
You can cherry-pick the patch.  I suppose you should probably update the
ChangeLog date.

Tom

RE: [PATCH] gdb/breakpoint: make a copy of the "commands" command's argument

[Aktemur, Tankut Baris via Gdb-patches]

On Friday, September 25, 2020 2:50 PM, Tom Tromey wrote:
> >> > I forgot to mention -- this seems like a good candidate to apply to the
> >> > gdb 10 branch as well.
> 
> >> What is the policy for a release branch?  Should I simply cherry-pick the patch from
> >> master and apply to the gdb-10 branch?
> 
> >> Shall I apply the patch to the gdb-10-branch, too?
> 
> Oops, I'm sorry I didn't respond.
> You can cherry-pick the patch.  I suppose you should probably update the
> ChangeLog date.
> 
> Tom

Pushed with the updated date.  Thanks.

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928