[PATCH 0/2] GDB test suite: Support targets with systemd-coredump

[PATCH 0/2] GDB test suite: Support targets with systemd-coredump

[Andreas Arnez]

Various GNU/Linux distributions nowadays ship with systemd-coredump
enabled by default.  On such systems all GDB test cases dealing with core
files are doomed to fail.

These patches aim at improving the situation by invoking the coredumpctl
command if core dumps can not be located otherwise.

Andreas Arnez (2):
  GDB test suite: Add helper for locating core files
  GDB test suite: Get core files on targets with systemd-coredump

 gdb/testsuite/gdb.arch/s390-multiarch.exp |   2 +-
 gdb/testsuite/gdb.arch/s390-vregs.exp     |   6 +-
 gdb/testsuite/gdb.base/bigcore.exp        |  11 +--
 gdb/testsuite/gdb.base/break-interp.exp   |   2 +-
 gdb/testsuite/gdb.base/corefile.exp       |   2 +-
 gdb/testsuite/gdb.threads/corethreads.exp |   2 +-
 gdb/testsuite/lib/gdb.exp                 | 138 ++++++++++++++++++------------
 7 files changed, 92 insertions(+), 71 deletions(-)

-- 
2.9.4

[PATCH 1/2] GDB test suite: Add helper for locating core files

[Andreas Arnez]

The test suite contains a convenience proc `core_find' that basically
performs two tasks:

1. Run a given command (expecting it to dump core).
2. Locate the core dump generated by the command, and return it.

However, some test cases just need the second part, because they run the
command in a different way.  Hence they implement their own logic for
locating the core dump.

This change replaces the instances of core dump retrieval logic by
invocations of a new separate proc.  And it renames `core_find' to
`run_and_get_core', to reduce confusion.

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (exec_in_dir): New helper proc.
	(core_find): Rename to...
	(run_and_get_core): ...this.  Remove the deletefiles argument.
	Split out the logic for locating the dump into...
	(find_core): ...this new proc.
	* gdb.arch/s390-multiarch.exp: Adjust invocations of core_find.
	* gdb.base/break-interp.exp: Likewise.
	* gdb.base/corefile.exp: Likewise.
	* gdb.threads/corethreads.exp: Likewise.
	* gdb.arch/s390-vregs.exp: Exploit find_core.
	* gdb.base/bigcore.exp: Likewise.
---
 gdb/testsuite/gdb.arch/s390-multiarch.exp |   2 +-
 gdb/testsuite/gdb.arch/s390-vregs.exp     |   6 +-
 gdb/testsuite/gdb.base/bigcore.exp        |  11 +--
 gdb/testsuite/gdb.base/break-interp.exp   |   2 +-
 gdb/testsuite/gdb.base/corefile.exp       |   2 +-
 gdb/testsuite/gdb.threads/corethreads.exp |   2 +-
 gdb/testsuite/lib/gdb.exp                 | 127 +++++++++++++++++-------------
 7 files changed, 80 insertions(+), 72 deletions(-)

diff --git a/gdb/testsuite/gdb.arch/s390-multiarch.exp b/gdb/testsuite/gdb.arch/s390-multiarch.exp
index 28545d7..9d45e04 100644
--- a/gdb/testsuite/gdb.arch/s390-multiarch.exp
+++ b/gdb/testsuite/gdb.arch/s390-multiarch.exp
@@ -51,7 +51,7 @@ proc compile_and_dump {variant ccopts binfile} {
     pass $test
 
     set test "create core file ($variant)"
-    set corefile [core_find $binfile]
+    set corefile [run_and_get_core $binfile]
     if {$corefile == ""} {
 	fail $test
 	return {}
diff --git a/gdb/testsuite/gdb.arch/s390-vregs.exp b/gdb/testsuite/gdb.arch/s390-vregs.exp
index d2c31e1..d5ed6f2 100644
--- a/gdb/testsuite/gdb.arch/s390-vregs.exp
+++ b/gdb/testsuite/gdb.arch/s390-vregs.exp
@@ -185,14 +185,12 @@ gdb_exit
 
 # Find the core file and rename it (avoid accumulating core files).
 
-set cores [glob -nocomplain -directory $coredir *core*]
-if {[llength $cores] != 1} {
+set destcore [find_core $binfile $coredir]
+if {$destcore == ""} {
     untested "core file not found"
     remote_exec build "rm -rf $coredir"
     return -1
 }
-set destcore [standard_output_file ${testfile}.core]
-remote_exec build "mv [file join $coredir [lindex $cores 0]] $destcore"
 remote_exec build "rm -rf $coredir"
 
 # Restart gdb and load the core file.  Compare the VRs.
diff --git a/gdb/testsuite/gdb.base/bigcore.exp b/gdb/testsuite/gdb.base/bigcore.exp
index dbd8c00..ba8480a 100644
--- a/gdb/testsuite/gdb.base/bigcore.exp
+++ b/gdb/testsuite/gdb.base/bigcore.exp
@@ -135,16 +135,7 @@ gdb_test "signal SIGABRT" "Program terminated with signal SIGABRT, .*"
 set timeout $oldtimeout
 
 # Find the corefile
-set file ""
-foreach pat [list core.${inferior_pid} ${testfile}.core core] {
-    set names [glob -nocomplain [standard_output_file $pat]]
-    if {[llength $names] == 1} {
-	set file [lindex $names 0]
-	remote_exec build "mv $file $corefile"
-	break
-    }
-}
-
+set file [find_core $binfile [file dirname $corefile] $corefile]
 if { $file == "" } {
     untested "can't generate a core file"
     return 0
diff --git a/gdb/testsuite/gdb.base/break-interp.exp b/gdb/testsuite/gdb.base/break-interp.exp
index 3a6d9a9..49967c6 100644
--- a/gdb/testsuite/gdb.base/break-interp.exp
+++ b/gdb/testsuite/gdb.base/break-interp.exp
@@ -201,7 +201,7 @@ proc test_core {file displacement} {
     with_test_prefix "core" {
 	global srcdir subdir gdb_prompt expect_out
 
-	set corefile [core_find $file {} "segv"]
+	set corefile [run_and_get_core $file "segv"]
 	if {$corefile == ""} {
 	    return
 	}
diff --git a/gdb/testsuite/gdb.base/corefile.exp b/gdb/testsuite/gdb.base/corefile.exp
index 21b974f..a9f7115 100644
--- a/gdb/testsuite/gdb.base/corefile.exp
+++ b/gdb/testsuite/gdb.base/corefile.exp
@@ -34,7 +34,7 @@ if {[build_executable $testfile.exp $testfile $srcfile debug] == -1} {
     return -1
 }
 
-set corefile [core_find $binfile {coremmap.data}]
+set corefile [run_and_get_core $binfile]
 if {$corefile == ""} {
     return 0
 }
diff --git a/gdb/testsuite/gdb.threads/corethreads.exp b/gdb/testsuite/gdb.threads/corethreads.exp
index f7d85fb..0f80625 100644
--- a/gdb/testsuite/gdb.threads/corethreads.exp
+++ b/gdb/testsuite/gdb.threads/corethreads.exp
@@ -26,7 +26,7 @@ if { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executa
      return -1
 }
 
-set corefile [core_find $binfile]
+set corefile [run_and_get_core $binfile]
 if {$corefile == ""} {
     return 0
 }
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 8af1b77..1fd0009 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5795,71 +5795,90 @@ if {[info exists GDB_PARALLEL]} {
     }
 }
 
-proc core_find {binfile {deletefiles {}} {arg ""}} {
-    global objdir subdir
+# Run COMMAND in directory DIR.  Suppress the command's output and ignore
+# its exit code.  Return 0 if the command could be executed, nonzero
+# otherwise.
+
+proc exec_in_dir {dir command} {
+    set err [catch {system "(cd ${dir}; ${command}; true) >/dev/null 2>&1"}]
+    if {$err} {
+	verbose -log "Failed executing \"${command}\"."
+    } else {
+	verbose -log "Executed: \"${command}\"."
+    }
+    return $err
+}
 
-    set destcore "$binfile.core"
-    file delete $destcore
+# Run a program that will dump core, then try to locate the core file.
+# Return the core file's filename, or "", if none was found.
 
-    # Create a core file named "$destcore" rather than just "core", to
-    # avoid problems with sys admin types that like to regularly prune all
-    # files named "core" from the system.
-    #
-    # Arbitrarily try setting the core size limit to "unlimited" since
-    # this does not hurt on systems where the command does not work and
-    # allows us to generate a core on systems where it does.
-    #
-    # Some systems append "core" to the name of the program; others append
-    # the name of the program to "core"; still others (like Linux, as of
-    # May 2003) create cores named "core.PID".  In the latter case, we
-    # could have many core files lying around, and it may be difficult to
-    # tell which one is ours, so let's run the program in a subdirectory.
-    set found 0
+proc run_and_get_core {binfile {arg ""}} {
+    if {[is_remote target]} {
+	warning "Can not generate core dump on remote target."
+	return ""
+    }
+    # Run the program in a subdirectory, to prevent generated files or
+    # core files from clashing, and to simplify clean-up.
     set coredir [standard_output_file coredir.[getpid]]
     file mkdir $coredir
-    catch "system \"(cd ${coredir}; ulimit -c unlimited; ${binfile} ${arg}; true) >/dev/null 2>&1\""
-    #      remote_exec host "${binfile}"
+
+    set err [exec_in_dir $coredir "ulimit -c unlimited; ${binfile} ${arg}"]
+    if {$err} {
+	# Try again without "ulimit -c", for shells that don't understand
+	# this.
+	set err [exec_in_dir $coredir "${binfile} ${arg}"]
+    }
+    if {$err} {
+	set destcore ""
+	warning "Could not invoke ${binfile} for generating a core dump"
+    } else {
+	set destcore [find_core $binfile $coredir]
+    }
+
+    # Clean up after ourselves.
+    file delete -force $coredir
+
+    return $destcore
+}
+
+# Find a core file that should have been created by the executable BINFILE
+# in the directory COREDIR.  Move it to "BINFILE.core", or to DESTCORE, if
+# specified.  Return that path name, or "" if no core file was found.
+
+proc find_core {binfile coredir {destcore ""}} {
+    if {[is_remote target]} {
+	warning "Can not access remote core file."
+	return ""
+    }
+    global objdir subdir
+
+    if {$destcore eq ""} {
+	set destcore "$binfile.core"
+    }
+    file delete $destcore
+
     foreach i "${coredir}/core ${coredir}/core.coremaker.c ${binfile}.core" {
-	if [remote_file build exists $i] {
-	    remote_exec build "mv $i $destcore"
-	    set found 1
+	if [file exists $i] {
+	    file rename $i $destcore
+	    return $destcore
 	}
     }
     # Check for "core.PID".
-    if { $found == 0 } {
-	set names [glob -nocomplain -directory $coredir core.*]
-	if {[llength $names] == 1} {
-	    set corefile [file join $coredir [lindex $names 0]]
-	    remote_exec build "mv $corefile $destcore"
-	    set found 1
-	}
-    }
-    if { $found == 0 } {
-	# The braindamaged HPUX shell quits after the ulimit -c above
-	# without executing ${binfile}.  So we try again without the
-	# ulimit here if we didn't find a core file above.
-	# Oh, I should mention that any "braindamaged" non-Unix system has
-	# the same problem. I like the cd bit too, it's really neat'n stuff.
-	catch "system \"(cd ${objdir}/${subdir}; ${binfile}; true) >/dev/null 2>&1\""
-	foreach i "${objdir}/${subdir}/core ${objdir}/${subdir}/core.coremaker.c ${binfile}.core" {
-	    if [remote_file build exists $i] {
-		remote_exec build "mv $i $destcore"
-		set found 1
-	    }
+    set names [glob -nocomplain -directory $coredir core.*]
+    if {[llength $names] == 1} {
+	set corefile [file join $coredir [lindex $names 0]]
+	file rename $corefile $destcore
+	return $destcore
+    }
+    foreach i "${objdir}/${subdir}/core ${objdir}/${subdir}/core.coremaker.c ${binfile}.core" {
+	if [file exists $i] {
+	    file rename $i $destcore
+	    return $destcore
 	}
     }
 
-    # Try to clean up after ourselves. 
-    foreach deletefile $deletefiles {
-	remote_file build delete [file join $coredir $deletefile]
-    }
-    remote_exec build "rmdir $coredir"
-	
-    if { $found == 0  } {
-	warning "can't generate a core file - core tests suppressed - check ulimit -c"
-	return ""
-    }
-    return $destcore
+    warning "Can not locate core dump generated by \"[file tail $binfile]\"."
+    return ""
 }
 
 # gdb_target_symbol_prefix compiles a test program and then examines
-- 
2.9.4

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Kevin Buettner]

On Mon, 18 Sep 2017 20:41:51 +0200
Andreas Arnez <arnez@linux.vnet.ibm.com> wrote:

> The test suite contains a convenience proc `core_find' that basically
> performs two tasks:
> 
> 1. Run a given command (expecting it to dump core).
> 2. Locate the core dump generated by the command, and return it.
> 
> However, some test cases just need the second part, because they run the
> command in a different way.  Hence they implement their own logic for
> locating the core dump.
> 
> This change replaces the instances of core dump retrieval logic by
> invocations of a new separate proc.  And it renames `core_find' to
> `run_and_get_core', to reduce confusion.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* lib/gdb.exp (exec_in_dir): New helper proc.
> 	(core_find): Rename to...
> 	(run_and_get_core): ...this.  Remove the deletefiles argument.
> 	Split out the logic for locating the dump into...
> 	(find_core): ...this new proc.
> 	* gdb.arch/s390-multiarch.exp: Adjust invocations of core_find.
> 	* gdb.base/break-interp.exp: Likewise.
> 	* gdb.base/corefile.exp: Likewise.
> 	* gdb.threads/corethreads.exp: Likewise.
> 	* gdb.arch/s390-vregs.exp: Exploit find_core.
> 	* gdb.base/bigcore.exp: Likewise.

Hi Andreas,

I've looked your patches over and for the most part, I like them.
They better consolidate the logic for finding core files to
lib/gdb.exp.

However, one serious problem is that your rewrite of code in
lib/gdb.exp removes support for the handling of remote core files.

E.g. when I test your patch on my x86-64 linux box using the
following command...

    make check RUNTESTFLAGS="--target_board=native-gdbserver"

...I see 32 fewer passes than before and also one more known failure.

Here are the passes that no longer occur when using your patch:

    PASS: gdb.base/corefile.exp: args: -core=corefile.core
    PASS: gdb.base/corefile.exp: args: execfile -core=corefile.core
    PASS: gdb.base/corefile.exp: core-file command
    PASS: gdb.base/corefile.exp: print coremaker_data
    PASS: gdb.base/corefile.exp: print coremaker_bss
    PASS: gdb.base/corefile.exp: print coremaker_ro
    PASS: gdb.base/corefile.exp: print func2::coremaker_local
    PASS: gdb.base/corefile.exp: $_exitsignal prints SIGABRT (6)
    PASS: gdb.base/corefile.exp: $_exitcode is void
    PASS: gdb.base/corefile.exp: backtrace in corefile.exp
    PASS: gdb.base/corefile.exp: up in corefile.exp
    PASS: gdb.base/corefile.exp: accessing original mmap data in core file
    PASS: gdb.base/corefile.exp: accessing mmapped data in core file
    PASS: gdb.base/corefile.exp: up in corefile.exp (reinit)
    PASS: gdb.base/corefile.exp: core
    PASS: gdb.base/corefile.exp: run: load core again
    PASS: gdb.base/corefile.exp: run: sanity check we see the core file
    PASS: gdb.base/corefile.exp: run: with core
    PASS: gdb.base/corefile.exp: run: core file is cleared
    PASS: gdb.base/corefile.exp: quit with a process
    PASS: gdb.base/corefile.exp: quit with processes: n
    PASS: gdb.base/corefile.exp: no question: load core
    PASS: gdb.base/corefile.exp: quit with a core file
    PASS: gdb.base/corefile.exp: core-file warning-free
    PASS: gdb.threads/corethreads.exp: load core
    PASS: gdb.threads/corethreads.exp: sanity check we see the core file
    PASS: gdb.threads/corethreads.exp: print pthread_t of thread0
    PASS: gdb.threads/corethreads.exp: print pthread_t of thread1
    PASS: gdb.threads/corethreads.exp: thread0 found
    PASS: gdb.threads/corethreads.exp: thread1 found
    PASS: gdb.threads/corethreads.exp: no other thread found

Instead, several warnings are now printed instead:

    WARNING: Can not generate core dump on remote target.

If you can restore support for handling of remote core files, I'd very
much like to review this patch again.

Thanks,

Kevin

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Andreas Arnez]

On Sat, Oct 07 2017, Kevin Buettner wrote:

[...]

> E.g. when I test your patch on my x86-64 linux box using the
> following command...
>
>     make check RUNTESTFLAGS="--target_board=native-gdbserver"
>
> ...I see 32 fewer passes than before and also one more known failure.
>
> Here are the passes that no longer occur when using your patch:

[...]

> Instead, several warnings are now printed instead:
>
>     WARNING: Can not generate core dump on remote target.

These warnings are newly introduced by the patch.  They are meant to
improve diagnostics when someone attempts to run the tests on a "real"
remote target.  I wanted to clearly document the fact that this is
unsupported (and always was).  Also, by documenting this restriction,
maybe someone feels encouraged to lift it ;-)

But it seems I went overboard, now also bailing out in case of
native-gdbserver, which is unnecessary, since no extra handling is
required for that.

How to fix this, though?  Rather than bailing out on "is_remote target",
maybe we should check for "isnative" instead?  See the delta-patch
below.  This should fix the problem with native-gdbserver and is
probably not worse than before in other scenarios, so maybe it's good
enough.  WDYT?

> If you can restore support for handling of remote core files, I'd very
> much like to review this patch again.

Thanks, I'd appreciate that.

--
Andreas

-- >8 --
Subject: [PATCH] Squash into "GDB test suite: Add helper for locating core files"

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 2c5e94d..091933a 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5850,7 +5850,7 @@ proc exec_in_dir {dir command} {
 # Return the core file's filename, or "", if none was found.
 
 proc run_and_get_core {binfile {arg ""}} {
-    if {[is_remote target]} {
+    if {![isnative]} {
 	warning "Can not generate core dump on remote target."
 	return ""
     }
@@ -5883,7 +5883,7 @@ proc run_and_get_core {binfile {arg ""}} {
 # specified.  Return that path name, or "" if no core file was found.
 
 proc find_core {binfile coredir {destcore ""}} {
-    if {[is_remote target]} {
+    if {![isnative]} {
 	warning "Can not access remote core file."
 	return ""
     }

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Kevin Buettner]

On Mon, 09 Oct 2017 20:46:21 +0200
Andreas Arnez <arnez@linux.vnet.ibm.com> wrote:

> > Instead, several warnings are now printed instead:
> >
> >     WARNING: Can not generate core dump on remote target.  
> 
> These warnings are newly introduced by the patch.

Yes, I saw that.

> They are meant to
> improve diagnostics when someone attempts to run the tests on a "real"
> remote target.  I wanted to clearly document the fact that this is
> unsupported (and always was).  Also, by documenting this restriction,
> maybe someone feels encouraged to lift it ;-)

In the distant past, I used to run the testsuite against resource
constrained linux machines often of a different architecture from the
host I was running the tests from.  These machines would run gdbserver
built for that architecture.

Now, I don't recall whether corefile support in the testsuite actually
worked for those targets, but it at least seems possible due to the
various invocations of remote_exec which are present (prior to your
patch).

Do you think you could restore those calls to remote_exec in your
patch?  Or do you know for a fact that they do not work?

Kevin

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Andreas Arnez]

On Wed, Oct 11 2017, Kevin Buettner wrote:

> On Mon, 09 Oct 2017 20:46:21 +0200
> Andreas Arnez <arnez@linux.vnet.ibm.com> wrote:
>
>> > Instead, several warnings are now printed instead:
>> >
>> >     WARNING: Can not generate core dump on remote target.  
>> 
>> These warnings are newly introduced by the patch.
>
> Yes, I saw that.
>
>> They are meant to
>> improve diagnostics when someone attempts to run the tests on a "real"
>> remote target.  I wanted to clearly document the fact that this is
>> unsupported (and always was).  Also, by documenting this restriction,
>> maybe someone feels encouraged to lift it ;-)
>
> In the distant past, I used to run the testsuite against resource
> constrained linux machines often of a different architecture from the
> host I was running the tests from.  These machines would run gdbserver
> built for that architecture.

OK, in that scenario, core file test cases would need to be roughly
executed in four steps:

(1) Build executable.
(2) Run executable on the remote machine.
(3) Transfer core dump to the local host.
(4) Start GDB on the local host.

AFAIK, steps (2) and (3) have not been implemented for the GDB test
suite so far.

> Now, I don't recall whether corefile support in the testsuite actually
> worked for those targets, but it at least seems possible due to the
> various invocations of remote_exec which are present (prior to your
> patch).

Fairly unlikely.  See, for instance, the beginning of corefile.exp:

  # are we on a target board
  if ![isnative] then {
      return
  }

A check like this appears in all core dump test cases.

Also note that the core_find command in gdb.exp uses expect's "system"
command for invoking the executable:

  catch "system \"(cd ${coredir}; ulimit -c unlimited; ${binfile} ${arg}; true) >/dev/null 2>&1\""

Which means that this command line will be executed on the local machine
and will fail in a remote setup.

But you're right that there were remote_exec invocations; mostly for
file handling such as:

  remote_exec build "mv $i $destcore"

  remote_exec build "mv $corefile $destcore"

  remote_exec build "rmdir $coredir"

TBH, I don't quite understand their intention.  In a native remote
setup, "build" is the remote machine.  Moving the core file around there
doesn't make it appear on the local machine.  And in a cross remote
setup, "build" is the local machine, whereas the core dump should be
generated on the remote machine.  So I think these commands only work if
host == build == target.  Since I found this too misleading, I replaced
them by local commands.

> Do you think you could restore those calls to remote_exec in your
> patch?  Or do you know for a fact that they do not work?

I'm pretty sure they don't.  Thus I wanted to document this restriction
more clearly.

--
Andreas

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Pedro Alves]

On 10/09/2017 07:46 PM, Andreas Arnez wrote:

> @@ -5883,7 +5883,7 @@ proc run_and_get_core {binfile {arg ""}} {
>  # specified.  Return that path name, or "" if no core file was found.
>  
>  proc find_core {binfile coredir {destcore ""}} {
> -    if {[is_remote target]} {
> +    if {![isnative]} {
>  	warning "Can not access remote core file."
>  	return ""
>      }

This seems incorrect to me.  "isnative" only checks
if the build and target _triplets_ are the same.  So
foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate
machine still returns isnative==true.

I think the real problem is that the native-gdbserver board
returns true to is_remote, when I think it shouldn't.

Doing that alone results in fallout in the testsuite, of
course.  I'm seeing if fixing it is doable.

Thanks,
Pedro Alves

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Pedro Alves]

On 10/12/2017 02:47 PM, Pedro Alves wrote:
> On 10/09/2017 07:46 PM, Andreas Arnez wrote:
> 
>> @@ -5883,7 +5883,7 @@ proc run_and_get_core {binfile {arg ""}} {
>>  # specified.  Return that path name, or "" if no core file was found.
>>  
>>  proc find_core {binfile coredir {destcore ""}} {
>> -    if {[is_remote target]} {
>> +    if {![isnative]} {
>>  	warning "Can not access remote core file."
>>  	return ""
>>      }
> 
> This seems incorrect to me.  "isnative" only checks
> if the build and target _triplets_ are the same.  So
> foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate
> machine still returns isnative==true.
> 
> I think the real problem is that the native-gdbserver board
> returns true to is_remote, when I think it shouldn't.
> 
> Doing that alone results in fallout in the testsuite, of
> course.  I'm seeing if fixing it is doable.

It's doable.  Below's what I have.  This exposed a number of
tests that were skipped for stale reasons.

The new "set cwd" command helps with this, which is nice
in the "I love it when a plan comes together" sense.  :-)

I should probably split this up and submit in pieces.

From 111e58e1310450539f1b1840666b3c2200e2364f Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 12 Oct 2017 14:45:57 +0100
Subject: [PATCH] native-gdbserver no longer is_remote and fix fallout

---
 gdb/testsuite/boards/gdbserver-base.exp            |   8 +
 gdb/testsuite/boards/native-extended-gdbserver.exp |   8 -
 gdb/testsuite/gdb.base/annota1.exp                 |   8 +-
 gdb/testsuite/gdb.base/annota3.exp                 |   8 +-
 gdb/testsuite/gdb.base/argv0-symlink.exp           |   5 +-
 gdb/testsuite/gdb.base/auxv.exp                    |  11 +-
 gdb/testsuite/gdb.base/catch-fork-static.exp       |   4 -
 gdb/testsuite/gdb.base/checkpoint.exp              |  10 +-
 gdb/testsuite/gdb.base/corefile.exp                |  16 +-
 gdb/testsuite/gdb.base/dprintf-non-stop.exp        |   6 -
 gdb/testsuite/gdb.base/find-unmapped.exp           |   3 +-
 gdb/testsuite/gdb.base/inferior-died.exp           |  13 +-
 gdb/testsuite/gdb.base/label.exp                   |   6 -
 gdb/testsuite/gdb.base/remote.exp                  |   7 +-
 gdb/testsuite/gdb.base/reread.exp                  |  68 +++----
 gdb/testsuite/gdb.base/scope.exp                   |  15 +-
 gdb/testsuite/gdb.base/shlib-call.exp              |  26 +--
 gdb/testsuite/gdb.base/solib-nodir.exp             |  13 +-
 gdb/testsuite/gdb.base/solib-search.exp            |  12 +-
 gdb/testsuite/gdb.base/term.exp                    |   4 +-
 gdb/testsuite/gdb.base/testenv.c                   |   6 +
 gdb/testsuite/gdb.base/testenv.exp                 | 206 ++++++++++++++-------
 gdb/testsuite/gdb.base/valgrind-db-attach.exp      |   4 +-
 gdb/testsuite/gdb.base/valgrind-disp-step.exp      |   5 -
 gdb/testsuite/gdb.base/valgrind-infcall.exp        |   5 -
 gdb/testsuite/gdb.cp/annota2.exp                   |   8 +-
 gdb/testsuite/gdb.cp/annota3.exp                   |   8 +-
 gdb/testsuite/gdb.multi/bkpt-multi-exec.exp        |   2 +-
 gdb/testsuite/gdb.python/py-evsignal.exp           |   3 +-
 gdb/testsuite/gdb.python/py-evthreads.exp          |   6 -
 gdb/testsuite/gdb.threads/non-ldr-exc-1.exp        |   5 -
 gdb/testsuite/gdb.threads/non-ldr-exc-2.exp        |   5 -
 gdb/testsuite/gdb.threads/non-ldr-exc-3.exp        |   5 -
 gdb/testsuite/gdb.threads/non-ldr-exc-4.exp        |   5 -
 gdb/testsuite/gdb.threads/thread_events.exp        |   8 +-
 gdb/testsuite/lib/gdb.exp                          |  12 +-
 gdb/testsuite/lib/selftest-support.exp             |  14 +-
 37 files changed, 304 insertions(+), 254 deletions(-)

diff --git a/gdb/testsuite/boards/gdbserver-base.exp b/gdb/testsuite/boards/gdbserver-base.exp
index 13321d6..04bdfa6 100644
--- a/gdb/testsuite/boards/gdbserver-base.exp
+++ b/gdb/testsuite/boards/gdbserver-base.exp
@@ -33,6 +33,14 @@ set_board_info gdb,predefined_tsv "\\\$trace_timestamp"
 
 set GDBFLAGS "${GDBFLAGS} -ex \"set auto-connect-native-target off\""
 
+# By default, dejagnu makes the board remote unless the board name
+# matches localhost.  Force it to be NOT remote.
+global board
+global board_info
+# Remove any target variant specifications from the name.
+set baseboard [lindex [split $board "/"] 0]
+set board_info($baseboard,isremote) 0
+
 proc ${board}_file { dest op args } {
     if { $op == "delete" } {
 	return 0
diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
index 4ea210f..9c35b12 100644
--- a/gdb/testsuite/boards/native-extended-gdbserver.exp
+++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
@@ -23,14 +23,6 @@
 load_generic_config "extended-gdbserver"
 load_board_description "gdbserver-base"
 
-# By default, dejagnu makes the board remote unless the board name
-# matches localhost.  Force it to be NOT remote.
-global board
-global board_info
-# Remove any target variant specifications from the name.
-set baseboard [lindex [split $board "/"] 0]
-set board_info($baseboard,isremote) 0
-
 set_board_info sockethost "localhost:"
 
 # We will be using the extended GDB remote protocol.
diff --git a/gdb/testsuite/gdb.base/annota1.exp b/gdb/testsuite/gdb.base/annota1.exp
index dd02bb4..bb3dc2f 100644
--- a/gdb/testsuite/gdb.base/annota1.exp
+++ b/gdb/testsuite/gdb.base/annota1.exp
@@ -16,12 +16,10 @@
 # This file was written by Elena Zannoni (ezannoni@cygnus.com)
 
 
-# are we on a target board? If so, don't run these tests.
-# note: this is necessary because we cannot use runto_main (which would
-# work for remote targets too) because of the different prompt we get
-# when using annotation level 2.
+# This testcase cannot use runto_main because of the different prompt
+# we get when using annotation level 2.
 #
-if [is_remote target] then {
+if ![target_can_use_run_cmd] {
     return 0
 }
 
diff --git a/gdb/testsuite/gdb.base/annota3.exp b/gdb/testsuite/gdb.base/annota3.exp
index ceb1682..5a971f0 100644
--- a/gdb/testsuite/gdb.base/annota3.exp
+++ b/gdb/testsuite/gdb.base/annota3.exp
@@ -16,12 +16,10 @@
 # This file was written by Elena Zannoni (ezannoni@cygnus.com)
 
 
-# are we on a target board? If so, don't run these tests.
-# note: this is necessary because we cannot use runto_main (which would
-# work for remote targets too) because of the different prompt we get
-# when using annotation level 2.
+# This testcase cannot use runto_main because of the different prompt
+# we get when using annotation level 2.
 #
-if [is_remote target] then {
+if ![target_can_use_run_cmd] {
     return 0
 }
 
diff --git a/gdb/testsuite/gdb.base/argv0-symlink.exp b/gdb/testsuite/gdb.base/argv0-symlink.exp
index a1248ed..1a4b0a6 100644
--- a/gdb/testsuite/gdb.base/argv0-symlink.exp
+++ b/gdb/testsuite/gdb.base/argv0-symlink.exp
@@ -80,8 +80,9 @@ gdb_test_no_output "set print repeats 10000"
 gdb_test_no_output "set print elements 10000"
 
 if { $has_argv0 } {
-    # gdbserver does not have this issue.
-    if ![is_remote target] {
+    # gdbserver in extended-remote mode does not have this issue.
+    # Plain remote does, however.
+    if {[target_info gdb_protocol] != "extended-remote"} {
 	setup_kfail "*-*-*" gdb/15934
     }
     gdb_test {print argv[0]} "/$dirlink/$filelink\"" $test
diff --git a/gdb/testsuite/gdb.base/auxv.exp b/gdb/testsuite/gdb.base/auxv.exp
index 833a1fd..06236b4 100644
--- a/gdb/testsuite/gdb.base/auxv.exp
+++ b/gdb/testsuite/gdb.base/auxv.exp
@@ -38,10 +38,12 @@ if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable \
 }
 
 # Use a fresh directory to confine the native core dumps.
-# Make it the working directory for gdb and its child.
+# Make it the working directory for the inferior.
 set coredir [standard_output_file coredir.[getpid]]
 file mkdir $coredir
-set core_works [expr [isnative] && ! [is_remote target]]
+set core_works [expr [isnative] \
+		&& ! [is_remote target] \
+		&& ! [target_info exists use_gdb_stub]]
 
 # Run GDB on the test program up to where it will dump core.
 
@@ -50,8 +52,9 @@ gdb_test_no_output "set print sevenbit-strings"
 gdb_test_no_output "set width 0"
 
 if {$core_works} {
-    if {[gdb_test "cd $coredir" ".*Working directory .*" \
-	     "cd to temporary directory for core dumps"]} {
+    verbose -log "coredir = $coredir"
+    if {[gdb_test_no_output "set cwd $coredir" \
+	     "set cwd to temporary directory for core dumps"]} {
 	set core_works 0
     }
 }
diff --git a/gdb/testsuite/gdb.base/catch-fork-static.exp b/gdb/testsuite/gdb.base/catch-fork-static.exp
index 2a96004..0a4bc63 100644
--- a/gdb/testsuite/gdb.base/catch-fork-static.exp
+++ b/gdb/testsuite/gdb.base/catch-fork-static.exp
@@ -21,10 +21,6 @@
 # ld.so probes before reaching main, and ptrace flags were set then.  But a
 # static executable would just keep running and never catch the fork.
 
-if { [is_remote target] || ![isnative] } then {
-    continue
-}
-
 # Until "catch fork" is implemented on other targets...
 #
 if { ![istarget "*-*-linux*"] && ![istarget "*-*-openbsd*"] } then {
diff --git a/gdb/testsuite/gdb.base/checkpoint.exp b/gdb/testsuite/gdb.base/checkpoint.exp
index 677c389..b93c5f4 100644
--- a/gdb/testsuite/gdb.base/checkpoint.exp
+++ b/gdb/testsuite/gdb.base/checkpoint.exp
@@ -13,10 +13,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-if { [is_remote target] || ![isnative] } then {
-    continue
-}
-
 # Until "set follow-fork-mode" and "catch fork" are implemented on
 # other targets...
 #
@@ -24,6 +20,12 @@ if {![istarget "*-*-linux*"]} then {
     continue
 }
 
+# Checkpoint support is currently implemented in the Linux native
+# target, so only works with "target native".
+if { [target_info gdb_protocol] != "" } {
+    continue
+}
+
 # Must name the source file explicitly, otherwise when driven by
 # checkpoints-ns.exp, we'd try compiling checkpoints-ns.c, which
 # doesn't exist.
diff --git a/gdb/testsuite/gdb.base/corefile.exp b/gdb/testsuite/gdb.base/corefile.exp
index 21b974f..63127a2 100644
--- a/gdb/testsuite/gdb.base/corefile.exp
+++ b/gdb/testsuite/gdb.base/corefile.exp
@@ -188,6 +188,13 @@ gdb_test "core" "No core file now."
 proc corefile_test_run {} {
     global corefile gdb_prompt
 
+    # This test assumes that GDB finds the default run target when
+    # already debugging a core, so it can't work if the board already
+    # wants to connect to something.
+    if { [target_info gdb_protocol] != "" } {
+	return
+    }
+
     gdb_test "core-file $corefile" "Core was generated by .*" "run: load core again"
     gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "run: sanity check we see the core file"
 
@@ -244,7 +251,14 @@ gdb_exit
 proc corefile_test_attach {} {
     global binfile corefile gdb_prompt
 
-    if ![is_remote target] {
+    # This test assumes that GDB finds the default run target when
+    # already debugging a core, so it can't work if the board already
+    # wants to connect to something.
+    if { [target_info gdb_protocol] != "" } {
+	return
+    }
+
+    if [can_spawn_for_attach] {
 	set test "attach: spawn sleep"
 	set res [remote_spawn host "$binfile sleep"]
 	if { $res < 0 || $res == "" } {
diff --git a/gdb/testsuite/gdb.base/dprintf-non-stop.exp b/gdb/testsuite/gdb.base/dprintf-non-stop.exp
index fed51ef..1df933b 100644
--- a/gdb/testsuite/gdb.base/dprintf-non-stop.exp
+++ b/gdb/testsuite/gdb.base/dprintf-non-stop.exp
@@ -13,12 +13,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-if [is_remote target] then {
-    # Testing with remote/non-stop is racy at the moment.
-    unsupported "testing dprintf with remote/non-stop is not supported."
-    return 0
-}
-
 standard_testfile
 set executable ${testfile}
 
diff --git a/gdb/testsuite/gdb.base/find-unmapped.exp b/gdb/testsuite/gdb.base/find-unmapped.exp
index 9982d0c..05feda4 100644
--- a/gdb/testsuite/gdb.base/find-unmapped.exp
+++ b/gdb/testsuite/gdb.base/find-unmapped.exp
@@ -13,7 +13,8 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-if {[is_remote target]} {
+if {[target_info gdb_protocol] == "remote"
+    || [target_info gdb_protocol] == "extended-remote"} {
     # gdbserver prints the warning message but expect is parsing only the
     # GDB output, not the gdbserver output.
     return 0
diff --git a/gdb/testsuite/gdb.base/inferior-died.exp b/gdb/testsuite/gdb.base/inferior-died.exp
index f2005fa..6f8b6d7 100644
--- a/gdb/testsuite/gdb.base/inferior-died.exp
+++ b/gdb/testsuite/gdb.base/inferior-died.exp
@@ -13,11 +13,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-if { [is_remote target] || ![isnative] } then {
-    unsupported "inferior-died.exp"
-    continue
-}
-
 # Until "set follow-fork-mode" and "catch fork" are implemented on
 # other targets...
 #
@@ -33,12 +28,16 @@ if { ![support_displaced_stepping] } {
 
 standard_testfile .c
 
-if { [prepare_for_testing "failed to prepare" ${testfile} ${testfile}.c] } {
+if { [build_executable "failed to build" ${testfile} ${testfile}.c] } {
     return -1
 }
 
+save_vars { GDBFLAGS } {
+    append GDBFLAGS " -ex \"set non-stop 1\""
+    clean_restart ${binfile}
+}
+
 gdb_test_no_output "set detach-on-fork off"
-gdb_test_no_output "set non-stop on"
 
 if ![runto_main] {
     return
diff --git a/gdb/testsuite/gdb.base/label.exp b/gdb/testsuite/gdb.base/label.exp
index d3939b6..3560167 100644
--- a/gdb/testsuite/gdb.base/label.exp
+++ b/gdb/testsuite/gdb.base/label.exp
@@ -13,12 +13,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-
-if [is_remote target] then {
-    return 0
-}
-
-
 #
 # test running programs
 #
diff --git a/gdb/testsuite/gdb.base/remote.exp b/gdb/testsuite/gdb.base/remote.exp
index 7f89b28..b8912b9 100644
--- a/gdb/testsuite/gdb.base/remote.exp
+++ b/gdb/testsuite/gdb.base/remote.exp
@@ -13,10 +13,9 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-
-
-# test only on a remote target board
-if {! [is_remote target]} {
+# Test only on a "target remote" board.
+if {[target_info gdb_protocol] != "remote"
+    && [target_info gdb_protocol] != "extended-remote"} {
     return
 }
 
diff --git a/gdb/testsuite/gdb.base/reread.exp b/gdb/testsuite/gdb.base/reread.exp
index 4e611ce..5a992b1 100644
--- a/gdb/testsuite/gdb.base/reread.exp
+++ b/gdb/testsuite/gdb.base/reread.exp
@@ -94,51 +94,39 @@ foreach_with_prefix opts {
 	# and reset the breakpoints correctly.
 	# Should see "Breakpoint 1, foo () at reread2.c:9"
 
-	set test "run to foo() second time"
-	if [is_remote target] {
-	    unsupported $test
-	} else {
-	    gdb_run_cmd
-	    gdb_test "" "Breakpoint.* foo .* at .*:9.*" $test
-	}
-
+	gdb_run_cmd
+	gdb_test "" "Breakpoint.* foo .* at .*:9.*" "run to foo() second time"
 
 	### Second pass: verify that GDB checks the executable file's
 	### timestamp when the program is *restarted*, not just when it exits.
 
-	if [is_remote target] {
-	    unsupported "second pass: GDB should check for changes before running"
-	} else {
-
-	    # Put the older executable back in place.
-	    gdb_rename_execfile ${binfile} ${binfile2}
-	    gdb_rename_execfile ${binfile1} ${binfile}
-
-	    # Restart GDB entirely.
-	    clean_restart ${binfile}
-
-	    # Set a breakpoint on foo and run to it.
-	    gdb_test "break foo" \
-		"Breakpoint.*at.* file .*$srcfile1, line 14.*" \
-		"second pass: breakpoint foo in first file"
-	    gdb_run_cmd
-	    gdb_test "" "Breakpoint.* foo .* at .*$srcfile1:14.*" \
-		"second pass: run to foo()"
-
-	    # This time, let the program run to completion.  If GDB checks the
-	    # executable file's timestamp now, it won't notice any change.
-	    gdb_continue_to_end "second pass"
-
-	    # Now move the newer executable into place, and re-run.  GDB
-	    # should still notice that the executable file has changed,
-	    # and still re-set the breakpoint appropriately.
-	    gdb_rename_execfile ${binfile} ${binfile1}
-	    gdb_rename_execfile ${binfile2} ${binfile}
-	    gdb_run_cmd
-	    gdb_test "" "Breakpoint.* foo .* at .*:9.*" \
-		"second pass: run to foo() second time"
-	}
+	# Put the older executable back in place.
+	gdb_rename_execfile ${binfile} ${binfile2}
+	gdb_rename_execfile ${binfile1} ${binfile}
+
+	# Restart GDB entirely.
+	clean_restart ${binfile}
+
+	# Set a breakpoint on foo and run to it.
+	gdb_test "break foo" \
+	    "Breakpoint.*at.* file .*$srcfile1, line 14.*" \
+	    "second pass: breakpoint foo in first file"
+	gdb_run_cmd
+	gdb_test "" "Breakpoint.* foo .* at .*$srcfile1:14.*" \
+	    "second pass: run to foo()"
+
+	# This time, let the program run to completion.  If GDB checks the
+	# executable file's timestamp now, it won't notice any change.
+	gdb_continue_to_end "second pass"
 
+	# Now move the newer executable into place, and re-run.  GDB
+	# should still notice that the executable file has changed,
+	# and still re-set the breakpoint appropriately.
+	gdb_rename_execfile ${binfile} ${binfile1}
+	gdb_rename_execfile ${binfile2} ${binfile}
+	gdb_run_cmd
+	gdb_test "" "Breakpoint.* foo .* at .*:9.*" \
+	    "second pass: run to foo() second time"
     }
 # End of tests.
 
diff --git a/gdb/testsuite/gdb.base/scope.exp b/gdb/testsuite/gdb.base/scope.exp
index 48d8245..a0ef6b6 100644
--- a/gdb/testsuite/gdb.base/scope.exp
+++ b/gdb/testsuite/gdb.base/scope.exp
@@ -505,16 +505,13 @@ setup_xfail "rs6000-*-*"
 
 gdb_test "print 'scope0.c'::filelocal_ro" "= 201"
 
-# gdb currently cannot access bss memory on some targets if the inferior
-# is not running.
+# Check that gdb can access bss memory if the inferior is not running.
 #
-# For PA boards using monitor/remote-pa.c, the bss test is going to 
-# randomly fail.  We've already put remote-pa on the target stack,
-# so we actually read memory from the board.  Problem is crt0.o
-# is responsible for clearing bss and that hasnt' happened yet.
-#
-# This is a problem for all non-native targets. -- manson
-if [is_remote target] {
+# For embedded stub-like boards, this test is going to randomly fail.
+# We've already put target remote on the target stack, so we actually
+# read memory from the board.  Problem is crt0.o is responsible for
+# clearing bss and that hasn't happened yet.
+if [target_info exists use_gdb_stub] {
     unsupported "print 'scope0.c'::filelocal_bss before run"
 } else {
     gdb_test "print 'scope0.c'::filelocal_bss" "= 0" \
diff --git a/gdb/testsuite/gdb.base/shlib-call.exp b/gdb/testsuite/gdb.base/shlib-call.exp
index e1664da..2c9b495 100644
--- a/gdb/testsuite/gdb.base/shlib-call.exp
+++ b/gdb/testsuite/gdb.base/shlib-call.exp
@@ -170,18 +170,22 @@ gdb_test_no_output "set width 0"
 # test that we can re-set breakpoints in shared libraries
 gdb_breakpoint "shr1" "allow-pending"
 
-# FIXME: should not send "run" explicitly.  Non-portable.
-
-if ![is_remote target] {
-  gdb_test "run" "Starting program:.*Breakpoint .,.*" \
-	"run to bp in shared library"
-
-  gdb_continue_to_end "" continue 1
+set test "run to bp in shared library"
+gdb_run_cmd
+gdb_test_multiple "" $test {
+    -re "Breakpoint .,.*${gdb_prompt} " {
+	pass $test
+    }
+}
 
-  gdb_test "run" "Starting program:.*Breakpoint .,.*" \
-	"re-run to bp in shared library (PR's 16495, 18213)"
+gdb_continue_to_end "" continue 1
 
-  gdb_continue_to_end "" continue 1
+set test "re-run to bp in shared library (PR's 16495, 18213)"
+gdb_run_cmd
+gdb_test_multiple "" $test {
+    -re "Breakpoint .,.*${gdb_prompt} " {
+	pass $test
+    }
 }
 
-return 0
+gdb_continue_to_end "" continue 1
diff --git a/gdb/testsuite/gdb.base/solib-nodir.exp b/gdb/testsuite/gdb.base/solib-nodir.exp
index 9cb682e..800ca01 100644
--- a/gdb/testsuite/gdb.base/solib-nodir.exp
+++ b/gdb/testsuite/gdb.base/solib-nodir.exp
@@ -13,8 +13,14 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-# are we on a target board
-if {[is_remote target] || [skip_shlib_tests]} {
+if {[skip_shlib_tests]} {
+    return
+}
+
+# We need to be able to influence the target's environment and working
+# directory.  Can't do that if when we connect the inferior is already
+# running.
+if [target_info exists use_gdb_stub] {
     return
 }
 
@@ -44,7 +50,8 @@ clean_restart $executable
 gdb_load_shlib ${binlibfile}
 
 gdb_test_no_output "set env LD_LIBRARY_PATH=:"
-gdb_test "cd ${binlibfiledir}" "Working directory [string_to_regexp ${binlibfiledir}]\\." "cd OBJDIR/${subdir}"
+gdb_test_no_output "set cwd ${binlibfiledir}" \
+    "set cwd OBJDIR/${subdir}"
 
 set test "library loaded"
 if [runto_main] {
diff --git a/gdb/testsuite/gdb.base/solib-search.exp b/gdb/testsuite/gdb.base/solib-search.exp
index 7272a76..a38f7ba 100644
--- a/gdb/testsuite/gdb.base/solib-search.exp
+++ b/gdb/testsuite/gdb.base/solib-search.exp
@@ -16,11 +16,19 @@
 # Test solib-search-path, and in the case of solib-svr4.c whether l_addr_p
 # is properly reset when the path is changed.
 
-if {[is_remote target] || [skip_shlib_tests]} {
-    untested "skipping remote target and shared library tests"
+if [skip_shlib_tests] {
+    untested "skipping shared library tests"
     return -1
 }
 
+# The testcase invokes commands on the build system, expecting that
+# the effects are visible on the host.
+if [is_remote host] {
+    untested "skipping remote host"
+    return -1
+}
+
+
 # This is required by gdb_compile_shlib.
 if {[get_compiler_info]} {
     warning "Could not get compiler info"
diff --git a/gdb/testsuite/gdb.base/term.exp b/gdb/testsuite/gdb.base/term.exp
index c922deb..7c3f1cb 100644
--- a/gdb/testsuite/gdb.base/term.exp
+++ b/gdb/testsuite/gdb.base/term.exp
@@ -13,8 +13,8 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-# Don't try this for remote targets.
-if [is_remote target] then {
+# Don't try this for non-"target native" targets.
+if {[target_info gdb_protocol] != ""} {
     continue
 }
 
diff --git a/gdb/testsuite/gdb.base/testenv.c b/gdb/testsuite/gdb.base/testenv.c
index 11e6bd3..bbc851e 100644
--- a/gdb/testsuite/gdb.base/testenv.c
+++ b/gdb/testsuite/gdb.base/testenv.c
@@ -24,6 +24,11 @@
 #include <stdio.h>
 #include <string.h>
 
+static void
+vars_counted (void)
+{
+}
+
 int main (int argc, char **argv, char **envp)
 
 {
@@ -39,6 +44,7 @@ int main (int argc, char **argv, char **envp)
 	  }
       }
     printf ("Program found %d variables starting with TEST_GDB\n", j);
+    vars_counted ();
     return 0; /* set breakpoint here.  */
 }
 
diff --git a/gdb/testsuite/gdb.base/testenv.exp b/gdb/testsuite/gdb.base/testenv.exp
index 52becfd..6f13de8 100644
--- a/gdb/testsuite/gdb.base/testenv.exp
+++ b/gdb/testsuite/gdb.base/testenv.exp
@@ -18,9 +18,10 @@
 # Check if environment variables are correctly passed to inferiors
 #
 
-# There's no support for passing environment variables in the remote protocol.
-if { [is_remote target] } {
-    return 0
+# Can't pass environment variables to the inferior if when we connect,
+# the inferior is already running.
+if [target_info exists use_gdb_stub] {
+    return
 }
 
 standard_testfile .c
@@ -32,92 +33,155 @@ if { [prepare_for_testing "failed to prepare" ${binfile} ${srcfile}] } {
      return -1
 }
 
-# make sure $pc is sane, in case we're talking to a board.
-if { ![runto_main] } {
-    gdb_suppress_tests
+# Test that the the inferior sees EXPECTED env vars starting with
+# "TEST_GDB".
+proc test_num_test_vars {expected message} {
+    set num [get_integer_valueof "j" -1 "$message, get num vars"]
+    gdb_assert {$num == $expected} "$message, confirmed"
 }
 
-set bp_line [gdb_get_line_number "set breakpoint here"]
-gdb_breakpoint  $bp_line
+# Restart test program, and prepare for another test sequence.
+# Returns true on success.
+proc run_and_count_vars {} {
+    global gdb_prompt
+
+    if ![runto "vars_counted"] {
+	return 0
+    }
+    gdb_test_multiple "up" "up" {
+	-re "#1 .*main.*$gdb_prompt $" {
+	}
+    }
+    return 1
+}
+
+# Find environment variable named VARNAME (peeking inferior variables
+# directly), and return its value.  Returns "<not found>" if not
+# found.
+
+proc find_env {varname} {
+    global gdb_prompt
+
+    for {set i 0} {1} {incr i} {
+	set test "printf \"var: %s\\n\", envp\[$i\] ? envp\[$i\] : \"\""
+	set var ""
+	gdb_test_multiple $test $test {
+	    -re "var: \r\n$gdb_prompt $" {
+		return "<not found>"
+	    }
+	    -re "var: \(\[^\r\n\]*\)\r\n$gdb_prompt $" {
+		set var $expect_out(1,string)
+	    }
+	}
+
+	if {[string match "$varname=*" $var]} {
+	    set from [expr [string first "=" $var] + 1]
+	    set to [string length $var]
+	    return [string range $var $from $to]
+	}
+    }
+}
 
 #
 # Test gdb set/unset environment commands.
-# Executable lists and counts all environment variables
+# The executable lists and counts all environment variables
 # starting with TEST_GDB.
 
+proc_with_prefix test_set_unset_env {} {
+    global binfile
+
+    clean_restart $binfile
+
+    # First test with no TEST_GDB_VAR.
+    with_test_prefix "test1" {
+	if ![run_and_count_vars] {
+	    return
+	}
+	test_num_test_vars 0 "no TEST_GDB vars"
+    }
+
+    # Second test with one TEST_GDB_VAR.
+    with_test_prefix "test2" {
+	gdb_test_no_output "set env TEST_GDB_VAR1 test1" \
+	    "set TEST_GDB_VAR1"
+
+	if ![run_and_count_vars] {
+	    return
+	}
+	test_num_test_vars 1 "one TEST_GDB var"
+    }
+
+    # Third test with two TEST_GDB_VAR.
+    with_test_prefix "test3" {
+	gdb_test_no_output "set env TEST_GDB_VAR2 test2" \
+	    "set TEST_GDB_VAR2"
+
+	if ![run_and_count_vars] {
+	    return
+	}
+
+	test_num_test_vars 2 "two TEST_GDB var"
+   }
+
+    # Fourth test with one TEST_GDB_VAR left, after one was removed
+    # with unset command.
+    with_test_prefix "test4" {
+	gdb_test_no_output "unset env TEST_GDB_VAR1" \
+	    "unset TEST_GDB_VAR1"
+
+	if ![run_and_count_vars] {
+	    return
+	}
+
+	test_num_test_vars 1 "one TEST_GDB var, after unset"
+    }
+}
 
-# First test with no TEST_GDB_VAR
-gdb_test "continue" \
-  ".*Program found 0 variables starting with TEST_GDB.*" \
-  "test no TEST_GDB var"
-
-gdb_test_no_output "set env TEST_GDB_VAR1 test1" \
-  "set TEST_GDB_VAR1"
-
-runto_main
-gdb_breakpoint  $bp_line
+proc_with_prefix test_inherit_env_var {} {
+    global binfile
+    global env
 
-# Second test with one TEST_GDB_VAR
-gdb_test "continue" \
-  ".*Program found 1 variables starting with TEST_GDB.*" \
-  "test with one TEST_GDB var"
+    # This test assumes that the build's environ (where dejagnu runs)
+    # is the same as the host's (where gdb runs) environ.
+    if [is_remote host] {
+	return
+    }
 
-gdb_test_no_output "set env TEST_GDB_VAR2 test2" \
-  "set TEST_GDB_VAR2"
+    save_vars {env(TEST_GDB_GLOBAL)} {
+	set env(TEST_GDB_GLOBAL) "Global environment value"
 
-runto_main
-gdb_breakpoint  $bp_line
+	clean_restart $binfile
 
-# Third test with two TEST_GDB_VAR
-gdb_test "continue"  \
-  ".*Program found 2 variables starting with TEST_GDB.*" \
-  "test with two TEST_GDB var"
+	# First test with only inherited TEST_GDB_GLOBAL.
+	with_test_prefix "test1" {
+	    if ![run_and_count_vars] {
+		return
+	    }
 
-gdb_test_no_output "unset env TEST_GDB_VAR1" \
-  "unset TEST_GDB_VAR1"
+	    gdb_test "show env" ".*TEST_GDB_GLOBAL=.*" \
+		"test passing TEST_GDB_GLOBAL to GDB"
 
-runto_main
-gdb_breakpoint  $bp_line
+	    test_num_test_vars 1 "TEST_GDB_GLOBAL"
 
-# Fourth test with one TEST_GDB_VAR left, after one was removed
-# with unset command.
-gdb_test "continue"  \
-  ".*Program found 1 variables starting with TEST_GDB.*" \
-  "test with one TEST_GDB var, after unset"
+	    set var [find_env "TEST_GDB_GLOBAL"]
 
-gdb_exit
+	    gdb_assert {[string equal $var "Global environment value"]} \
+		"test with TEST_GDB_GLOBAL"
+	}
 
-set env(TEST_GDB_GLOBAL) "Global environment value"
+	# Second test with one TEST_GDB_VAR.
+	with_test_prefix "test2" {
+	    gdb_test_no_output "unset env TEST_GDB_GLOBAL" \
+		"unset TEST_GDB_GLOBAL"
 
-clean_restart $binfile
+	    if ![run_and_count_vars] {
+		return
+	    }
 
-# make sure $pc is sane, in case we're talking to a board.
-if { ![runto_main] } {
-    gdb_suppress_tests
+	    test_num_test_vars 0 "test with TEST_GDB_GLOBAL unset"
+	}
+    }
 }
 
-set bp_line [gdb_get_line_number "set breakpoint here"]
-gdb_breakpoint  $bp_line
-
-gdb_test "show env" ".*TEST_GDB_GLOBAL=.*" "test passing TEST_GDB_GLOBAL to GDB"
-# First test with only inherited TEST_GDB_GLOBAL
-gdb_test "continue" \
-  ".*TEST_GDB_GLOBAL=Global environment value.*Program found 1 variables starting with TEST_GDB.*" \
-  "test with TEST_GDB_GLOBAL"
-
-gdb_test_no_output "unset env TEST_GDB_GLOBAL" \
-  "unset TEST_GDB_GLOBAL"
-
-runto_main
-gdb_breakpoint  $bp_line
-
-# Second test with one TEST_GDB_VAR
-gdb_test "continue" \
-  ".*Program found 0 variables starting with TEST_GDB.*" \
-  "test with TEST_GDB_GLOBAL unset"
-
-gdb_exit
-
-# Clear environment in case we're doing multiple runs
-unset env(TEST_GDB_GLOBAL)
-
+test_set_unset_env
+test_inherit_env_var
diff --git a/gdb/testsuite/gdb.base/valgrind-db-attach.exp b/gdb/testsuite/gdb.base/valgrind-db-attach.exp
index 306132d..2a2963a 100644
--- a/gdb/testsuite/gdb.base/valgrind-db-attach.exp
+++ b/gdb/testsuite/gdb.base/valgrind-db-attach.exp
@@ -13,8 +13,8 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-if [is_remote target] {
-    # The test always runs locally.
+# Skip if already connected to some target.
+if { [target_info gdb_protocol] != "" } {
     return 0
 }
 
diff --git a/gdb/testsuite/gdb.base/valgrind-disp-step.exp b/gdb/testsuite/gdb.base/valgrind-disp-step.exp
index b904874..0e9e0d1 100644
--- a/gdb/testsuite/gdb.base/valgrind-disp-step.exp
+++ b/gdb/testsuite/gdb.base/valgrind-disp-step.exp
@@ -18,11 +18,6 @@
 # really tests is that GDB falls back to in-line stepping
 # automatically instead of getting stuck or crashing.
 
-if [is_remote target] {
-    # The test always runs locally.
-    return 0
-}
-
 standard_testfile .c
 if {[build_executable "failed to build" $testfile $srcfile {debug}] == -1} {
     return -1
diff --git a/gdb/testsuite/gdb.base/valgrind-infcall.exp b/gdb/testsuite/gdb.base/valgrind-infcall.exp
index e1222fa..6eefa3d 100644
--- a/gdb/testsuite/gdb.base/valgrind-infcall.exp
+++ b/gdb/testsuite/gdb.base/valgrind-infcall.exp
@@ -13,11 +13,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-if [is_remote target] {
-    # The test always runs locally.
-    return 0
-}
-
 standard_testfile .c
 if {[build_executable $testfile.exp $testfile $srcfile {debug}] == -1} {
     return -1
diff --git a/gdb/testsuite/gdb.cp/annota2.exp b/gdb/testsuite/gdb.cp/annota2.exp
index a00161d..78e8795 100644
--- a/gdb/testsuite/gdb.cp/annota2.exp
+++ b/gdb/testsuite/gdb.cp/annota2.exp
@@ -29,12 +29,10 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile \
     return -1
 }
 
-# are we on a target board? If so, don't run these tests.
-# note: this is necessary because we cannot use runto_main (which would
-# work for remote targets too) because of the different prompt we get
-# when using annotation level 2.
+# This testcase cannot use runto_main because of the different prompt
+# we get when using annotation level 2.
 #
-if [is_remote target] then {
+if ![target_can_use_run_cmd] {
     return 0
 }
 
diff --git a/gdb/testsuite/gdb.cp/annota3.exp b/gdb/testsuite/gdb.cp/annota3.exp
index 4215e76..3141251 100644
--- a/gdb/testsuite/gdb.cp/annota3.exp
+++ b/gdb/testsuite/gdb.cp/annota3.exp
@@ -29,12 +29,10 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile \
     return -1
 }
 
-# are we on a target board? If so, don't run these tests.
-# note: this is necessary because we cannot use runto_main (which would
-# work for remote targets too) because of the different prompt we get
-# when using annotation level 2.
+# This testcase cannot use runto_main because of the different prompt
+# we get when using annotation level 2.
 #
-if [is_remote target] then {
+if ![target_can_use_run_cmd] {
     return 0
 }
 
diff --git a/gdb/testsuite/gdb.multi/bkpt-multi-exec.exp b/gdb/testsuite/gdb.multi/bkpt-multi-exec.exp
index dba6697..f308aed 100644
--- a/gdb/testsuite/gdb.multi/bkpt-multi-exec.exp
+++ b/gdb/testsuite/gdb.multi/bkpt-multi-exec.exp
@@ -13,7 +13,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-if { [is_remote target] || ![isnative] } then {
+if { ![target_can_use_run_cmd] } then {
     continue
 }
 
diff --git a/gdb/testsuite/gdb.python/py-evsignal.exp b/gdb/testsuite/gdb.python/py-evsignal.exp
index 8a39f4e..4138b9c 100644
--- a/gdb/testsuite/gdb.python/py-evsignal.exp
+++ b/gdb/testsuite/gdb.python/py-evsignal.exp
@@ -13,7 +13,8 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-if {[is_remote target]} {
+if {[target_info gdb_protocol] == "remote"
+    || [target_info gdb_protocol] == "extended-remote"} {
     # RuntimeError: Could not find event thread
     kfail "python/12966" "Signal Thread 3"
     return -1
diff --git a/gdb/testsuite/gdb.python/py-evthreads.exp b/gdb/testsuite/gdb.python/py-evthreads.exp
index 64b2af5..a72966f 100644
--- a/gdb/testsuite/gdb.python/py-evthreads.exp
+++ b/gdb/testsuite/gdb.python/py-evthreads.exp
@@ -13,12 +13,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-if {[is_remote target]} {
-    # RuntimeError: Could not find event thread
-    kfail "python/12966" "Run to breakpoint 1"
-    return -1
-}
-
 if { ![support_displaced_stepping] } { 
     unsupported "displaced stepping"
     return -1
diff --git a/gdb/testsuite/gdb.threads/non-ldr-exc-1.exp b/gdb/testsuite/gdb.threads/non-ldr-exc-1.exp
index 689246f..558810d 100644
--- a/gdb/testsuite/gdb.threads/non-ldr-exc-1.exp
+++ b/gdb/testsuite/gdb.threads/non-ldr-exc-1.exp
@@ -16,11 +16,6 @@
 # Test that when a thread other than the main thread execs, we follow
 # through to the new incarnation of the main thread.
 
-# No exec event support in the remote protocol.
-if { [is_remote target] } then {
-    continue
-}
-
 standard_testfile
 set executable ${testfile}
 
diff --git a/gdb/testsuite/gdb.threads/non-ldr-exc-2.exp b/gdb/testsuite/gdb.threads/non-ldr-exc-2.exp
index 1b7be25..077c192 100644
--- a/gdb/testsuite/gdb.threads/non-ldr-exc-2.exp
+++ b/gdb/testsuite/gdb.threads/non-ldr-exc-2.exp
@@ -17,11 +17,6 @@
 # through to the new incarnation of the main thread, even if the main
 # thread had already exited before the exec.
 
-# No exec event support in the remote protocol.
-if { [is_remote target] } then {
-    continue
-}
-
 standard_testfile
 set executable ${testfile}
 
diff --git a/gdb/testsuite/gdb.threads/non-ldr-exc-3.exp b/gdb/testsuite/gdb.threads/non-ldr-exc-3.exp
index 74ee6a7..3c88d1d 100644
--- a/gdb/testsuite/gdb.threads/non-ldr-exc-3.exp
+++ b/gdb/testsuite/gdb.threads/non-ldr-exc-3.exp
@@ -19,11 +19,6 @@
 # non-ldr-exc-2.exp in that we have more than two threads in the
 # program when the exec happens.
 
-# No exec event support in the remote protocol.
-if { [is_remote target] } then {
-    continue
-}
-
 standard_testfile
 set executable ${testfile}
 
diff --git a/gdb/testsuite/gdb.threads/non-ldr-exc-4.exp b/gdb/testsuite/gdb.threads/non-ldr-exc-4.exp
index f306b9d..04e81e3 100644
--- a/gdb/testsuite/gdb.threads/non-ldr-exc-4.exp
+++ b/gdb/testsuite/gdb.threads/non-ldr-exc-4.exp
@@ -18,11 +18,6 @@
 # from non-ldr-exc-1.exp in that we have more than two threads in the
 # program when the exec happens.
 
-# No exec event support in the remote protocol.
-if { [is_remote target] } then {
-    continue
-}
-
 standard_testfile
 set executable ${testfile}
 
diff --git a/gdb/testsuite/gdb.threads/thread_events.exp b/gdb/testsuite/gdb.threads/thread_events.exp
index d8d19c8..82d81c1 100644
--- a/gdb/testsuite/gdb.threads/thread_events.exp
+++ b/gdb/testsuite/gdb.threads/thread_events.exp
@@ -27,9 +27,11 @@ if ![istarget *-*-linux*] then {
     return
 }
 
-# When using gdbserver, even on Linux, we don't get notifications
-# about new threads.  This is expected, so don't test for that.
-if [is_remote target] then {
+# When using "target remote", even on Linux with gdbserver, we don't
+# get notifications about new threads.  This is expected, so don't
+# test for that.
+if {[target_info gdb_protocol] == "remote"
+    || [target_info gdb_protocol] == "extended-remote"} {
     return
 }
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 9d8296a..cf04274 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -226,6 +226,16 @@ proc delete_breakpoints {} {
     }
 }
 
+# Returns true iff the target supports using the "run" command.
+
+proc target_can_use_run_cmd {} {
+    if [target_info exists use_gdb_stub] {
+	return 0
+    }
+
+    return 1
+}
+
 # Generic run command.
 #
 # The second pattern below matches up to the first newline *only*.
@@ -4107,7 +4117,7 @@ proc can_spawn_for_attach { } {
     # The "attach" command doesn't make sense when the target is
     # stub-like, where GDB finds the program already started on
     # initial connection.
-    if {[target_info exists use_gdb_stub]} {
+    if [target_info exists use_gdb_stub] {
 	return 0
     }
 
diff --git a/gdb/testsuite/lib/selftest-support.exp b/gdb/testsuite/lib/selftest-support.exp
index 987b5c8..e889c00 100644
--- a/gdb/testsuite/lib/selftest-support.exp
+++ b/gdb/testsuite/lib/selftest-support.exp
@@ -87,14 +87,16 @@ proc selftest_setup { executable function } {
     set timeout 600
 
     set description "run until breakpoint at $function"
-    gdb_test_multiple "run $INTERNAL_GDBFLAGS" "$description" {
-        -re "Starting program.*Breakpoint \[0-9\]+,.*$function \\(\\).* at .*main.c:.*$gdb_prompt $" {
+    gdb_test_no_output "set args $INTERNAL_GDBFLAGS"
+    gdb_run_cmd
+    gdb_test_multiple "" "$description" {
+        -re "Breakpoint \[0-9\]+,.*$function \\(\\).* at .*main.c:.*$gdb_prompt $" {
             pass "$description"
         }
-        -re "Starting program.*Breakpoint \[0-9\]+,.*$function \\(\\).*$gdb_prompt $" {
+        -re "Breakpoint \[0-9\]+,.*$function \\(\\).*$gdb_prompt $" {
             xfail "$description (line numbers scrambled?)"
         }
-	-re "Starting program.*Breakpoint \[0-9\]+,.* at .*main.c:.*$function.*$gdb_prompt $" {
+	-re "Breakpoint \[0-9\]+,.* at .*main.c:.*$function.*$gdb_prompt $" {
 	    # $function may be inlined, so the program stops at the line
 	    # calling $function.
 	    pass "$description"
@@ -124,7 +126,9 @@ proc selftest_setup { executable function } {
 proc do_self_tests {function body} {
     global GDB tool
 
-    # Are we on a target board.
+    # Are we testing with a remote board (which won't have access to
+    # the GDB binary and its auxilliary data files), or with a cross
+    # debugger?
     if { [is_remote target] || ![isnative] } then {
 	return
     }
-- 
2.5.5

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Pedro Alves]

On 10/12/2017 05:47 PM, Pedro Alves wrote:
> On 10/12/2017 02:47 PM, Pedro Alves wrote:
>> On 10/09/2017 07:46 PM, Andreas Arnez wrote:
>>
>>> @@ -5883,7 +5883,7 @@ proc run_and_get_core {binfile {arg ""}} {
>>>  # specified.  Return that path name, or "" if no core file was found.
>>>  
>>>  proc find_core {binfile coredir {destcore ""}} {
>>> -    if {[is_remote target]} {
>>> +    if {![isnative]} {
>>>  	warning "Can not access remote core file."
>>>  	return ""
>>>      }
>>
>> This seems incorrect to me.  "isnative" only checks
>> if the build and target _triplets_ are the same.  So
>> foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate
>> machine still returns isnative==true.
>>
>> I think the real problem is that the native-gdbserver board
>> returns true to is_remote, when I think it shouldn't.
>>
>> Doing that alone results in fallout in the testsuite, of
>> course.  I'm seeing if fixing it is doable.
> 
> It's doable.  Below's what I have.  This exposed a number of
> tests that were skipped for stale reasons.
> 
> The new "set cwd" command helps with this, which is nice
> in the "I love it when a plan comes together" sense.  :-)
> 
> I should probably split this up and submit in pieces.

FYI, this is now fully done and all pushed to master.
The native-gdbserver board no longer returns true
to [is_remote target].

Thanks,
Pedro Alves

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Andreas Arnez]

On Thu, Oct 12 2017, Pedro Alves wrote:

> On 10/09/2017 07:46 PM, Andreas Arnez wrote:
>
>> @@ -5883,7 +5883,7 @@ proc run_and_get_core {binfile {arg ""}} {
>>  # specified.  Return that path name, or "" if no core file was found.
>>  
>>  proc find_core {binfile coredir {destcore ""}} {
>> -    if {[is_remote target]} {
>> +    if {![isnative]} {
>>  	warning "Can not access remote core file."
>>  	return ""
>>      }
>
> This seems incorrect to me.  "isnative" only checks
> if the build and target _triplets_ are the same.  So
> foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate
> machine still returns isnative==true.

Exactly, that's why I originally wrote is_remote instead.  And I also
wondered why the core dump tests check isnative.  Does anyone run the
testsuite on a native remote setup?

> I think the real problem is that the native-gdbserver board
> returns true to is_remote, when I think it shouldn't.
>
> Doing that alone results in fallout in the testsuite, of
> course.  I'm seeing if fixing it is doable.

Right, this seems all a bit mixed up to me, similar to the confusing
remote_file and remote_exec operations in core_find.

--
Andreas

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Maciej W. Rozycki]

On Thu, 12 Oct 2017, Andreas Arnez wrote:

> > This seems incorrect to me.  "isnative" only checks
> > if the build and target _triplets_ are the same.  So
> > foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate
> > machine still returns isnative==true.
> 
> Exactly, that's why I originally wrote is_remote instead.  And I also
> wondered why the core dump tests check isnative.  Does anyone run the
> testsuite on a native remote setup?

 Well, I had cases where I did that, and offhand I can find two reasons:

1. You want to verify `gdbserver' itself rather than GDB, in which case 
   you may even run it locally (i.e. where the remote target is really 
   `localhost').

2. You want to test a feature (e.g. an extra register set) only your 
   target system has and it is too slow or unequipped to run DejaGNU 
   itself.

  Maciej

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Andreas Arnez]

On Fri, Oct 13 2017, Maciej W. Rozycki wrote:

> On Thu, 12 Oct 2017, Andreas Arnez wrote:
>
>> > This seems incorrect to me.  "isnative" only checks
>> > if the build and target _triplets_ are the same.  So
>> > foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate
>> > machine still returns isnative==true.
>> 
>> Exactly, that's why I originally wrote is_remote instead.  And I also
>> wondered why the core dump tests check isnative.  Does anyone run the
>> testsuite on a native remote setup?
>
>  Well, I had cases where I did that, and offhand I can find two reasons:
>
> 1. You want to verify `gdbserver' itself rather than GDB, in which case 
>    you may even run it locally (i.e. where the remote target is really 
>    `localhost').

This is the native-gdbserver setup, right?  In that case the core dump
tests should work...

> 2. You want to test a feature (e.g. an extra register set) only your 
>    target system has and it is too slow or unequipped to run DejaGNU 
>    itself.

...and in this case they shouldn't.  Do you remember whether you saw
FAILs from corefile.exp and friends?

--
Andreas

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Maciej W. Rozycki]

On Fri, 13 Oct 2017, Andreas Arnez wrote:

> >> Exactly, that's why I originally wrote is_remote instead.  And I also
> >> wondered why the core dump tests check isnative.  Does anyone run the
> >> testsuite on a native remote setup?
> >
> >  Well, I had cases where I did that, and offhand I can find two reasons:
> >
> > 1. You want to verify `gdbserver' itself rather than GDB, in which case 
> >    you may even run it locally (i.e. where the remote target is really 
> >    `localhost').
> 
> This is the native-gdbserver setup, right?  In that case the core dump
> tests should work...

 Yes and indeed.

> > 2. You want to test a feature (e.g. an extra register set) only your 
> >    target system has and it is too slow or unequipped to run DejaGNU 
> >    itself.
> 
> ...and in this case they shouldn't.  Do you remember whether you saw
> FAILs from corefile.exp and friends?

 Nope, sorry.  It was a while ago and I only looked for regressions with 
whatever I meant to verify and not preexisting failures anyway.

  Maciej

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Pedro Alves]

On 10/12/2017 06:00 PM, Andreas Arnez wrote:
> On Thu, Oct 12 2017, Pedro Alves wrote:
> 
>> On 10/09/2017 07:46 PM, Andreas Arnez wrote:
>>
>>> @@ -5883,7 +5883,7 @@ proc run_and_get_core {binfile {arg ""}} {
>>>  # specified.  Return that path name, or "" if no core file was found.
>>>  
>>>  proc find_core {binfile coredir {destcore ""}} {
>>> -    if {[is_remote target]} {
>>> +    if {![isnative]} {
>>>  	warning "Can not access remote core file."
>>>  	return ""
>>>      }
>>
>> This seems incorrect to me.  "isnative" only checks
>> if the build and target _triplets_ are the same.  So
>> foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate
>> machine still returns isnative==true.
> 
> Exactly, that's why I originally wrote is_remote instead.  And I also
> wondered why the core dump tests check isnative.  Does anyone run the
> testsuite on a native remote setup?

I think most if not all isnative checks are stale/bogus,
and likely were introduced at a time when cross debugging,
including cross-core debugging wasn't that well supported.
I'd be delighted to see them cleaned up and replaced with
a more appropriate check.

> Right, this seems all a bit mixed up to me, similar to the confusing
> remote_file and remote_exec operations in core_find.

I wonder whether it'd be equally simple to move in the opposite
direction of making these operations use the right FOO
"remote_exec FOO", making it easier to actually support remote
core dump testing.  (Maybe not by default, since uploading the
cores may be slow, but with a board option/setting.)

Thanks,
Pedro Alves

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Pedro Alves]

On 10/09/2017 07:46 PM, Andreas Arnez wrote:
> On Sat, Oct 07 2017, Kevin Buettner wrote:
> 
> [...]
> 
>> E.g. when I test your patch on my x86-64 linux box using the
>> following command...
>>
>>     make check RUNTESTFLAGS="--target_board=native-gdbserver"
>>
>> ...I see 32 fewer passes than before and also one more known failure.
>>
>> Here are the passes that no longer occur when using your patch:
> 
> [...]
> 
>> Instead, several warnings are now printed instead:
>>
>>     WARNING: Can not generate core dump on remote target.
> 
> These warnings are newly introduced by the patch.  They are meant to
> improve diagnostics when someone attempts to run the tests on a "real"
> remote target.  I wanted to clearly document the fact that this is
> unsupported (and always was).  Also, by documenting this restriction,
> maybe someone feels encouraged to lift it ;-)

Wouldn't an UNTESTED or UNSUPPORTED be better?  It's what
we tend to do with other cases of unsupported/untested tests.
Those convey what happened to the testscase, and are
accounted for and an untested/unsupported count is shown at
the end of a test run, while a count of warnings isn't.

Thanks,
Pedro Alves

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Maciej W. Rozycki]

On Tue, 17 Oct 2017, Pedro Alves wrote:

> >> E.g. when I test your patch on my x86-64 linux box using the
> >> following command...
> >>
> >>     make check RUNTESTFLAGS="--target_board=native-gdbserver"
> >>
> >> ...I see 32 fewer passes than before and also one more known failure.
> >>
> >> Here are the passes that no longer occur when using your patch:
> > 
> > [...]
> > 
> >> Instead, several warnings are now printed instead:
> >>
> >>     WARNING: Can not generate core dump on remote target.
> > 
> > These warnings are newly introduced by the patch.  They are meant to
> > improve diagnostics when someone attempts to run the tests on a "real"
> > remote target.  I wanted to clearly document the fact that this is
> > unsupported (and always was).  Also, by documenting this restriction,
> > maybe someone feels encouraged to lift it ;-)
> 
> Wouldn't an UNTESTED or UNSUPPORTED be better?  It's what
> we tend to do with other cases of unsupported/untested tests.

 I think UNSUPPORTED is the right status; UNTESTED is meant for missing 
tests really; see:

UNTESTED
    A test was not run.  This is a placeholder, used when there is no real 
    test case yet.

vs:

UNSUPPORTED
    There is no support for the tested case. This may mean that a 
    conditional feature of an operating system, or of a compiler, is not 
    implemented.  DejaGnu also uses this message when a testing 
    environment (often a "bare board" target) lacks basic support for 
    compiling or running the test case.  For example, a test for the 
    system subroutine gethostname would never work on a target board 
    running only a boot monitor.

(taken from <http://www.delorie.com/gnu/docs/dejagnu/dejagnu_6.html>).

 FWIW,

  Maciej

Re: [PATCH 1/2] GDB test suite: Add helper for locating core files

[Pedro Alves]

Hi Maciej,

On 10/17/2017 07:19 PM, Maciej W. Rozycki wrote:
> On Tue, 17 Oct 2017, Pedro Alves wrote:

>> Wouldn't an UNTESTED or UNSUPPORTED be better?  It's what
>> we tend to do with other cases of unsupported/untested tests.
> 
>  I think UNSUPPORTED is the right status; UNTESTED is meant for missing 
> tests really; see:
> 
> UNTESTED
>     A test was not run.  This is a placeholder, used when there is no real 
>     test case yet.
> 
> vs:
> 
> UNSUPPORTED
>     There is no support for the tested case. This may mean that a 
>     conditional feature of an operating system, or of a compiler, is not 
>     implemented.  DejaGnu also uses this message when a testing 
>     environment (often a "bare board" target) lacks basic support for 
>     compiling or running the test case.  For example, a test for the 
>     system subroutine gethostname would never work on a target board 
>     running only a boot monitor.
> 

IMHO, this "placeholder" status of UNTESTED is pretty useless, at
least for GDB.  I don't recall ever adding such a placeholder
testcase, and I don't think GDB uses it like that.

I think a more useful distinction would be:

- UNSUPPORTED to indicate that the feature isn't supported by GDB or
  the remote stub.  E.g., in this case we'd use it if the GDB port
  does not support debugging core dumps at all.

- UNTESTED to indicate that the feature is supported by GDB but
  the test wasn't run because it's not possible to run it,
  or we choose to not run it, in the current test environment, e.g.,
  because of a board limitation.  In this case, we skip core tests
  when the host or target boards are remote, even though
  the feature is supported by GDB (even with "target remote" and
  cross debugging, at least on some ports).

We use UNTESTED in many places already meaning something like
the above (instead of placeholder status), though I'm not aware
of rationale written down anywhere.  The above is just my
intuition, and most of these untested/unsupported calls predate
my involvement with GDB.

Thanks,
Pedro Alves

[PATCH 2/2] GDB test suite: Get core files on targets with systemd-coredump

[Andreas Arnez]

So far the test suite skips tests if they need system-generated core files
and the core files can not be found.  In particular this is usually the
case on systems with an active systemd-coredump service.  On such systems,
core files are not written into the local directory, but made accessible
via a command-line utitily "coredumpctl" instead.

This patch enables processing core files on such systems as well.  Note
that there are a few quirks:

* In my tests, after invoking a command that dumps core, it could happen
  that "coredumpctl" did not find the dump immediately afterwards.  After
  waiting a bit, the dump was found and could be accessed.  Thus the patch
  performs a single wait-and-retry in case of failure.

* There does not seem to be a way for a user to remove specific core dumps
  from the journal.  Thus it can happen that "coredumpctl" returns an old
  dump, and the test case continues with that instead of the new one.  It
  might be possible to improve the logic here, by considering the time
  stamps as well.  I leave that for a future patch.

* On the system I've tested it on, the bigcore.exp test case still failed
  because coredumpctl truncated the core file after 4G for some reason.

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (find_core): Add handling for targets with
	systemd-coredump, using the coredumpctl command.
---
 gdb/testsuite/lib/gdb.exp | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 1fd0009..7f80e12 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5877,6 +5877,19 @@ proc find_core {binfile coredir {destcore ""}} {
 	}
     }
 
+    # We may be running on a system with the systemd-coredump service in
+    # effect; try the coredumpctl command.  But due to asynchronous
+    # processing the core dump may not have been processed yet.  Thus wait
+    # a bit and retry after failure.
+    foreach delay {0 1} {
+	sleep $delay
+	if {![catch {exec -ignorestderr coredumpctl -o $destcore dump $binfile}]} {
+	    if [file exists $destcore] {
+		return $destcore
+	    }
+	}
+    }
+
     warning "Can not locate core dump generated by \"[file tail $binfile]\"."
     return ""
 }
-- 
2.9.4

Re: [PATCH 2/2] GDB test suite: Get core files on targets with systemd-coredump

[Pedro Alves]

On 09/18/2017 07:41 PM, Andreas Arnez wrote:
> So far the test suite skips tests if they need system-generated core files
> and the core files can not be found.  In particular this is usually the
> case on systems with an active systemd-coredump service.  On such systems,
> core files are not written into the local directory, but made accessible
> via a command-line utitily "coredumpctl" instead.
> 
> This patch enables processing core files on such systems as well.  Note
> that there are a few quirks:
> 
> * In my tests, after invoking a command that dumps core, it could happen
>   that "coredumpctl" did not find the dump immediately afterwards.  After
>   waiting a bit, the dump was found and could be accessed.  Thus the patch
>   performs a single wait-and-retry in case of failure.
> 
> * There does not seem to be a way for a user to remove specific core dumps
>   from the journal.  Thus it can happen that "coredumpctl" returns an old
>   dump, and the test case continues with that instead of the new one.  It
>   might be possible to improve the logic here, by considering the time
>   stamps as well.  I leave that for a future patch.
> 
> * On the system I've tested it on, the bigcore.exp test case still failed
>   because coredumpctl truncated the core file after 4G for some reason.

I'm a bit unsure about whether this is the right approach,
expecially given the caveats above.  Also, this seems to mean that
running the testsuite on such a system clutters the system log on and on,
maybe even triggers dispatch of notifications to admins, etc.

I wonder whether there's a way to tell systemd-coredump to
let the core dumps be generated on the file system for the current
shell environment?  Like we try to run "ulimit -c unlimited".

Failing that, it may be better to instead make the testsuite skip
the tests gracefully, and display a big and visible warning
if systemd-coredump is detected as active.

I mean, you already have to tweak other things in the system in
order to be able to run the testsuite correctly.  For example,
you have to tweak /proc/sys/kernel/yama/ptrace_scope to make
attach tests work at all, for example.  systemd-coredump kind of
seems like more of the same.

Dunno, as I said, I'm unsure.

Thanks,
Pedro Alves

Re: [PATCH 2/2] GDB test suite: Get core files on targets with systemd-coredump

[Andreas Arnez]

On Tue, Oct 17 2017, Pedro Alves wrote:

> On 09/18/2017 07:41 PM, Andreas Arnez wrote:
>> So far the test suite skips tests if they need system-generated core files
>> and the core files can not be found.  In particular this is usually the
>> case on systems with an active systemd-coredump service.  On such systems,
>> core files are not written into the local directory, but made accessible
>> via a command-line utitily "coredumpctl" instead.
>> 
>> This patch enables processing core files on such systems as well.  Note
>> that there are a few quirks:
>> 
>> * In my tests, after invoking a command that dumps core, it could happen
>>   that "coredumpctl" did not find the dump immediately afterwards.  After
>>   waiting a bit, the dump was found and could be accessed.  Thus the patch
>>   performs a single wait-and-retry in case of failure.
>> 
>> * There does not seem to be a way for a user to remove specific core dumps
>>   from the journal.  Thus it can happen that "coredumpctl" returns an old
>>   dump, and the test case continues with that instead of the new one.  It
>>   might be possible to improve the logic here, by considering the time
>>   stamps as well.  I leave that for a future patch.
>> 
>> * On the system I've tested it on, the bigcore.exp test case still failed
>>   because coredumpctl truncated the core file after 4G for some reason.
>
> I'm a bit unsure about whether this is the right approach,
> expecially given the caveats above.  Also, this seems to mean that
> running the testsuite on such a system clutters the system log on and on,
> maybe even triggers dispatch of notifications to admins, etc.

The caveats above are really bugs/design flaws of systemd-coredump.  If
nothing else, maybe this discussion helps addressing them.  Offering no
way to prevent system log cluttering could be viewed as another flaw,
see also below.

> I wonder whether there's a way to tell systemd-coredump to
> let the core dumps be generated on the file system for the current
> shell environment?  Like we try to run "ulimit -c unlimited".

I agree that there *should* be such a way -- but I haven't found any.
And such a mechanism should probably allow suppressing the log entries
as well.

> Failing that, it may be better to instead make the testsuite skip
> the tests gracefully, and display a big and visible warning
> if systemd-coredump is detected as active.

This might be the right trade-off if we expect test cases to be executed
only on systems that the user has full control over.  But I consider
this restriction too tight and would prefer a "best effort" approach
instead.  Maybe we should emit a warning *and* try our best to execute
the test?

> I mean, you already have to tweak other things in the system in
> order to be able to run the testsuite correctly.  For example,
> you have to tweak /proc/sys/kernel/yama/ptrace_scope to make
> attach tests work at all, for example.  systemd-coredump kind of
> seems like more of the same.

So should we document a sequence of admin commands that makes a system
debug-ready, or in particular ready for the GDB test suite?

But I'm not so sure about this.  IMHO a default mainstream Linux
installation should be suited for development- and debugging purposes
*without* any tweaking.  Also, if there are good reasons for a security
measure, we shouldn't rely on disabling it globally.

With respect to Yama's ptrace scope, the distributions seem to differ.
For instance, Fedora does not activate it by default
(https://fedoraproject.org/wiki/Security_Features_Matrix), while Ubuntu
does (https://wiki.ubuntu.com/Security/Features).  And I wonder whether
this feature couldn't be adjusted to be more debug-friendly either.

--
Andreas

Re: [PATCH 2/2] GDB test suite: Get core files on targets with systemd-coredump

[Pedro Alves]

On 10/17/2017 06:36 PM, Andreas Arnez wrote:
> On Tue, Oct 17 2017, Pedro Alves wrote:
> 
>> On 09/18/2017 07:41 PM, Andreas Arnez wrote:
>>> So far the test suite skips tests if they need system-generated core files
>>> and the core files can not be found.  In particular this is usually the
>>> case on systems with an active systemd-coredump service.  On such systems,
>>> core files are not written into the local directory, but made accessible
>>> via a command-line utitily "coredumpctl" instead.
>>>
>>> This patch enables processing core files on such systems as well.  Note
>>> that there are a few quirks:
>>>
>>> * In my tests, after invoking a command that dumps core, it could happen
>>>   that "coredumpctl" did not find the dump immediately afterwards.  After
>>>   waiting a bit, the dump was found and could be accessed.  Thus the patch
>>>   performs a single wait-and-retry in case of failure.
>>>
>>> * There does not seem to be a way for a user to remove specific core dumps
>>>   from the journal.  Thus it can happen that "coredumpctl" returns an old
>>>   dump, and the test case continues with that instead of the new one.  It
>>>   might be possible to improve the logic here, by considering the time
>>>   stamps as well.  I leave that for a future patch.
>>>
>>> * On the system I've tested it on, the bigcore.exp test case still failed
>>>   because coredumpctl truncated the core file after 4G for some reason.
>>
>> I'm a bit unsure about whether this is the right approach,
>> expecially given the caveats above.  Also, this seems to mean that
>> running the testsuite on such a system clutters the system log on and on,
>> maybe even triggers dispatch of notifications to admins, etc.
> 
> The caveats above are really bugs/design flaws of systemd-coredump.  If
> nothing else, maybe this discussion helps addressing them.  Offering no
> way to prevent system log cluttering could be viewed as another flaw,
> see also below.
> 
>> I wonder whether there's a way to tell systemd-coredump to
>> let the core dumps be generated on the file system for the current
>> shell environment?  Like we try to run "ulimit -c unlimited".
> 
> I agree that there *should* be such a way -- but I haven't found any.
> And such a mechanism should probably allow suppressing the log entries
> as well.
> 
>> Failing that, it may be better to instead make the testsuite skip
>> the tests gracefully, and display a big and visible warning
>> if systemd-coredump is detected as active.
> 
> This might be the right trade-off if we expect test cases to be executed
> only on systems that the user has full control over.  But I consider
> this restriction too tight and would prefer a "best effort" approach
> instead.  Maybe we should emit a warning *and* try our best to execute
> the test?

Not sure, really.  It seems like the "best effort" results in
racy tests, e.g., if "coredumpctl" returns an old dump, or
if coredumpctl decides to rate-limit core dump generation (which
according to the docs, it does).  It very much sounds like that
can lead to hard to diagnose problems and send GDB hackers tilting
at windmills.

> 
>> I mean, you already have to tweak other things in the system in
>> order to be able to run the testsuite correctly.  For example,
>> you have to tweak /proc/sys/kernel/yama/ptrace_scope to make
>> attach tests work at all, for example.  systemd-coredump kind of
>> seems like more of the same.
> 
> So should we document a sequence of admin commands that makes a system
> debug-ready, or in particular ready for the GDB test suite?

IMO, yes.  We already have something like that, but it's mixed with
the instructions for setting up builders:

  https://sourceware.org/gdb/wiki/BuildBot#Fedora-specific_instructions

(Note we already suggest disabling ABRT and tweaking 
kernel.core_pattern.)

It'd be great to move that info to some specific page about setting
up an environment for developing and testing GDB.  Also, some of
the command sequences there could move to scripts under gdb/contrib/,
IMHO.

> 
> But I'm not so sure about this.  IMHO a default mainstream Linux
> installation should be suited for development- and debugging purposes
> *without* any tweaking.  Also, if there are good reasons for a security
> measure, we shouldn't rely on disabling it globally.

I think that battle is lost.  Mainstream Linux installations are 
already very much not suited for development OOTB.  You have to install 
a bunch of development  packages that are not installed by default, 
before you can build  anything, including compiler, etc.  If you can
install packages, then you can also disable a few features that
really are not meant for development environments.  What we're missing
is a simple "one-click button" way to adapt an installation / user
environment for development.

> 
> With respect to Yama's ptrace scope, the distributions seem to differ.
> For instance, Fedora does not activate it by default
> (https://fedoraproject.org/wiki/Security_Features_Matrix), while Ubuntu
> does (https://wiki.ubuntu.com/Security/Features).  And I wonder whether
> this feature couldn't be adjusted to be more debug-friendly either.

The whole point of the feature is to prevent debugging, so I don't
see how, off hand.  Note that on Fedora nowadays, if you install GDB, 
then Yama's ptrace scope is relaxed via a rpm package dependency.
systemd-coredump could also be treated in the same way -- disable
it for users that have GDB or other development tools installed.
I don't actually know whether it's possible to disable systemd-coredump for a
single user, though it seems to me like an obviously desirable feature.

Thanks,
Pedro Alves

Re: [PATCH 2/2] GDB test suite: Get core files on targets with systemd-coredump

[Pedro Alves]

On 10/17/2017 07:08 PM, Pedro Alves wrote:

> Not sure, really.  It seems like the "best effort" results in
> racy tests, e.g., if "coredumpctl" returns an old dump, or
> if coredumpctl decides to rate-limit core dump generation (which
> according to the docs, it does).  It very much sounds like that
> can lead to hard to diagnose problems and send GDB hackers tilting
> at windmills.

I should add that I won't oppose this too strongly _if_ we make
the warning not easily lost in the noise, like e.g., maybe tacked
to the end of all test runs, near the final test results summary?

Thanks,
Pedro Alves

Re: [PATCH 2/2] GDB test suite: Get core files on targets with systemd-coredump

[Pedro Alves]

On 10/17/2017 07:14 PM, Pedro Alves wrote:
> On 10/17/2017 07:08 PM, Pedro Alves wrote:
> 
>> Not sure, really.  It seems like the "best effort" results in
>> racy tests, e.g., if "coredumpctl" returns an old dump, or
>> if coredumpctl decides to rate-limit core dump generation (which
>> according to the docs, it does).  It very much sounds like that
>> can lead to hard to diagnose problems and send GDB hackers tilting
>> at windmills.
> 
> I should add that I won't oppose this too strongly _if_ we make
> the warning not easily lost in the noise, like e.g., maybe tacked
> to the end of all test runs, near the final test results summary?

Another approach would be to not run the core tests by default,
unless you specify some 

make check \
 RUNTESTFLAGS="YES_I_KNOW_ALL_ABOUT_SYSTEMD_COREDUMP_RUN_THE_TESTS_ANYWAY_MKAY=1"
option.

(and then the testsuite would print a warning suggesting that.)

Thanks,
Pedro Alves

Re: [PATCH 2/2] GDB test suite: Get core files on targets with systemd-coredump

[Andreas Arnez]

On Tue, Oct 17 2017, Pedro Alves wrote:

> On 10/17/2017 06:36 PM, Andreas Arnez wrote:

[...]

>> This might be the right trade-off if we expect test cases to be executed
>> only on systems that the user has full control over.  But I consider
>> this restriction too tight and would prefer a "best effort" approach
>> instead.  Maybe we should emit a warning *and* try our best to execute
>> the test?
>
> Not sure, really.  It seems like the "best effort" results in
> racy tests, e.g., if "coredumpctl" returns an old dump, or
> if coredumpctl decides to rate-limit core dump generation (which
> according to the docs, it does).  It very much sounds like that
> can lead to hard to diagnose problems and send GDB hackers tilting
> at windmills.

That might be.  However, the same problems may affect *any* coredumpctl
user, not just the GDB test suite.  And coredumpctl users are *our*
users, after all.  Maybe we should postpone GDB test suite support for
systemd-coredump until these problems are fixed.  But if all "informed
developers" just give up and disable systemd-coredump, I fear that they
will never be addressed.

>
>> 
>>> I mean, you already have to tweak other things in the system in
>>> order to be able to run the testsuite correctly.  For example,
>>> you have to tweak /proc/sys/kernel/yama/ptrace_scope to make
>>> attach tests work at all, for example.  systemd-coredump kind of
>>> seems like more of the same.
>> 
>> So should we document a sequence of admin commands that makes a system
>> debug-ready, or in particular ready for the GDB test suite?
>
> IMO, yes.  We already have something like that, but it's mixed with
> the instructions for setting up builders:
>
>   https://sourceware.org/gdb/wiki/BuildBot#Fedora-specific_instructions
>
> (Note we already suggest disabling ABRT and tweaking 
> kernel.core_pattern.)
>
> It'd be great to move that info to some specific page about setting
> up an environment for developing and testing GDB.  Also, some of
> the command sequences there could move to scripts under gdb/contrib/,
> IMHO.

Yeah, that would be good.

>
>> 
>> But I'm not so sure about this.  IMHO a default mainstream Linux
>> installation should be suited for development- and debugging purposes
>> *without* any tweaking.  Also, if there are good reasons for a security
>> measure, we shouldn't rely on disabling it globally.
>
> I think that battle is lost.

That surely sounds depressing...  I guess I'm late to the battlefield
then ;-)

> Mainstream Linux installations are already very much not suited for
> development OOTB.  You have to install a bunch of development packages
> that are not installed by default, before you can build anything,
> including compiler, etc.  If you can install packages, then you can
> also disable a few features that really are not meant for development
> environments.  What we're missing is a simple "one-click button" way
> to adapt an installation / user environment for development.

Let me just point out that I see a difference between installing
additional packages and disabling security measures.  Admins might be
easily convinced to do the former, but there will probably be more push
back on the latter.  A "one-click button" would not really help with
that.

And all this sounds as if developers were no longer seen as a target
group of a Fedora distribution, say.  On the other hand -- quote --:
"Fedora Workstation is a polished, easy to use operating system for
laptop and desktop computers, with a complete set of tools for
developers and makers of all kinds."

>
>> 
>> With respect to Yama's ptrace scope, the distributions seem to differ.
>> For instance, Fedora does not activate it by default
>> (https://fedoraproject.org/wiki/Security_Features_Matrix), while Ubuntu
>> does (https://wiki.ubuntu.com/Security/Features).  And I wonder whether
>> this feature couldn't be adjusted to be more debug-friendly either.
>
> The whole point of the feature is to prevent debugging, so I don't
> see how, off hand.

Well, I think the goal is to prevent visibility of sensitive data like
passwords and keys through ptrace -- which is a fair point.  But does
this really require disabling ptrace from "non-ancestor" processes
completely?  It just seems to me that the collateral damage to debug
capabilities was accepted too easily in this design.

[...]

Anyway, regarding GDB test suite support for systemd-coredump, I won't
push too hard.  While I have a slight preference towards "best effort",
I understand your concern with possible surprises.  So I'm fine with
dropping this patch.  Patch #1 in this series might still be useful, so
I'll send an updated version of it.

--
Andreas

Re: [PATCH 2/2] GDB test suite: Get core files on targets with systemd-coredump

[Pedro Alves]

On 10/18/2017 04:56 PM, Andreas Arnez wrote:
> On Tue, Oct 17 2017, Pedro Alves wrote:
> 
>> On 10/17/2017 06:36 PM, Andreas Arnez wrote:
> 
> [...]
> 
>>> This might be the right trade-off if we expect test cases to be executed
>>> only on systems that the user has full control over.  But I consider
>>> this restriction too tight and would prefer a "best effort" approach
>>> instead.  Maybe we should emit a warning *and* try our best to execute
>>> the test?
>>
>> Not sure, really.  It seems like the "best effort" results in
>> racy tests, e.g., if "coredumpctl" returns an old dump, or
>> if coredumpctl decides to rate-limit core dump generation (which
>> according to the docs, it does).  It very much sounds like that
>> can lead to hard to diagnose problems and send GDB hackers tilting
>> at windmills.
> 
> That might be.  However, the same problems may affect *any* coredumpctl
> user, not just the GDB test suite.  And coredumpctl users are *our*
> users, after all.  

Sure, but we're not interested in testing coredumpctl's ability to copy 
a core dump out of the log before that file is handed over to gdb?
We're testing GDB's ability to load a core dump file that the kernel produced,
doesn't matter to us really how many hoops the file went through before
it was fed to gdb.  I mean, we don't go and run filesystem-level stress-tests
because we copy files around in the filesystem -- we just assume the
filesystem is bug free, and let filesystem folks worry about such testing.

If coredumpctl Just Worked flawlessly for our testsuite's use
case (which is not like a regular user's use case), then I'd be all
for this.  My problem is really that it doesn't really work
all that well, and by not informing GDB developers that there's
(currently...) a better non-racy way to run
make-sure-you-can-load-kernel-core-dumps tests, then we're
not doing our follow GDB developers a real service.

> Maybe we should postpone GDB test suite support for
> systemd-coredump until these problems are fixed.  But if all "informed
> developers" just give up and disable systemd-coredump, I fear that they
> will never be addressed.

Judging from our rate of fixes for our own racy-tests, some of
which are caused by other-components issues (frequently glibc/kernel),
I don't think that exposing us to more sources of races will help with
that either.

>>> But I'm not so sure about this.  IMHO a default mainstream Linux
>>> installation should be suited for development- and debugging purposes
>>> *without* any tweaking.  Also, if there are good reasons for a security
>>> measure, we shouldn't rely on disabling it globally.
>>
>> I think that battle is lost.
> 
> That surely sounds depressing...  I guess I'm late to the battlefield
> then ;-)

My remark may have sounded fatalistic, but in reality nothing is ever
settled forever in software land.  All it really takes is someone
motivated to go and improve things, work with the relevant
communities, etc.  It's just that it's an uphill battle, where
there seems to be a lot more interest/investment in the community to
make things secure by default even if it makes it a little more
inconvenient for the system component developer. Balancing both a bit
better might be possible, but it requires commitment, involvement and
a vigilant eye over the whole stack.  History is showing that this
just won't come "for free" for us, unfortunately.

Let's also not forget that the needs of a GDB developer are not the same
as the needs of a developer that uses GDB to debug its own programs for
a given system.  GDB _is_ part of the system, and regression testing it involves
stressing other system components in ways that a normal developer
normally won't.  For example, running the testsuite could in theory well
end up producing hundreds of core dumps for a variation of tests in a few
seconds.   A regular developer doesn't need that.  Another example, ideally, we'd have
tests that make sure that GDB behaves correctly when Yama rejects an attach, when
it rejects spawning a child process, _in addition_ to testing actual run and
attach succeeding.  So ideally we'd have a way to control the Yama scope at run
time without affecting the whole system.  But that's something that you can't
do unless you're root, and running the testsuite with root permissions is
not something that I'd recommend (though maybe we could have an
optional "testsuite/gdb.root/" directory with such tests).

> 
>> Mainstream Linux installations are already very much not suited for
>> development OOTB.  You have to install a bunch of development packages
>> that are not installed by default, before you can build anything,
>> including compiler, etc.  If you can install packages, then you can
>> also disable a few features that really are not meant for development
>> environments.  What we're missing is a simple "one-click button" way
>> to adapt an installation / user environment for development.
> 
> Let me just point out that I see a difference between installing
> additional packages and disabling security measures.  Admins might be
> easily convinced to do the former, but there will probably be more push
> back on the latter.  A "one-click button" would not really help with
> that.
> 
> And all this sounds as if developers were no longer seen as a target
> group of a Fedora distribution, say.  On the other hand -- quote --:
> "Fedora Workstation is a polished, easy to use operating system for
> laptop and desktop computers, with a complete set of tools for
> developers and makers of all kinds."
> 
>>
>>>
>>> With respect to Yama's ptrace scope, the distributions seem to differ.
>>> For instance, Fedora does not activate it by default
>>> (https://fedoraproject.org/wiki/Security_Features_Matrix), while Ubuntu
>>> does (https://wiki.ubuntu.com/Security/Features).  And I wonder whether
>>> this feature couldn't be adjusted to be more debug-friendly either.
>>
>> The whole point of the feature is to prevent debugging, so I don't
>> see how, off hand.
> 
> Well, I think the goal is to prevent visibility of sensitive data like
> passwords and keys through ptrace -- which is a fair point.  But does
> this really require disabling ptrace from "non-ancestor" processes
> completely?  It just seems to me that the collateral damage to debug
> capabilities was accepted too easily in this design.
> 
> [...]
> 
> Anyway, regarding GDB test suite support for systemd-coredump, I won't
> push too hard.  While I have a slight preference towards "best effort",
> I understand your concern with possible surprises.  So I'm fine with
> dropping this patch.  

(Again, I don't mind this going in if we make it visibly warn/optional.)

> Patch #1 in this series might still be useful, so
> I'll send an updated version of it.

Yes, please.  Thanks!

-- 
Pedro Alves

Re: [PATCH 2/2] GDB test suite: Get core files on targets with systemd-coredump

[Andreas Arnez]

On Thu, Oct 19 2017, Pedro Alves wrote:

[...]

> If coredumpctl Just Worked flawlessly for our testsuite's use
> case (which is not like a regular user's use case), then I'd be all
> for this.  My problem is really that it doesn't really work
> all that well, [...]

FWIW, to help the GDB test suite (and others) in the future, I've opened
a systemd feature request to support switching to "classical" core dump
behavior for certain programs:

  https://github.com/systemd/systemd/issues/7166

Anyone interested, feel free to subscribe.

--
Andreas

Re: [PATCH 2/2] GDB test suite: Get core files on targets with systemd-coredump

[Pedro Alves]

On 10/23/2017 02:41 PM, Andreas Arnez wrote:
> On Thu, Oct 19 2017, Pedro Alves wrote:
> 
> [...]
> 
>> If coredumpctl Just Worked flawlessly for our testsuite's use
>> case (which is not like a regular user's use case), then I'd be all
>> for this.  My problem is really that it doesn't really work
>> all that well, [...]
> 
> FWIW, to help the GDB test suite (and others) in the future, I've opened
> a systemd feature request to support switching to "classical" core dump
> behavior for certain programs:
> 
>   https://github.com/systemd/systemd/issues/7166
> 
> Anyone interested, feel free to subscribe.

Excellent, thanks much!  I've subscribed.

Pedro Alves