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

[Pedro Alves <pedro@palves.net>]

Hi!

Nit: remove period at end of $subject.

On 2024-02-09 11:56, Alexandra Hájková wrote:
> It also tests the gcore script being run without its
> accessible terminal.
> 
> This file was written by Jan Kratochvil a long time ago. I modernized
> the test making it use various procs from lib/gdb.exp and added some
> comments.

Please use Co-Authored-By:.

> 
> Tested by using make check-all-boards.
> ---
>  gdb/testsuite/gdb.base/gcorebg.c   | 65 ++++++++++++++++++++
>  gdb/testsuite/gdb.base/gcorebg.exp | 95 ++++++++++++++++++++++++++++++
>  gdb/testsuite/lib/gdb.exp          |  7 +++
>  3 files changed, 167 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/gcorebg.c
>  create mode 100644 gdb/testsuite/gdb.base/gcorebg.exp
> 
> diff --git a/gdb/testsuite/gdb.base/gcorebg.c b/gdb/testsuite/gdb.base/gcorebg.c
> new file mode 100644
> index 00000000000..5749e4fcc8e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcorebg.c
> @@ -0,0 +1,65 @@
> +/* Copyright 2024 Free Software Foundation, Inc.

As you say, the testcase isn't new, and has been included/distributed along with
Fedora/RHEL gdb sources for a long time, presumably.  The original copyright year
range should be preserved.

> +
> +   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>
> +
> +int
> +main (int argc, char **argv)
> +{
> +  pid_t pid = 0;
> +  pid_t ppid;
> +  char buf[1024*2 + 500];
> +  int gotint;
> +
> +  if (argc != 4)
> +    {
> +      fprintf (stderr, "Syntax: %s {standard|detached} <gcore command> <core output file>\n",
> +	       argv[0]);
> +      exit (1);
> +    }
> +
> +  pid = fork ();
> +
> +  switch (pid)
> +    {
> +      case 0:
> +	if (strcmp (argv[1], "detached") == 0)
> +	  setpgrp ();
> +	ppid = getppid ();
> +	gotint = snprintf (buf, sizeof (buf), "sh %s -o %s %d", argv[2], argv[3], (int) ppid);
> +	assert (gotint < sizeof (buf));
> +	system (buf);

Should check/assert result.

> +	fprintf (stderr, "Killing parent PID %d\n", ppid);
> +	kill (ppid, SIGTERM);

Why?  (Please add comments describing logic/flow.)

> +	break;
> +
> +      case -1:
> +	perror ("fork err\n");
> +	exit (1);
> +	break;
> +
> +      default:
> +	fprintf (stderr,"Sleeping as PID %d\n", getpid ());

Missing space after first comma.

> +	sleep (60);

So IIUC, the parent is waiting here for 60 seconds until the child kills it with SIGTERM.
During those 60 seconds, the child spawned a grandchild (via system) to use gcore on
the (grand)parent.  Please add comments explaining the flow.  Like e.g., here, you could
add, before the sleep:

  /* Wait here until the child is done with gcore-ing us.  If things go right,
     the child kills us with SIGTERM before this sleep returns.  */

You could avoid having this sleep by instead creating a pipe before fork, and then
the parent side would block reading from the pipe.  The child would the close the pipe
once it is done with gcore, which would unblock the parent.

> +    }
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/gcorebg.exp b/gdb/testsuite/gdb.base/gcorebg.exp
> new file mode 100644
> index 00000000000..a9cbee4b580
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcorebg.exp
> @@ -0,0 +1,95 @@
> +# Copyright 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 form

form -> from

> +# inside GDB). It also tests the gcore script being run without its

Double space after period.

> +# accessible terminal.
> +
> +standard_testfile
> +require {!is_remote host}
> +require {!is_remote target}
> +
> +set corefile [standard_output_file ${testfile}.test]

How about naming this ".core" instead of ".test", to make it more obvious what it is
if the file is left behind on the file system?

> +
> +if {[build_executable $testfile.exp $testfile ${srcfile}] == -1 } {

Pass "failed to build" or some such as first argument instead of "$testfile.exp", and ...

> +     untested gcorebg.exp

... remove this line.  build_executable already calls "untested".

> +     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
> +remote_file target delete "./gdb"

What is this line doing?  What is this "gdb" file?

> +
> +# Generate the core file.
> +proc test_body { detached } {


Instead of including "$detached" in every test message below, the modern thing is to use test
prefixes, with with_test_prefix or proc_with_prefix.

> +    global binfile
> +    global GCORE
> +    global corefile
> +
> +    # We can't use gdb_test_multiple here because GDB is not started.
> +    set res [remote_spawn target "$binfile $detached $GCORE $corefile"]
> +    if { $res < 0 || $res == "" } {
> +	fail "Spawning $detached gcore"
> +	return 1
> +    }
> +    pass "Spawning $detached gcore"

Add an empty line here, please.  Above the test is "Spawning...", below it is "Spawned",
and a line break would make that distinction clearer.

> +    remote_expect target 20 {
> +	timeout {
> +	    fail "Spawned $detached gcore finished (timeout)"
> +	    remote_exec target "kill -9 -[exp_pid -i $res]"
> +	    return 1
> +	}
> +	"Saved corefile .*\r\nKilling parent PID " {
> +	    pass "Spawned $detached gcore finished"
> +	    remote_wait target 20

What is this remote_wait for?

> +	}
> +    }
> +
> +    if {1 == [llength [glob -nocomplain [join [list $corefile *] ""]]]} {
> +	pass "Core file generated by $detached gcore"
> +    } else {
> +	fail "Core file generated by $detached gcore"
> +    }

       ^^^^ could be simplified with a gdb_assert.

> +    core_clean
> +}
> +
> +global env
> +
> +save_vars { $env(PATH) } {
> +    set env(PATH) [join [list . $env(PATH)] ":"]

Why?  (Add comment.)

> +    verbose "PATH = $env(PATH)" 2
> +
> +    # 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
> +}
> +
> +# Cleanup.
> +
> +remote_file target delete "./gdb"

Same question as earlier.

> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 9a906f0f349..255b4ae7c83 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -146,6 +146,13 @@ 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/../../gcore]
> +}
> +verbose "using GCORE = $GCORE" 2
> +
>  # The path to the GDB binary to test.
>  global GDB
>