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

[Pedro Alves <palves at redhat dot com>]

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