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

[PATCH v2 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.

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

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

gdb/testsuite/ChangeLog:

2014-07-07  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   | 35 ++++++++++++++++
 gdb/testsuite/gdb.base/dprintf-detach.exp | 70 +++++++++++++++++++++++++++++++
 2 files changed, 105 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..b5c2fc4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-detach.c
@@ -0,0 +1,35 @@
+/* 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 <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..5dcdd28
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-detach.exp
@@ -0,0 +1,70 @@
+#   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
+
+# 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"
+
+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.
+gdb_start
+gdb_test "attach $inferior_pid" "Attaching to process $inferior_pid.*Reading symbols from $escapedbinfile.*" "re-attach to inferior"
-- 
2.0.0

[PATCH v2 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-07-07  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 908a1ea..fb833d0 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3112,7 +3112,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 v2 2/2] Only leave dprintf inserted if it is marked as persistent (PR breakpoints/17012)

[Joel Brobecker]

> 2014-07-07  Simon Marchi  <simon.marchi@ericsson.com>
> 
> 	PR breakpoints/17012
> 	* breakpoint.c (remove_breakpoints_pid): Only skip removing
> 	dprintf if it is marked as persistent.

Pedro is really the maintainer who is the most familiar with
breakpoint.c, I think, so it'd be better to wait for his feedback.
But, with my patch champion hat on, I would suggest merging both
patches into one, or else schedule the testsuite patch after the
actual fix. It's a bit of a detail in this case, but generally
speaking, we avoid having tests that fail until the fix is in.

> ---
>  gdb/breakpoint.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 908a1ea..fb833d0 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -3112,7 +3112,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

-- 
Joel

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

[Pedro Alves]

On 07/07/2014 08:17 PM, Simon Marchi wrote:

> +#include <stdlib.h>
> +
> +static void
> +function (void)
> +{
> +  sleep (1);

'sleep' needs unistd.h, not stdlib.h.

> +}
> +
> +int
> +main (void)
> +{
> +  int i;
> +
> +  for (i = 0; i < 30; i++)
> +  {
> +    function ();
> +  }

Could you please make this follow the same coding standard
as GDB?  That is, a single statement doesn't need braces.

> +}

> +# Only GNU/Linux is known to support (dprintf and detach).
> +if { ! [istarget "*-*-linux*"] } {
> +  return 0
> +}

dprintf call-style should has no target dependencies, so please
remove this making it run on all targets/archs.

> +
> +# Are we on a target board?
> +if [is_remote target] then {
> +    return 0
> +}

This should be a $use_gdb_stub check instead, and give out
an explanation for why we need to skip the test:

# The test relies on "detach/attach".
if [$use_gdb_stub] then {
    return 0
}

> +
> +standard_testfile
> +set escapedbinfile  [string_to_regexp ${binfile}]

Spurious double space.

> +
> +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"

It's best to make it so then.  That'll expand the coverage
of the test to more targets.  All targets can do always-inserted,
but only a few can do non-stop.  For extra coverage, I'd even
make the test exercise with both always-inserted on and off.
See bottom of break-unload-file.exp, for example.

> +
> +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:

if {$inferior_pid == -1} {
   return
}

after gdb_test_multiple, so that if the test fails, we don't
try to use a bogus pid.

> +# 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.
> +gdb_start
> +gdb_test "attach $inferior_pid" "Attaching to process $inferior_pid.*Reading symbols from $escapedbinfile.*" "re-attach to inferior"

I think that as is, this fails with --target_board=native-extended-gdbserver,
because with that, "attach" won't know which binary the program is running.
A gdb_load/clean_restart be missing too -- I think you might get
a complain about not knowing how to attach, as auto-connecting to
native target is force-disabled with that board (to catch these issues exactly).

Thanks,
-- 
Pedro Alves

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

[Pedro Alves]

On 07/07/2014 08:17 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.
> 
> gdb/Changelog:
> 
> 2014-07-07  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 908a1ea..fb833d0 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -3112,7 +3112,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;

I think that we don't really need the "bl->owner->type == bp_dprintf"
check anymore.  persist" carries all the semantics we need here.

Also, this is a boolean, so best write:

    if (bl->target_info.persist)
        continue;

without the "== 1" part.

But ... most importantly.  How do we end up with 'persist' set
on a call-style dprintf, given:

static void
build_target_command_list (struct bp_location *bl)
{
...
  /* For now, limit to agent-style dprintf breakpoints.  */
  if (dprintf_style != dprintf_style_agent)
    return;

I'm confused, as you say the test fails now, but nowhere in the
test are you setting the style to agent.  I think I'm missing
something.  :-)

-- 
Pedro Alves

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

[Pedro Alves]

On 07/15/2014 06:26 PM, Pedro Alves wrote:

> But ... most importantly.  How do we end up with 'persist' set
> on a call-style dprintf, given:
> 
> static void
> build_target_command_list (struct bp_location *bl)
> {
> ...
>   /* For now, limit to agent-style dprintf breakpoints.  */
>   if (dprintf_style != dprintf_style_agent)
>     return;
> 
> I'm confused, as you say the test fails now, but nowhere in the
> test are you setting the style to agent.  I think I'm missing
> something.  :-)
> 

Yeah, I was missing dinner...  :-)

Of course, the point is that indeed, for dprintf style != agent,
'persist' is not set, and those are the locations we want to remove.
Silly me.

BTW, I think it'd be good to make the test exercise all
dprintf styles, as all types are affected, and also
"set disconnected-dprintf off/on", to make "agent" style
affected.  With disconnected on, the agent style dprintf
will be persistent when testing against gdbserver
(extended-remote), but we should likewise be able to
detach and reattach back.

Something along the lines of:

foreach always_inserted { "off" "on" } {
    foreach style { "gdb" "call" "agent" } {
        foreach disconnected { "on" "off" } {
	   test $always_inserted $style $disconnected
	}
    }
}

-- 
Pedro Alves