Re: [PATCH v6] Add a test for the gcore script

[Lancelot SIX <lancelot.six@amd.com>]

On Sun, Jun 02, 2024 at 05:50:59PM +0200, Alexandra Hájková wrote:
> From: Alexandra Hájková <ahajkova@redhat.com>
> 
> It also tests the gcore script being run without its accessible
> terminal.
> 
> This test was written by Jan Kratochvil a long time ago. I modernized
> the test making it use various procs from lib/gdb.exp, reorganizing it
> and added some comments.
> 
> Modify the gcore script to make it possible to pass the --data-directory to
> it. This prevents a lot of these warnings:
> 
> Python Exception <class 'AttributeError'>: module 'gdb' has no attribute
> '_handle_missing_debuginfo'
> 
> Tested by using make check-all-boards.
> 
> Co-Authored-By: Jan Kratochvil <jan.kratochvil@redhat.com>

Hi Alexandra,

If I run this testcase with `sysctl kernel.yama.ptrace_scope=1`, I have:

    Running ..../gdb/testsuite/gdb.base/gcorebg.exp ...
    FAIL: gdb.base/gcorebg.exp: detached = standard: Spawned gcore finished
    FAIL: gdb.base/gcorebg.exp: detached = standard: Core file generated by gcore
    FAIL: gdb.base/gcorebg.exp: detached = detached: Spawned gcore finished
    FAIL: gdb.base/gcorebg.exp: detached = detached: Core file generated by gcore

The log is:

    PASS: gdb.base/gcorebg.exp: detached = standard: Spawned gcore
    ptrace: Operation not permitted.
    You can't do that without a process to debug.
    The program is not being run.
    gcore: failed to create ..../gdb/testsuite/outputs/gdb.base/gcorebg/gcorebg.core.275084
    FAIL: gdb.base/gcorebg.exp: detached = standard: Spawned gcore finished
    FAIL: gdb.base/gcorebg.exp: detached = standard: Core file generated by gcore

I think you can avoid this by using

    require can_spawn_for_attach

I do have minor remarks inlined in the patch as well.

> ---
> v6: 
> - fixed typos
> - used Jan Kratochvil's old invalid email for the record
> - improved the "usage" message
> - fixed has_gcore_script proc
> 
>  gdb/doc/gdb.texinfo                |  2 +-
>  gdb/gcore.in                       | 12 +++-
>  gdb/testsuite/gdb.base/gcorebg.c   | 86 +++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.base/gcorebg.exp | 88 ++++++++++++++++++++++++++++++
>  gdb/testsuite/lib/gdb.exp          | 18 ++++++
>  5 files changed, 202 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/gcorebg.c
>  create mode 100644 gdb/testsuite/gdb.base/gcorebg.exp
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 750f368f980..67edfdaf800 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -50984,7 +50984,7 @@ Richard M. Stallman and Roland H. Pesch, July 1991.
>  
>  @format
>  @c man begin SYNOPSIS gcore
> -gcore [-a] [-o @var{prefix}] @var{pid1} [@var{pid2}...@var{pidN}]
> +gcore [-a] [-o @var{prefix}] [-d @var{path}] @var{pid1} [@var{pid2}...@var{pidN}]
>  @c man end
>  @end format
>  
> diff --git a/gdb/gcore.in b/gdb/gcore.in
> index 982c854eb70..0c40e5a54cd 100644
> --- a/gdb/gcore.in
> +++ b/gdb/gcore.in
> @@ -27,7 +27,9 @@ prefix=core
>  # to ensure gdb dumps all mappings (OS dependent).
>  dump_all_cmds=()
>  
> -while getopts :ao: opt; do
> +data_directory_opt=()
> +
> +while getopts :ao:d: opt; do
>      case "$opt" in
>          a)
>              case "$OSTYPE" in
> @@ -40,8 +42,11 @@ while getopts :ao: opt; do
>          o)
>              prefix=$OPTARG
>              ;;
> +        d)
> +            data_directory_opt=("--data-directory" "$OPTARG")
> +            ;;
>          *)
> -            echo "usage:  @GCORE_TRANSFORM_NAME@ [-a] [-o prefix] pid1 [pid2...pidN]"
> +            echo "usage:  @GCORE_TRANSFORM_NAME@ [-a] [-o prefix] [-d data-directory] pid1 [pid2...pidN]"
>              exit 2
>              ;;
>      esac
> @@ -51,7 +56,7 @@ shift $((OPTIND-1))
>  
>  if [ "$#" -eq "0" ]
>  then
> -    echo "usage:  @GCORE_TRANSFORM_NAME@ [-a] [-o prefix] pid1 [pid2...pidN]"
> +    echo "usage:  @GCORE_TRANSFORM_NAME@ [-a] [-o prefix] [-d data-directory] pid1 [pid2...pidN]"
>      exit 2
>  fi
>  
> @@ -98,6 +103,7 @@ do
>  	# `</dev/null' to avoid touching interactive terminal if it is
>  	# available but not accessible as GDB would get stopped on SIGTTIN.
>  	"$binary_path/@GDB_TRANSFORM_NAME@" </dev/null \
> +            "${data_directory_opt[@]}" \
>  	    --nx --batch --readnever -iex 'set debuginfod enabled off' \
>  	    -ex "set pagination off" -ex "set height 0" -ex "set width 0" \
>  	    "${dump_all_cmds[@]}" \
> diff --git a/gdb/testsuite/gdb.base/gcorebg.c b/gdb/testsuite/gdb.base/gcorebg.c
> new file mode 100644
> index 00000000000..1e9ddbd8649
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcorebg.c
> @@ -0,0 +1,86 @@
> +/* Copyright 2007-2024 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <signal.h>
> +#include <string.h>
> +#include <assert.h>
> +
> +/* Expects 4 arguments:
> +
> +   1. Either 'standard' or 'detached', where 'standard' tests
> +   a general gcore script spawn with its controlling terminal available
> +   and 'detached' tests gcore script spawn without its controlling
> +   terminal available.
> +   2. Path to the gcore script.
> +   3. Path to the data-directory to pass to the gcore script.
> +   4. The core file output name.  */
> +
> +int
> +main (int argc, char **argv)
> +{
> +  pid_t pid = 0;
> +  pid_t ppid;
> +  char buf[1024*2 + 500];
> +  int gotint, res;
> +  int fd[2];
> +
> +  assert (argc == 5);
> +
> +  if (pipe(fd) == -1)
> +    {
> +      perror ("pipe err\n");
> +      exit (1);
> +    }
> +  pid = fork ();
> +
> +  switch (pid)
> +    {
> +    case 0:
> +      close (fd[0]);
> +      if (strcmp (argv[1], "detached") == 0)
> +	setpgrp ();
> +      ppid = getppid ();
> +      gotint = snprintf (buf, sizeof (buf), "%s -d %s -o %s %d",
> +			 argv[2], argv[3], argv[4], (int) ppid);
> +      assert (gotint < sizeof (buf));
> +      res = system (buf);
> +      assert (res != -1);
> +      close(fd[1]);
> +      break;
> +
> +    case -1:
> +      close (fd[0]);
> +      close (fd[1]);
> +      perror ("fork err\n");
> +      exit (1);
> +      break;
> +
> +    default:
> +      close (fd[1]);
> +      /* Wait here until the child is done with gcore-ing us.  When the
> +	 child has finished it will close its end of the pipe and this read
> +	 call will return.  */
> +      res = read (fd[0], buf, 1);

I guess in theory read could return -1 and errno be EINTR.  I was
wondering if there is a reason to prefer creating a pipe rather than
using waitpid to wait for the child finish?

> +      assert (res == 0);
> +      close (fd[0]);
> +      break;
> +    }
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/gcorebg.exp b/gdb/testsuite/gdb.base/gcorebg.exp
> new file mode 100644
> index 00000000000..5bcb547b9e1
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcorebg.exp
> @@ -0,0 +1,88 @@
> +# Copyright 2007-2024 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +# This is a test for the gcore script (not the gcore command from
> +# inside GDB).  It also tests the gcore script being run without its
> +# accessible terminal.
> +
> +standard_testfile
> +require {!is_remote host}
> +require {!is_remote target}
> +require has_gcore_script

As mentioned above, I think you also need:

    require can_spawn_for_attach

> +
> +set corefile [standard_output_file ${testfile}.core]
> +
> +if {[build_executable "failed to build" $testfile ${srcfile}] == -1 } {
> +     return -1
> +}
> +
> +# Cleanup.
> +
> +proc core_clean {} {
> +    global corefile
> +
> +    foreach file [glob -nocomplain [join [list $corefile *] ""]] {
> +	verbose "Delete file $file" 1
> +	remote_file target delete $file
> +    }
> +}
> +core_clean
> +
> +# Generate the core file.
> +proc test_body { detached } {
> +    global binfile
> +    global GCORE
> +    global corefile
> +    global GDB_DATA_DIRECTORY
> +
> +    with_test_prefix "detached = $detached" {

See below, I gues you could use foreach_with_prefix outside of this
function which would be more "usual".

> +	# We can't use gdb_test_multiple here because GDB is not started.
> +	set res [remote_spawn target "$binfile $detached $GCORE $GDB_DATA_DIRECTORY $corefile"]
> +	if { $res < 0 || $res == "" } {
> +	    fail "Spawning gcore"
> +	    return 1
> +	}
> +	pass "Spawned gcore"
> +
> +	set saw_corefile_created false
> +	set testname "Spawned gcore finished"
> +	remote_expect target 20 {
> +	    timeout {
> +		fail "$testname (timeout)"
> +		remote_exec target "kill -9 -[exp_pid -i $res]"
> +		return
> +	    }
> +	    -re "Saved corefile \[^\r\n\]+\r\n" {
> +		set saw_corefile_created true
> +		exp_continue
> +	    }
> +	    eof {
> +		gdb_assert { $saw_corefile_created } $testname
> +	    }
> +	}
> +
> +	gdb_assert {1 == [llength [glob -nocomplain [join [list $corefile *] ""]]]} "Core file generated by gcore"
> +	core_clean
> +    }
> +}
> +
> +# First a general gcore script spawn with its controlling terminal available.
> +
> +test_body standard
> +
> +# And now gcore script spawn without its controlling terminal available.
> +# It is spawned through `gcorebg.c' using setpgrp ().
> +
> +test_body detached

Could be

foreach_with_prefix detached { detached standard } {
    test_body $detached
}

> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index cdc3721a1cd..25a9cec5c34 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -158,6 +158,24 @@ load_lib gdb-utils.exp
>  load_lib memory.exp
>  load_lib check-test-names.exp
>  
> +# The path to the GCORE script to test.
> +global GCORE
> +if {![info exists GCORE]} {
> +    set GCORE [findfile $base_dir/../../gdb/gcore]
> +} else {
> +    set GCORE ""
> +}
> +verbose "using GCORE = $GCORE" 2
> +
> +proc has_gcore_script {} {
> +    global GCORE
> +    if {$GCORE == ""} {
> +	return 0
> +    } else {
> +	return 1
> +    }
> +}
> +
>  # The path to the GDB binary to test.
>  global GDB
>  
> 
> -- 
> 2.45.0
> 

Best,
Lancelot.