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

[PATCH v3 1/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.

New in v3:
* Follow-up Pedro's review
  * Remove == 1 for check on boolean.

gdb/Changelog:

	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 683ed2b..01e9b36 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3110,7 +3110,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)
       continue;
 
     if (bl->inserted)
-- 
2.1.0

[PATCH v3 2/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.

One issue is that the test does not pass when running with
--target_board=native-extended-gdbserver, partly due to bug 17302 [1].

One quick hack I tried for this was to add a useless "next" between the
call to getpid() and detach, which avoids the bug. There is still one
case where the test fails, and that is with:

- breakpoint always-inserted on
- dprintf-style agent
- disconnected-dprintf on

What happens is that my detach does not actually detach the process,
because some persistent commands (the disconnected dprintf) is present.
However since gdbserver is ran with --once, when gdb disconnects,
gdbserver goes down and takes with it all the processes it spawned and
that are still under its control (which includes my test process).
When the test checks if the test process is still alive, it obvisouly
fails. Investigating about that led me to ask a question on the ML [2]
about the behavior of detach.

Until the remote case is sorted out, would it be ok to disable the
test for all remote targets or KFAIL the tests? I will tackle the
blocking problems in separate patches.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=17302
[2] https://sourceware.org/ml/gdb/2014-08/msg00115.html

New in v3:
* Follow-up Pedro's review
  * Fix wrong include and formatting in test program.
  * Remove check to run the test only on Linux.
  * Disable test for stubs instead of all remote targets
  * Test with all cominations of dprintf-style, breakpoint
    always-inserted and disconnected-dprintf.
  * Check if "call getpid ()" succeeds.
  * Clean restart gdb when re-attaching,

New in v2:
* Verify process existence by trying to attach it with a new gdb.
* Added/updated copyright notices.

gdb/testsuite/ChangeLog:

	PR breakpoints/17012
	gdb.base/dprintf-detach.c: New file.
	gdb.base/dprintf-detach.exp: New file.
---
 gdb/testsuite/gdb.base/dprintf-detach.c   | 33 ++++++++++++
 gdb/testsuite/gdb.base/dprintf-detach.exp | 87 +++++++++++++++++++++++++++++++
 2 files changed, 120 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..16d7023
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-detach.c
@@ -0,0 +1,33 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+#include <unistd.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..c0fb6fc
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-detach.exp
@@ -0,0 +1,87 @@
+#   Copyright 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
+
+# The test relies on "detach/attach".
+if { [target_info exists use_gdb_stub] } 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
+}
+
+proc dprintf_detach_test { breakpoint_always_inserted dprintf_style disconnected_dprintf } {
+    set test_prefix "bai=${breakpoint_always_inserted} ds=${dprintf_style} dd=${disconnected_dprintf}"
+    global binfile decimal gdb_prompt escapedbinfile
+
+    with_test_prefix "$test_prefix" {
+	# Start with a clean gdb
+	clean_restart ${binfile}
+
+	gdb_test_no_output "set breakpoint always-inserted ${breakpoint_always_inserted}"
+	gdb_test_no_output "set dprintf-style ${dprintf_style}"
+	gdb_test_no_output "set disconnected-dprintf ${disconnected_dprintf}"
+
+	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
+	    }
+	}
+
+	if {$inferior_pid == -1} {
+	    return
+	}
+
+	# Add a dprintf and detach.
+	gdb_test "dprintf function, \"hello\"" "Dprintf .*" "dprintf insertion"
+	gdb_test "detach" "Detaching from program: .*$escapedbinfile, .*" "detach program"
+
+	gdb_exit
+
+	# Give some time for the ex-inferior to run and hopefully not crash.
+	sleep 1
+
+	# Check that the process still exists by attaching a new gdb to it.
+	clean_restart ${binfile}
+
+	gdb_test "attach $inferior_pid" "Attaching to program: $escapedbinfile, process $inferior_pid.*Reading symbols from.*" "re-attach to inferior"
+    }
+}
+
+foreach breakpoint_always_inserted { "on" "off" } {
+    foreach dprintf_style { "gdb" "call" "agent" } {
+	foreach disconnected_dprintf { "on" "off" } {
+	    dprintf_detach_test $breakpoint_always_inserted $dprintf_style $disconnected_dprintf
+	}
+    }
+}
-- 
2.1.0

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

[Pedro Alves]

On 08/22/2014 08:05 PM, Simon Marchi wrote:


> Until the remote case is sorted out, would it be ok to disable the
> test for all remote targets or KFAIL the tests? I will tackle the
> blocking problems in separate patches.

Sure.

This is OK otherwise.

Though please push the test and fix as a single commit.

Thanks,
Pedro Alves

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

[Simon Marchi]

On 14-09-04 12:45 PM, Pedro Alves wrote:
> On 08/22/2014 08:05 PM, Simon Marchi wrote:
> 
> 
>> Until the remote case is sorted out, would it be ok to disable the
>> test for all remote targets or KFAIL the tests? I will tackle the
>> blocking problems in separate patches.
> 
> Sure.

I am not sure how to do this though. Is this ok?

	# Disable this test when using gdb for now (the inferior crashes when
	# detaching due to PR 17302).
	set test "re-attach to inferior"
	if ![gdb_is_target_remote] {
		# Give some time for the ex-inferior to run and hopefully not crash.
		sleep 1

		gdb_test "attach $inferior_pid" "Attaching to program: $escapedbinfile, process $inferior_pid.*Reading symbols from.*" "$test"
	} else {
		kfail "breakpoints/17302" "$test"
	}

I know that "remote" does not necessarily means gdbserver, but I don't
know how to check for gdbserver specifically. I put the sleep in the condition
so we don't waste time for nothing if we are not going to run the test anyway.

> This is OK otherwise.
> 
> Though please push the test and fix as a single commit.
> 
> Thanks,
> Pedro Alves
> 

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

[Pedro Alves]

On 09/04/2014 07:37 PM, Simon Marchi wrote:
> On 14-09-04 12:45 PM, Pedro Alves wrote:
>> On 08/22/2014 08:05 PM, Simon Marchi wrote:
>>
>>
>>> Until the remote case is sorted out, would it be ok to disable the
>>> test for all remote targets or KFAIL the tests? I will tackle the
>>> blocking problems in separate patches.
>>
>> Sure.
> 
> I am not sure how to do this though. Is this ok?

Looks like it.  Alternatively, you can use setup_kfail
and always do the re-attach.

> 
> 	# Disable this test when using gdb for now (the inferior crashes when
> 	# detaching due to PR 17302).
> 	set test "re-attach to inferior"
> 	if ![gdb_is_target_remote] {
> 		# Give some time for the ex-inferior to run and hopefully not crash.
> 		sleep 1
> 
> 		gdb_test "attach $inferior_pid" "Attaching to program: $escapedbinfile, process $inferior_pid.*Reading symbols from.*" "$test"
> 	} else {
> 		kfail "breakpoints/17302" "$test"

"server/17302"

> 	}
> 
> I know that "remote" does not necessarily means gdbserver, but I don't
> know how to check for gdbserver specifically.

There's a check in qtro.exp you could reuse.  See "probe for GDBserver".

Thanks,
Pedro Alves

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

[Simon Marchi]

On 2014-09-05 11:41 AM, Pedro Alves wrote:
> On 09/04/2014 07:37 PM, Simon Marchi wrote:
>> On 14-09-04 12:45 PM, Pedro Alves wrote:
>>> On 08/22/2014 08:05 PM, Simon Marchi wrote:
>>>
>>>
>>>> Until the remote case is sorted out, would it be ok to disable the
>>>> test for all remote targets or KFAIL the tests? I will tackle the
>>>> blocking problems in separate patches.
>>>
>>> Sure.
>>
>> I am not sure how to do this though. Is this ok?
> 
> Looks like it.  Alternatively, you can use setup_kfail
> and always do the re-attach.
> 
>>
>> 	# Disable this test when using gdb for now (the inferior crashes when
>> 	# detaching due to PR 17302).
>> 	set test "re-attach to inferior"
>> 	if ![gdb_is_target_remote] {
>> 		# Give some time for the ex-inferior to run and hopefully not crash.
>> 		sleep 1
>>
>> 		gdb_test "attach $inferior_pid" "Attaching to program: $escapedbinfile, process $inferior_pid.*Reading symbols from.*" "$test"
>> 	} else {
>> 		kfail "breakpoints/17302" "$test"
> 
> "server/17302"
> 
>> 	}
>>
>> I know that "remote" does not necessarily means gdbserver, but I don't
>> know how to check for gdbserver specifically.
> 
> There's a check in qtro.exp you could reuse.  See "probe for GDBserver".
> 
> Thanks,
> Pedro Alves

All right, I finally pushed this, adding the target_is_gdbserver test to kfail
the test when using gdbserver.

Thanks.

From fc1269757f5614cd893c36120f61a5014a45fe37 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Wed, 10 Dec 2014 16:10:05 -0500
Subject: [PATCH] Only leave dprintf inserted if it is marked as persistent (PR
 breakpoints/17012)

On Linux native, if dprintfs 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.

The rationale of the line modified by the patch was to leave dprintfs
inserted in order to support disconnected dprintfs. However, not all
dprintfs are persistent. Also, there's no reason other kinds of
breakpoints can't be persistent either. So this replaces the bp_dprintf
check with a check on whether the location is persistent.

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.

One issue about the test is that it does not pass when using
--target_board=native-extended-gdbserver, partly due to bug 17302 [1].

One quick hack I tried for this was to add a useless "next" between the
call to getpid() and detach, which avoids the bug. There is still one
case where the test fails, and that is with:

- breakpoint always-inserted on
- dprintf-style agent
- disconnected-dprintf on

What happens is that my detach does not actually detach the process,
because some persistent commands (the disconnected dprintf) is present.
However since gdbserver is ran with --once, when gdb disconnects,
gdbserver goes down and takes with it all the processes it spawned and
that are still under its control (which includes my test process).
When the test checks if the test process is still alive, it obvisouly
fails. Investigating about that led me to ask a question on the ML [2]
about the behavior of detach.

Until the remote case is sorted out, the problematic test is marked as
KFAIL.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=17302
[2] https://sourceware.org/ml/gdb/2014-08/msg00115.html

gdb/Changelog:

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

gdb/testsuite/ChangeLog:

	PR breakpoints/17012
	* gdb.base/dprintf-detach.c: New file.
	* gdb.base/dprintf-detach.exp: New file.
---
 gdb/ChangeLog                             |  6 ++
 gdb/breakpoint.c                          |  5 +-
 gdb/testsuite/ChangeLog                   |  6 ++
 gdb/testsuite/gdb.base/dprintf-detach.c   | 33 +++++++++++
 gdb/testsuite/gdb.base/dprintf-detach.exp | 95 +++++++++++++++++++++++++++++++
 5 files changed, 141 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/dprintf-detach.c
 create mode 100644 gdb/testsuite/gdb.base/dprintf-detach.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b6b8859..441a20c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2014-12-10  Simon Marchi  <simon.marchi@ericsson.com>
+
+	PR breakpoints/17012
+	* breakpoint.c (remove_breakpoints_pid): Skip removing breakpoint if
+	it is marked as persistent.
+
 2014-12-10  Doug Evans  <xdje42@gmail.com>

 	* cp-namespace.c (cp_lookup_symbol_nonlocal): Move definition,
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 574d06c..8ccaf43 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3220,10 +3220,7 @@ remove_breakpoints_pid (int pid)
     if (bl->pspace != inf->pspace)
       continue;

-    if (bl->owner->type == bp_dprintf)
-      continue;
-
-    if (bl->inserted)
+    if (bl->inserted && !bl->target_info.persist)
       {
 	val = remove_breakpoint (bl, mark_uninserted);
 	if (val != 0)
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 4abd097..5ce3029 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,11 @@
 2014-12-10  Simon Marchi  <simon.marchi@ericsson.com>

+	PR breakpoints/17012
+	* gdb.base/dprintf-detach.c: New file.
+	* gdb.base/dprintf-detach.exp: New file.
+
+2014-12-10  Simon Marchi  <simon.marchi@ericsson.com>
+
 	* gdb.trace/qtro.exp: Replace gdbserver detection code by...
 	* lib/gdb.exp (target_is_gdbserver): New procedure.

diff --git a/gdb/testsuite/gdb.base/dprintf-detach.c b/gdb/testsuite/gdb.base/dprintf-detach.c
new file mode 100644
index 0000000..16d7023
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-detach.c
@@ -0,0 +1,33 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+#include <unistd.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..2cee42c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-detach.exp
@@ -0,0 +1,95 @@
+#   Copyright 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
+
+load_lib gdbserver-support.exp
+
+# The test relies on "detach/attach".
+if { [target_info exists use_gdb_stub] } 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
+}
+
+proc dprintf_detach_test { breakpoint_always_inserted dprintf_style disconnected_dprintf } {
+    set test_prefix "bai=${breakpoint_always_inserted} ds=${dprintf_style} dd=${disconnected_dprintf}"
+    global binfile decimal gdb_prompt escapedbinfile
+
+    with_test_prefix "$test_prefix" {
+	# Start with a clean gdb
+	clean_restart ${binfile}
+
+	gdb_test_no_output "set breakpoint always-inserted ${breakpoint_always_inserted}"
+	gdb_test_no_output "set dprintf-style ${dprintf_style}"
+	gdb_test_no_output "set disconnected-dprintf ${disconnected_dprintf}"
+
+	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
+	    }
+	}
+
+	if {$inferior_pid == -1} {
+	    return
+	}
+
+	# Add a dprintf and detach.
+	gdb_test "dprintf function, \"hello\"" "Dprintf .*" "dprintf insertion"
+	gdb_test "detach" "Detaching from program: .*$escapedbinfile, .*" "detach program"
+
+	gdb_exit
+
+	# Check that the process still exists by attaching a new gdb to it.
+	clean_restart ${binfile}
+	set test "re-attach to inferior"
+	set is_gdbserver [target_is_gdbserver]
+
+	if { $is_gdbserver == 1 } {
+		setup_kfail "*-*-*" "server/17302"
+	} else {
+		# Give some time for the ex-inferior to run and hopefully not crash.
+		sleep 1
+	}
+
+	gdb_test "attach $inferior_pid" "Attaching to program: $escapedbinfile, process $inferior_pid.*Reading symbols from.*" "$test"
+    }
+}
+
+foreach breakpoint_always_inserted { "on" "off" } {
+    foreach dprintf_style { "gdb" "call" "agent" } {
+	foreach disconnected_dprintf { "on" "off" } {
+	    dprintf_detach_test $breakpoint_always_inserted $dprintf_style $disconnected_dprintf
+	}
+    }
+}
-- 
2.1.3

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

[Simon Marchi]

On 14-08-22 03:05 PM, Simon Marchi wrote:
> 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.
> 
> New in v3:
> * Follow-up Pedro's review
>   * Remove == 1 for check on boolean.
> 
> gdb/Changelog:
> 
> 	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 683ed2b..01e9b36 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -3110,7 +3110,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)
>        continue;
>  
>      if (bl->inserted)

Ping.

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

[Pedro Alves]

On 08/22/2014 08:05 PM, Simon Marchi wrote:
> On Linux native, if dprintf are inserted when detaching, they are left

"dprintfs"

> 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.

s/dprinfs/dprintfs/

A nit: personally I prefer if logs sounds a little more confident
once questions are resolved.  I'd suggest:

 s/I believe that the/The/
 s/line I modified/line modified by the patch/

resulting in:

 The rationale of the line modified by the patch was to leave dprintfs
 inserted in order to support disconnected dprintfs.  However, not all
 dprintfs are persistent.  Also, there's no reason other kinds of
 breakpoints can't be persistent either.  So this replaces the bp_dprintf
 check with a check on whether the location is persistent.

> 
> 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.
> 

> New in v3:
> * Follow-up Pedro's review
>   * Remove == 1 for check on boolean.

There was also a point about removing the "type == bp_dprintf"
check completely.  Did you find we actually need it for some reason?

I think it's better to treat bl->target_info's contents as
undefined if the breakpoint is not inserted.  So I think the
clearest and best would be to merge this check with the one below,
resulting in

-    if (bl->owner->type == bp_dprintf)
-      continue;
-
-    if (bl->inserted)
     if (bl->inserted && !bl->target_info.persist)

I realize this may sound like a nit, but just this past week I was
playing with replacing the bl->target_info field with a pointer to
a refcounted target_info object, and the pointer would only be
set when the breakpoint is inserted   :-)

OK with that change.

Thanks,
Pedro Alves

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

[Simon Marchi]

On 14-09-04 12:45 PM, Pedro Alves wrote:
> On 08/22/2014 08:05 PM, Simon Marchi wrote:
>> On Linux native, if dprintf are inserted when detaching, they are left
> 
> "dprintfs"
> 
>> 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.
> 
> s/dprinfs/dprintfs/
> 
> A nit: personally I prefer if logs sounds a little more confident
> once questions are resolved.  I'd suggest:
> 
>  s/I believe that the/The/
>  s/line I modified/line modified by the patch/
> 
> resulting in:
> 
>  The rationale of the line modified by the patch was to leave dprintfs
>  inserted in order to support disconnected dprintfs.  However, not all
>  dprintfs are persistent.  Also, there's no reason other kinds of
>  breakpoints can't be persistent either.  So this replaces the bp_dprintf
>  check with a check on whether the location is persistent.
> 
>>
>> 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.
>>
> 
>> New in v3:
>> * Follow-up Pedro's review
>>   * Remove == 1 for check on boolean.
> 
> There was also a point about removing the "type == bp_dprintf"
> check completely.  Did you find we actually need it for some reason?

Right now, persist can only be set for dprintfs (in build_target_command_list),
so it shouldn't change anything. But like you said, there is no reason why the
persist field should apply to dprintf only, so I agree we can remove the check.

> I think it's better to treat bl->target_info's contents as
> undefined if the breakpoint is not inserted.  So I think the
> clearest and best would be to merge this check with the one below,
> resulting in
> 
> -    if (bl->owner->type == bp_dprintf)
> -      continue;
> -
> -    if (bl->inserted)
>      if (bl->inserted && !bl->target_info.persist)
> 
> I realize this may sound like a nit, but just this past week I was
> playing with replacing the bl->target_info field with a pointer to
> a refcounted target_info object, and the pointer would only be
> set when the breakpoint is inserted   :-)

It makes it cleaner anyway, good suggestion.

> OK with that change.
> 
> Thanks,
> Pedro Alves