[gdb/testsuite] Rewrite catch-follow-exec.exp

  On Wed, Oct 10, 2018 at 02:44:24PM +0100, Gary Benson wrote:
> Simon Marchi wrote:
> > On 2018-10-10 05:27, Gary Benson wrote:
> > > Tom de Vries wrote:
> > > > On 10/9/18 3:51 PM, Gary Benson wrote:
> > > > > Tom de Vries wrote:
> > > > > >      append FLAGS " \"$binfile\""
> > > > > >      append FLAGS " -batch"
> > > > > > +    append FLAGS " -ex \"target native\""
> > > > > >      append FLAGS " -ex \"catch exec\""
> > > > > >      append FLAGS " -ex \"set follow-exec-mode new\""
> > > > >
> > > > > I'm a little confused with this part, doesn't this force the
> > > > > test to run on the host?
> > > >
> > > > The "target native" was an attempt to fix problems when running
> > > > with --target_board=native-gdbserver. Perhaps it's better to
> > > > bail out in that case, but I haven't yet figured out how to. Any
> > > > advice here?
> > >
> > > Tests that can't run remote usually bail with something like this
> > > at the start:
> > >
> > >   if ![isnative] then {
> > >       return
> > >   }
> > 
> > I have not looked at the test (I can do it latter today if
> > necessary), but this comment caught my attention.  isnative is
> > likely not what you want to use, make sure to read the "Local vs
> > Remote vs Native" section of the gdb/testsuite/README file.
> 
> Oh!  Ok, so [target_info gdb_protocol] != ""] maybe?

Attached patch uses this this.

OK for trunk?

Thanks,
- Tom

[gdb/testsuite] Rewrite catch-follow-exec.exp

There are two problems with the current catch-follow-exec.exp:
- INTERNAL_GDBFLAGS (containing the datadir setting) is not used
- remote host testing doesn't work

Fix the former by using gdb_spawn_with_cmdline_opts.  Fix the latter by
requiring gdb-native.

Build on x86_64-linux with and without ubsan, tested with and without
--target_board=native-gdbserver.

2018-10-05  Tom de Vries  <tdevries@suse.de>

	PR gdb/23730
	* gdb.base/catch-follow-exec.c: Add copyright notice.
	* gdb.base/catch-follow-exec.exp: Rewrite to use
	gdb_spawn_with_cmdline_opts.  Require gdb-native.

---
 gdb/testsuite/gdb.base/catch-follow-exec.c   | 17 ++++++++++++
 gdb/testsuite/gdb.base/catch-follow-exec.exp | 41 ++++++++++++++++++++++------
 2 files changed, 49 insertions(+), 9 deletions(-)
  

Tom de Vries wrote:
> On Wed, Oct 10, 2018 at 02:44:24PM +0100, Gary Benson wrote:
> > Oh!  Ok, so [target_info gdb_protocol] != ""] maybe?
> 
> Attached patch uses this this.
> 
> OK for trunk?

Please reorder the checks at the start like this, to minimize the
work done before bailing:

  1. check gdb_protocol native
  2. check /bin/ls exists on target
  3. build_executable

The patch is ok by me with these changes, but please wait for a
maintainer to give the final approval, I don't have those powers :)
And thanks for doing the work Tom!

Cheers,
Gary

> [gdb/testsuite] Rewrite catch-follow-exec.exp
> 
> There are two problems with the current catch-follow-exec.exp:
> - INTERNAL_GDBFLAGS (containing the datadir setting) is not used
> - remote host testing doesn't work
> 
> Fix the former by using gdb_spawn_with_cmdline_opts.  Fix the latter by
> requiring gdb-native.
> 
> Build on x86_64-linux with and without ubsan, tested with and without
> --target_board=native-gdbserver.
> 
> 2018-10-05  Tom de Vries  <tdevries@suse.de>
> 
> 	PR gdb/23730
> 	* gdb.base/catch-follow-exec.c: Add copyright notice.
> 	* gdb.base/catch-follow-exec.exp: Rewrite to use
> 	gdb_spawn_with_cmdline_opts.  Require gdb-native.
> 
> ---
>  gdb/testsuite/gdb.base/catch-follow-exec.c   | 17 ++++++++++++
>  gdb/testsuite/gdb.base/catch-follow-exec.exp | 41 ++++++++++++++++++++++------
>  2 files changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.c b/gdb/testsuite/gdb.base/catch-follow-exec.c
> index fa68a2a34e..1a59f58aa5 100644
> --- a/gdb/testsuite/gdb.base/catch-follow-exec.c
> +++ b/gdb/testsuite/gdb.base/catch-follow-exec.c
> @@ -1,3 +1,20 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2018 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 <stdio.h>
>  #include <string.h>
>  #include <unistd.h>
> diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.exp b/gdb/testsuite/gdb.base/catch-follow-exec.exp
> index 0e32ed4a6f..2ccd0115e7 100644
> --- a/gdb/testsuite/gdb.base/catch-follow-exec.exp
> +++ b/gdb/testsuite/gdb.base/catch-follow-exec.exp
> @@ -22,8 +22,8 @@ if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
>      return -1
>  }
>  
> -if { ![remote_file target exists /bin/bash] } {
> -    unsupported "no bash"
> +if { [target_info gdb_protocol] != "" } {
> +    unsupported "not native"
>      return
>  }
>  
> @@ -34,7 +34,7 @@ if { ![remote_file target exists /bin/ls] } {
>  
>  proc catch_follow_exec { } {
>      global binfile
> -    global GDB
> +    global gdb_spawn_id
>  
>      set test "catch-follow-exec"
>  
> @@ -45,13 +45,36 @@ proc catch_follow_exec { } {
>      append FLAGS " -ex \"run\""
>      append FLAGS " -ex \"info prog\""
>  
> -    catch {exec /bin/bash -c "$GDB $FLAGS"} catchlog
> -    send_log "$catchlog\n"
> +    gdb_exit
> +    if {[gdb_spawn_with_cmdline_opts "$FLAGS"] != 0} {
> +	fail "spawn"
> +	return
> +    }
> +
> +    gdb_test_multiple "" "run til exit" {
> +	"runtime error:" {
> +	    # Error in case of --enable-ubsan
> +	    fail "No runtime error"
> +	}
> +	eof {
> +	    set result [wait -i $gdb_spawn_id]
> +	    verbose $result
> +
> +	    gdb_assert { [lindex $result 2] == 0 }
> +
> +	    # We suspect this will be zero instead of one after fixing PR23368
> +	    # - "gdb goes to into background when hitting exec catchpoint with
> +	    # follow-exec-mode new"
> +            gdb_assert { [lindex $result 3] == 1 }
> +
> +	    # Error in case of --disable-ubsan, we get
> +	    # "CHILDKILLED SIGSEGV {segmentation violation}" as extra
> +	    # argument(s).
> +	    gdb_assert { [llength $result] == 4 }
> +	}
>  
> -    if { [regexp {No selected thread} $catchlog] } {
> -	pass $test
> -    } else {
> -	fail $test
> +	remote_close host
> +	clear_gdb_spawn_id
>      }
>  }
>  
>
  

On 2018-10-11 04:33, Gary Benson wrote:
> Tom de Vries wrote:
>> On Wed, Oct 10, 2018 at 02:44:24PM +0100, Gary Benson wrote:
>> > Oh!  Ok, so [target_info gdb_protocol] != ""] maybe?
>> 
>> Attached patch uses this this.
>> 
>> OK for trunk?
> 
> Please reorder the checks at the start like this, to minimize the
> work done before bailing:
> 
>   1. check gdb_protocol native
>   2. check /bin/ls exists on target
>   3. build_executable
> 
> The patch is ok by me with these changes, but please wait for a
> maintainer to give the final approval, I don't have those powers :)
> And thanks for doing the work Tom!
> 
> Cheers,
> Gary

Just wondering.  Would it make life easier if we fixed PR 23368, which 
is the reason we have to do the test in an unnatural way?  I don't know 
if the fix I proposed here is totally correct:

https://sourceware.org/bugzilla/show_bug.cgi?id=23368#c4

but I think it would be an improvement compared to what we have now.  I 
ran it through the buildbot and there are no regressions.  If you want I 
can submit a proper patch for that, and then we can rewrite this test in 
a more normal way.

Simon
  

On 10/14/18 12:18 AM, Simon Marchi wrote:
> On 2018-10-11 04:33, Gary Benson wrote:
>> Tom de Vries wrote:
>>> On Wed, Oct 10, 2018 at 02:44:24PM +0100, Gary Benson wrote:
>>> > Oh!  Ok, so [target_info gdb_protocol] != ""] maybe?
>>>
>>> Attached patch uses this this.
>>>
>>> OK for trunk?
>>
>> Please reorder the checks at the start like this, to minimize the
>> work done before bailing:
>>
>>   1. check gdb_protocol native
>>   2. check /bin/ls exists on target
>>   3. build_executable
>>
>> The patch is ok by me with these changes, but please wait for a
>> maintainer to give the final approval, I don't have those powers :)
>> And thanks for doing the work Tom!
>>
>> Cheers,
>> Gary
> 
> Just wondering.  Would it make life easier if we fixed PR 23368, which
> is the reason we have to do the test in an unnatural way?

Yes.

> I don't know
> if the fix I proposed here is totally correct:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=23368#c4
> 
> but I think it would be an improvement compared to what we have now.  I
> ran it through the buildbot and there are no regressions.  If you want I
> can submit a proper patch for that, and then we can rewrite this test in
> a more normal way.
> 

That makes sense, but also I propose to commit the patch I've submitted
(with the early-out checks reorder as Gary suggested above), since
that's also an improvement on the current situation.

OK?

Thanks,
- Tom
  

On 2018-10-11 03:47, Tom de Vries wrote:
> [gdb/testsuite] Rewrite catch-follow-exec.exp
> 
> There are two problems with the current catch-follow-exec.exp:
> - INTERNAL_GDBFLAGS (containing the datadir setting) is not used
> - remote host testing doesn't work
> 
> Fix the former by using gdb_spawn_with_cmdline_opts.  Fix the latter by
> requiring gdb-native.
> 
> Build on x86_64-linux with and without ubsan, tested with and without
> --target_board=native-gdbserver.

My build of GDB (and probably some on the buildbot too?) uses 
-fsanitize=address, and on it the test does not pass.  On a build 
without -fsanitize=address, it does pass.  The failing test is:

FAIL: gdb.base/catch-follow-exec.exp: [lindex $result 3] == 1

and the value of $result is "17872 exp10 0 23".  This is because ASan 
exits with 23 if it detects leaks. If there's a way to set 
LSAN_OPTIONS='exitcode=1' in the environment GDB runs in, it would 
probably make it work...

> 2018-10-05  Tom de Vries  <tdevries@suse.de>
> 
> 	PR gdb/23730
> 	* gdb.base/catch-follow-exec.c: Add copyright notice.
> 	* gdb.base/catch-follow-exec.exp: Rewrite to use
> 	gdb_spawn_with_cmdline_opts.  Require gdb-native.
> 
> ---
>  gdb/testsuite/gdb.base/catch-follow-exec.c   | 17 ++++++++++++
>  gdb/testsuite/gdb.base/catch-follow-exec.exp | 41 
> ++++++++++++++++++++++------
>  2 files changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.c
> b/gdb/testsuite/gdb.base/catch-follow-exec.c
> index fa68a2a34e..1a59f58aa5 100644
> --- a/gdb/testsuite/gdb.base/catch-follow-exec.c
> +++ b/gdb/testsuite/gdb.base/catch-follow-exec.c
> @@ -1,3 +1,20 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2018 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 <stdio.h>
>  #include <string.h>
>  #include <unistd.h>
> diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.exp
> b/gdb/testsuite/gdb.base/catch-follow-exec.exp
> index 0e32ed4a6f..2ccd0115e7 100644
> --- a/gdb/testsuite/gdb.base/catch-follow-exec.exp
> +++ b/gdb/testsuite/gdb.base/catch-follow-exec.exp
> @@ -22,8 +22,8 @@ if {[build_executable "failed to prepare" $testfile
> $srcfile debug] == -1} {
>      return -1
>  }
> 
> -if { ![remote_file target exists /bin/bash] } {
> -    unsupported "no bash"
> +if { [target_info gdb_protocol] != "" } {
> +    unsupported "not native"

Please add a comment here, something like:

# Even though the feature under features being tested are supported by 
gdbserver,
# the way this test is written doesn't make it easy with a remote 
target.

>      return
>  }
> 
> @@ -34,7 +34,7 @@ if { ![remote_file target exists /bin/ls] } {
> 
>  proc catch_follow_exec { } {
>      global binfile
> -    global GDB
> +    global gdb_spawn_id
> 
>      set test "catch-follow-exec"
> 
> @@ -45,13 +45,36 @@ proc catch_follow_exec { } {
>      append FLAGS " -ex \"run\""
>      append FLAGS " -ex \"info prog\""
> 
> -    catch {exec /bin/bash -c "$GDB $FLAGS"} catchlog
> -    send_log "$catchlog\n"
> +    gdb_exit
> +    if {[gdb_spawn_with_cmdline_opts "$FLAGS"] != 0} {
> +	fail "spawn"
> +	return
> +    }
> +
> +    gdb_test_multiple "" "run til exit" {
> +	"runtime error:" {
> +	    # Error in case of --enable-ubsan
> +	    fail "No runtime error"
> +	}

Can you explain what this catches?  If this isn't there but UBSan is 
enabled, the crash isn't detected properly by the code below?

Please use a lower case letter for the test name.

> +	eof {
> +	    set result [wait -i $gdb_spawn_id]
> +	    verbose $result
> +
> +	    gdb_assert { [lindex $result 2] == 0 }
> +
> +	    # We suspect this will be zero instead of one after fixing 
> PR23368
> +	    # - "gdb goes to into background when hitting exec catchpoint 
> with
> +	    # follow-exec-mode new"
> +            gdb_assert { [lindex $result 3] == 1 }
> +
> +	    # Error in case of --disable-ubsan, we get
> +	    # "CHILDKILLED SIGSEGV {segmentation violation}" as extra
> +	    # argument(s).
> +	    gdb_assert { [llength $result] == 4 }
> +	}
> 
> -    if { [regexp {No selected thread} $catchlog] } {
> -	pass $test
> -    } else {
> -	fail $test
> +	remote_close host
> +	clear_gdb_spawn_id
>      }
>  }

Simon
  

On 2018-10-15 15:54, Tom de Vries wrote:
> That makes sense, but also I propose to commit the patch I've submitted
> (with the early-out checks reorder as Gary suggested above), since
> that's also an improvement on the current situation.
> 
> OK?
> 
> Thanks,
> - Tom

Sure, I am fine with making the situation better in the mean time.  I 
have replied to your patch directly.

Simon
  

On 10/16/18 12:11 AM, Simon Marchi wrote:
>> +    gdb_test_multiple "" "run til exit" {
>> +    "runtime error:" {
>> +        # Error in case of --enable-ubsan
>> +        fail "No runtime error"
>> +    }
> 
> Can you explain what this catches?  If this isn't there but UBSan is
> enabled, the crash isn't detected properly by the code below?

Indeed.

If I disable the fix this test-case is testing (so, I'm trying to make
the test-case fail), and I build with --disable-ubsan, I get:
...
27079 exp8 0 0 CHILDKILLED SIGSEGV {segmentation violation}
PASS: gdb.base/catch-follow-exec.exp: [lindex $result 2] == 0
FAIL: gdb.base/catch-follow-exec.exp: [lindex $result 3] == 1
FAIL: gdb.base/catch-follow-exec.exp: [llength $result] == 4
...
and with --enable-ubsan:
...
^[[1m/data/gdb_versions/devel/src/gdb/infcmd.c:2099:11:^[[1m^[[31m
runtime error: ^[[1m^[[0m^[[1mmember access wi\
thin null pointer of type 'struct thread_info'^[[1m^[[0m^M
FAIL: gdb.base/catch-follow-exec.exp: No runtime error
...

Without the extra "runtime error:" clause, I'm not detecting the fail:
...
^[[1m/data/gdb_versions/devel/src/gdb/infcmd.c:2099:11:^[[1m^[[31m
runtime error: ^[[1m^[[0m^[[1mmember access wi\
thin null pointer of type 'struct thread_info'^[[1m^[[0m^M
PASS: gdb.base/catch-follow-exec.exp: [lindex $result 2] == 0
PASS: gdb.base/catch-follow-exec.exp: [lindex $result 3] == 1
PASS: gdb.base/catch-follow-exec.exp: [llength $result] == 4
...

Thanks,
- Tom
  

[ was: Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp ]

On 10/16/18 12:11 AM, Simon Marchi wrote:
> Please use a lower case letter for the test name.

Hmm, that happens more often, appearantly:
...
find gdb/testsuite/ -type f -name "*.exp" | grep -v /lib/ | xargs egrep
$'[\t ](fail|pass|unsupported|untested|xfail|kfail) "[A-Z][a-z]' | egrep
-v 'PowerPC|Rust'
gdb/testsuite/gdb.ada/bp_inlined_func.exp:   fail "Cannot run to main,
testcase aborted"
gdb/testsuite/gdb.ada/excep_handle.exp:   fail "Cannot run to main,
testcase aborted"
gdb/testsuite/gdb.ada/mi_string_access.exp:   fail "Cannot run to main,
testcase aborted"
gdb/testsuite/gdb.ada/mi_var_union.exp:   fail "Cannot run to main,
testcase aborted"
gdb/testsuite/gdb.arch/arc-analyze-prologue.exp:    fail "Can't run to main"
gdb/testsuite/gdb.arch/arc-decode-insn.exp:    fail "Can't run to main"
gdb/testsuite/gdb.base/readnever.exp:    untested "Couldn't compile
${srcfile}"
gdb/testsuite/gdb.fortran/printing-types.exp:    untested "Could not run
to breakpoint MAIN__"
gdb/testsuite/gdb.guile/scm-lazy-string.exp:    fail "Can't run to main"
...

Does this need fixing?

Thanks,
- Tom
  

On 2018-10-17 03:30, Tom de Vries wrote:
> [ was: Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp ]
> 
> On 10/16/18 12:11 AM, Simon Marchi wrote:
>> Please use a lower case letter for the test name.
> 
> Hmm, that happens more often, appearantly:
> ...
> find gdb/testsuite/ -type f -name "*.exp" | grep -v /lib/ | xargs egrep
> $'[\t ](fail|pass|unsupported|untested|xfail|kfail) "[A-Z][a-z]' | 
> egrep
> -v 'PowerPC|Rust'
> gdb/testsuite/gdb.ada/bp_inlined_func.exp:   fail "Cannot run to main,
> testcase aborted"
> gdb/testsuite/gdb.ada/excep_handle.exp:   fail "Cannot run to main,
> testcase aborted"
> gdb/testsuite/gdb.ada/mi_string_access.exp:   fail "Cannot run to main,
> testcase aborted"
> gdb/testsuite/gdb.ada/mi_var_union.exp:   fail "Cannot run to main,
> testcase aborted"
> gdb/testsuite/gdb.arch/arc-analyze-prologue.exp:    fail "Can't run to 
> main"
> gdb/testsuite/gdb.arch/arc-decode-insn.exp:    fail "Can't run to main"
> gdb/testsuite/gdb.base/readnever.exp:    untested "Couldn't compile
> ${srcfile}"
> gdb/testsuite/gdb.fortran/printing-types.exp:    untested "Could not 
> run
> to breakpoint MAIN__"
> gdb/testsuite/gdb.guile/scm-lazy-string.exp:    fail "Can't run to 
> main"
> ...
> 
> Does this need fixing?

Sure, it would need fixing, if you want to take the time to do it.  
Here's the reference in our wiki where that rule is stated, for 
reference.

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Follow_the_test_name_convention

Thanks,

Simon
  

On 2018-10-15 3:54 p.m., Tom de Vries wrote:
>> Just wondering.  Would it make life easier if we fixed PR 23368, which
>> is the reason we have to do the test in an unnatural way?
> 
> Yes.

Hi Tom,

PR 23368 should be fixed now.  Do you plan on updating catch-follow-exec.exp
to be written in a more standard way?

Simon
  

On 10/23/18 11:04 PM, Simon Marchi wrote:
> On 2018-10-15 3:54 p.m., Tom de Vries wrote:
>>> Just wondering.  Would it make life easier if we fixed PR 23368, which
>>> is the reason we have to do the test in an unnatural way?
>>
>> Yes.
> 
> Hi Tom,
> 
> PR 23368 should be fixed now.  Do you plan on updating catch-follow-exec.exp
> to be written in a more standard way?

Sure, will do.

Thanks,
- Tom