[PATCH 1/2] Add dprintf and detach test (PR breakpoints/17012)

[PATCH 1/2] Add dprintf and detach test (PR breakpoints/17012)

[Simon Marchi]

This adds a test to demonstrate PR 17012, where adding a dprintf in a
linux native process and detaching leaves the trap instruction in the
process. I copied bits from many other existing tests.

The test fails now, but is fixed by the following commit.

gdb/testsuite/ChangeLog:

2014-06-18  Simon Marchi  simon.marchi@ericsson.com

	PR breakpoints/17012
	gdb.base/dprintf-detach.c: New file.
	gdb.base/dprintf-detach.exp: New file.

---
 gdb/testsuite/gdb.base/dprintf-detach.c   | 18 +++++++
 gdb/testsuite/gdb.base/dprintf-detach.exp | 80 +++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/dprintf-detach.c
 create mode 100644 gdb/testsuite/gdb.base/dprintf-detach.exp

diff --git a/gdb/testsuite/gdb.base/dprintf-detach.c b/gdb/testsuite/gdb.base/dprintf-detach.c
new file mode 100644
index 0000000..91f49ce
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-detach.c
@@ -0,0 +1,18 @@
+#include <stdlib.h>
+
+static void
+function (void)
+{
+  sleep (1);
+}
+
+int
+main (void)
+{
+  int i;
+
+  for (i = 0; i < 30; i++)
+  {
+    function ();
+  }
+}
diff --git a/gdb/testsuite/gdb.base/dprintf-detach.exp b/gdb/testsuite/gdb.base/dprintf-detach.exp
new file mode 100644
index 0000000..18ba154
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-detach.exp
@@ -0,0 +1,80 @@
+#   Copyright 2003-2014 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/>.  */
+
+# This test checks that inserting a dprintf and detaching does not crash
+# the program.
+#
+# Related bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17012
+
+# Only GNU/Linux is known to support (dprintf and detach).
+if { ! [istarget "*-*-linux*"] } {
+  return 0
+}
+
+# Are we on a target board?
+if [is_remote target] then {
+    return 0
+}
+
+standard_testfile
+set escapedbinfile  [string_to_regexp ${binfile}]
+
+if [prepare_for_testing "failed to prepare for dprintf-detach" \
+    ${testfile} ${srcfile} {debug}] {
+    return -1
+}
+
+# The problem was showing up in non-stop mode, since it enables
+# "breakpoint always-inserted", so this could also be
+# "set breakpoint always-inserted on".
+gdb_test_no_output "set non-stop on"
+
+if ![runto_main] {
+    fail "Can't run to main"
+    return -1
+}
+
+# Get PID of test program.
+set inferior_pid -1
+set test "get inferior process ID"
+gdb_test_multiple "call getpid ()" $test {
+    -re ".* = ($decimal).*$gdb_prompt $" {
+	set inferior_pid $expect_out(1,string)
+	pass $test
+    }
+}
+
+# Add a dprintf and detach.
+gdb_test "dprintf function, \"hello\"" "Dprintf .*" "dprintf insertion"
+gdb_test "detach" "Detaching from program: .*$escapedbinfile, .*" "detach program"
+
+# Exit gdb. Until we do that, the process will exist as a zombie.
+gdb_exit
+
+# Give some time for the ex-inferior to run and hopefully not crash.
+sleep 1
+
+# Check that the process still exists.
+set test "detached process should continue to exist"
+if {[catch {exec kill -0 $inferior_pid}]} {
+	# Process does not exist.
+	fail "$test"
+} else {
+	# Process exists.
+	pass "$test"
+}
+
+# Clean up.
+catch {exec kill -9 $inferior_pid}
-- 
2.0.0

[PATCH 2/2] Only leave dprintf inserted if it is marked as persistent (PR breakpoints/17012)

[Simon Marchi]

On Linux native, if dprintf are inserted when detaching, they are left
in the inferior which causes it to crash from a SIGTRAP. It also happens
with dprintfs on remote targets, when set disconnected-dprintf is off.

I believe that the rationale of the line I modified was to leave dprinfs
inserted in order to support disconnected dprintfs. This adds a check to
see if the dprintf should actually stay inserted or not.

bl->target_info.persist will be 1 only if disconnected-dprintf is on and
we are debugging a remote target. On native, it will always be 0,
regardless of the value of disconnected-dprintf. This makes sense, since
disconnected dprintfs are not supported by the native target.

gdb/Changelog:

2014-06-18  Simon Marchi  <simon.marchi@ericsson.com>

	PR breakpoints/17012
	* breakpoint.c (remove_breakpoints_pid): Only skip removing
	dprintf if it is marked as persistent.

---
 gdb/breakpoint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2240f08..1aa02f9 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3111,7 +3111,7 @@ remove_breakpoints_pid (int pid)
     if (bl->pspace != inf->pspace)
       continue;
 
-    if (bl->owner->type == bp_dprintf)
+    if (bl->owner->type == bp_dprintf && bl->target_info.persist == 1)
       continue;
 
     if (bl->inserted)
-- 
2.0.0

Re: [PATCH 1/2] Add dprintf and detach test (PR breakpoints/17012)

[Simon Marchi]

Ping.


-------- Original Message --------
Subject: [PATCH 1/2] Add dprintf and detach test (PR breakpoints/17012)
Date: Wed, 18 Jun 2014 10:26:10 -0400
From: Simon Marchi <simon.marchi@ericsson.com>
To: <gdb-patches@sourceware.org>
CC: Simon Marchi <simon.marchi@ericsson.com>

This adds a test to demonstrate PR 17012, where adding a dprintf in a
linux native process and detaching leaves the trap instruction in the
process. I copied bits from many other existing tests.

The test fails now, but is fixed by the following commit.

gdb/testsuite/ChangeLog:

2014-06-18  Simon Marchi  simon.marchi@ericsson.com

	PR breakpoints/17012
	gdb.base/dprintf-detach.c: New file.
	gdb.base/dprintf-detach.exp: New file.

---
 gdb/testsuite/gdb.base/dprintf-detach.c   | 18 +++++++
 gdb/testsuite/gdb.base/dprintf-detach.exp | 80 +++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/dprintf-detach.c
 create mode 100644 gdb/testsuite/gdb.base/dprintf-detach.exp

diff --git a/gdb/testsuite/gdb.base/dprintf-detach.c b/gdb/testsuite/gdb.base/dprintf-detach.c
new file mode 100644
index 0000000..91f49ce
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-detach.c
@@ -0,0 +1,18 @@
+#include <stdlib.h>
+
+static void
+function (void)
+{
+  sleep (1);
+}
+
+int
+main (void)
+{
+  int i;
+
+  for (i = 0; i < 30; i++)
+  {
+    function ();
+  }
+}
diff --git a/gdb/testsuite/gdb.base/dprintf-detach.exp b/gdb/testsuite/gdb.base/dprintf-detach.exp
new file mode 100644
index 0000000..18ba154
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-detach.exp
@@ -0,0 +1,80 @@
+#   Copyright 2003-2014 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/>.  */
+
+# This test checks that inserting a dprintf and detaching does not crash
+# the program.
+#
+# Related bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17012
+
+# Only GNU/Linux is known to support (dprintf and detach).
+if { ! [istarget "*-*-linux*"] } {
+  return 0
+}
+
+# Are we on a target board?
+if [is_remote target] then {
+    return 0
+}
+
+standard_testfile
+set escapedbinfile  [string_to_regexp ${binfile}]
+
+if [prepare_for_testing "failed to prepare for dprintf-detach" \
+    ${testfile} ${srcfile} {debug}] {
+    return -1
+}
+
+# The problem was showing up in non-stop mode, since it enables
+# "breakpoint always-inserted", so this could also be
+# "set breakpoint always-inserted on".
+gdb_test_no_output "set non-stop on"
+
+if ![runto_main] {
+    fail "Can't run to main"
+    return -1
+}
+
+# Get PID of test program.
+set inferior_pid -1
+set test "get inferior process ID"
+gdb_test_multiple "call getpid ()" $test {
+    -re ".* = ($decimal).*$gdb_prompt $" {
+	set inferior_pid $expect_out(1,string)
+	pass $test
+    }
+}
+
+# Add a dprintf and detach.
+gdb_test "dprintf function, \"hello\"" "Dprintf .*" "dprintf insertion"
+gdb_test "detach" "Detaching from program: .*$escapedbinfile, .*" "detach program"
+
+# Exit gdb. Until we do that, the process will exist as a zombie.
+gdb_exit
+
+# Give some time for the ex-inferior to run and hopefully not crash.
+sleep 1
+
+# Check that the process still exists.
+set test "detached process should continue to exist"
+if {[catch {exec kill -0 $inferior_pid}]} {
+	# Process does not exist.
+	fail "$test"
+} else {
+	# Process exists.
+	pass "$test"
+}
+
+# Clean up.
+catch {exec kill -9 $inferior_pid}
-- 
2.0.0

Re: [PATCH 1/2] Add dprintf and detach test (PR breakpoints/17012)

[Joel Brobecker]

Hello Simon,

> 2014-06-18  Simon Marchi  simon.marchi@ericsson.com
> 
> 	PR breakpoints/17012
> 	gdb.base/dprintf-detach.c: New file.
> 	gdb.base/dprintf-detach.exp: New file.

Sorry for the delay in answering this.

> diff --git a/gdb/testsuite/gdb.base/dprintf-detach.c b/gdb/testsuite/gdb.base/dprintf-detach.c
> new file mode 100644
> index 0000000..91f49ce
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/dprintf-detach.c
> @@ -0,0 +1,18 @@
> +#include <stdlib.h>

This file needs a copyright notice, please.

> diff --git a/gdb/testsuite/gdb.base/dprintf-detach.exp b/gdb/testsuite/gdb.base/dprintf-detach.exp
> new file mode 100644
> index 0000000..18ba154
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/dprintf-detach.exp
> @@ -0,0 +1,80 @@
> +#   Copyright 2003-2014 Free Software Foundation, Inc.

Can you confirm that 2003 is the  correct year for the start
of the range?

> +
> +# 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/>.  */
> +
> +# This test checks that inserting a dprintf and detaching does not crash
> +# the program.
> +#
> +# Related bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17012
> +
> +# Only GNU/Linux is known to support (dprintf and detach).
> +if { ! [istarget "*-*-linux*"] } {
> +  return 0
> +}
> +
> +# Are we on a target board?
> +if [is_remote target] then {
> +    return 0
> +}
> +
> +standard_testfile
> +set escapedbinfile  [string_to_regexp ${binfile}]
> +
> +if [prepare_for_testing "failed to prepare for dprintf-detach" \
> +    ${testfile} ${srcfile} {debug}] {
> +    return -1
> +}
> +
> +# The problem was showing up in non-stop mode, since it enables
> +# "breakpoint always-inserted", so this could also be
> +# "set breakpoint always-inserted on".
> +gdb_test_no_output "set non-stop on"
> +
> +if ![runto_main] {
> +    fail "Can't run to main"
> +    return -1
> +}
> +
> +# Get PID of test program.
> +set inferior_pid -1
> +set test "get inferior process ID"
> +gdb_test_multiple "call getpid ()" $test {
> +    -re ".* = ($decimal).*$gdb_prompt $" {
> +	set inferior_pid $expect_out(1,string)
> +	pass $test
> +    }
> +}
> +
> +# Add a dprintf and detach.
> +gdb_test "dprintf function, \"hello\"" "Dprintf .*" "dprintf insertion"
> +gdb_test "detach" "Detaching from program: .*$escapedbinfile, .*" "detach program"
> +
> +# Exit gdb. Until we do that, the process will exist as a zombie.

Two spaces after the period.

> +gdb_exit
> +
> +# Give some time for the ex-inferior to run and hopefully not crash.
> +sleep 1
> +
> +# Check that the process still exists.
> +set test "detached process should continue to exist"
> +if {[catch {exec kill -0 $inferior_pid}]} {
> +	# Process does not exist.
> +	fail "$test"
> +} else {
> +	# Process exists.
> +	pass "$test"
> +}
> +
> +# Clean up.
> +catch {exec kill -9 $inferior_pid}

I am not quite sure about how well how the above would work in practice.
How about restarting GDB, and do an "attach" gdb_test? That  way, we
can  kill the inferior process from GDB as well. (it's a good thing
you used a 30-times loops as well  to make sure the process eventually
terminates on its own as well).

-- 
Joel

Re: [PATCH 1/2] Add dprintf and detach test (PR breakpoints/17012)

[Simon Marchi]

On 14-07-07 11:06 AM, Joel Brobecker wrote:
> Hello Simon,
> 
>> 2014-06-18  Simon Marchi  simon.marchi@ericsson.com
>>
>> 	PR breakpoints/17012
>> 	gdb.base/dprintf-detach.c: New file.
>> 	gdb.base/dprintf-detach.exp: New file.
> 
> Sorry for the delay in answering this.
> 
>> diff --git a/gdb/testsuite/gdb.base/dprintf-detach.c b/gdb/testsuite/gdb.base/dprintf-detach.c
>> new file mode 100644
>> index 0000000..91f49ce
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/dprintf-detach.c
>> @@ -0,0 +1,18 @@
>> +#include <stdlib.h>
> 
> This file needs a copyright notice, please.

I checked a few other test files and they didn't have the copyright notice,
that's why I didn't add it. I have no problem adding it if it should be
there.

>> diff --git a/gdb/testsuite/gdb.base/dprintf-detach.exp b/gdb/testsuite/gdb.base/dprintf-detach.exp
>> new file mode 100644
>> index 0000000..18ba154
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/dprintf-detach.exp
>> @@ -0,0 +1,80 @@
>> +#   Copyright 2003-2014 Free Software Foundation, Inc.
> 
> Can you confirm that 2003 is the  correct year for the start
> of the range?

Oops, just 2014.

>> +
>> +# 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/>.  */
>> +
>> +# This test checks that inserting a dprintf and detaching does not crash
>> +# the program.
>> +#
>> +# Related bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17012
>> +
>> +# Only GNU/Linux is known to support (dprintf and detach).
>> +if { ! [istarget "*-*-linux*"] } {
>> +  return 0
>> +}
>> +
>> +# Are we on a target board?
>> +if [is_remote target] then {
>> +    return 0
>> +}
>> +
>> +standard_testfile
>> +set escapedbinfile  [string_to_regexp ${binfile}]
>> +
>> +if [prepare_for_testing "failed to prepare for dprintf-detach" \
>> +    ${testfile} ${srcfile} {debug}] {
>> +    return -1
>> +}
>> +
>> +# The problem was showing up in non-stop mode, since it enables
>> +# "breakpoint always-inserted", so this could also be
>> +# "set breakpoint always-inserted on".
>> +gdb_test_no_output "set non-stop on"
>> +
>> +if ![runto_main] {
>> +    fail "Can't run to main"
>> +    return -1
>> +}
>> +
>> +# Get PID of test program.
>> +set inferior_pid -1
>> +set test "get inferior process ID"
>> +gdb_test_multiple "call getpid ()" $test {
>> +    -re ".* = ($decimal).*$gdb_prompt $" {
>> +	set inferior_pid $expect_out(1,string)
>> +	pass $test
>> +    }
>> +}
>> +
>> +# Add a dprintf and detach.
>> +gdb_test "dprintf function, \"hello\"" "Dprintf .*" "dprintf insertion"
>> +gdb_test "detach" "Detaching from program: .*$escapedbinfile, .*" "detach program"
>> +
>> +# Exit gdb. Until we do that, the process will exist as a zombie.
> 
> Two spaces after the period.
> 
>> +gdb_exit
>> +
>> +# Give some time for the ex-inferior to run and hopefully not crash.
>> +sleep 1
>> +
>> +# Check that the process still exists.
>> +set test "detached process should continue to exist"
>> +if {[catch {exec kill -0 $inferior_pid}]} {
>> +	# Process does not exist.
>> +	fail "$test"
>> +} else {
>> +	# Process exists.
>> +	pass "$test"
>> +}
>> +
>> +# Clean up.
>> +catch {exec kill -9 $inferior_pid}
> 
> I am not quite sure about how well how the above would work in practice.
> How about restarting GDB, and do an "attach" gdb_test? That  way, we
> can  kill the inferior process from GDB as well. (it's a good thing
> you used a 30-times loops as well  to make sure the process eventually
> terminates on its own as well).

It "works", as in "it runs fine on my machine". Your suggestion to attach the
process with a new gdb seems more portable, as it doesn't rely on kill. I will
try to make a version based on that.

Re: [PATCH 1/2] Add dprintf and detach test (PR breakpoints/17012)

[Joel Brobecker]

> I checked a few other test files and they didn't have the copyright
> notice, that's why I didn't add it. I have no problem adding it if it
> should be there.

Yeah, we've been pretty bad about this in the past, and normally
we should go back and fix all files. But it can be so much work
researching those that we tend to do it piecemeal, when we happen
to notice one of these files. In the meantime, we try to be better
with all new files being added.

-- 
Joel