From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id B870B3858C31 for ; Mon, 29 Apr 2024 12:09:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B870B3858C31 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org B870B3858C31 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714392575; cv=none; b=AM9J1j75mGQB03Uw40/FSKGMrDP78Qfxtqairl9G54SUbCyWnAR+jDxCRkqQQW5cOrnmXHDxemoSuOonVwi89eqgnVzZxLP26XR/KL7/vzgJ7ZIjq4qHJ8ByBy/gBClQPj04RA8JoVNrSGD0VBPXYAWjd2MJF3OL557GXK5xfjE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714392575; c=relaxed/simple; bh=Tm7DJnL1J8qCoLzbHn6uaBybccTno3e61zaxhIRdoPw=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=FgSPNhLVuW444N0Ajf52g/7PNFC2g4Ee+LcAiXCW1WpzcWaTg+qMy0fqI7VTSn0owwvKsAKWdiYmkM/FK7quYCiLV+76p4NX5woAKY1bFXMis34813U/P0FOXoZdDERhlB4FCzCDcKJ6YtVTRscdwK+xgyaKR6tbSsjRQrfwNVE= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1714392571; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=irC5Je9Q5WNCA2GiJt8sNytBPZGnjETy0f+ZrG1w/t4=; b=Xti6MboWVSzNn+2Ps5uimWw8XDL/Zf2FJF4mD1BLlNYHPgz3UtjHRcJOUkpAEm7jBCZDoO KW1yonKjoUnNRjSuAiPJi9HoXMMkcX8glSSOKRaRqI6W/YQACXvG7Ajt5cGCaRuoC8/z8B HEXoEjPbBbuhIKKpe9P+sJ2BvwcqeQ4= Received: from mail-lj1-f199.google.com (mail-lj1-f199.google.com [209.85.208.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-205-i2fOIOX0OIG4G7xyc3b4hw-1; Mon, 29 Apr 2024 08:09:29 -0400 X-MC-Unique: i2fOIOX0OIG4G7xyc3b4hw-1 Received: by mail-lj1-f199.google.com with SMTP id 38308e7fff4ca-2dfb1329c9cso14420201fa.2 for ; Mon, 29 Apr 2024 05:09:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714392567; x=1714997367; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=yxhysDlgniEw1UqVvAbpBHd645k7uSQRAew2+WgH5kM=; b=SvVXyMMIezEZggky/8Uzi/bxKc+fzCJwAVX0SJiXUjNRZqJAwOqJrMgP59L8u4aDyX CrqwLweM8NM49vagL9Xtu5kvAJmNU9y5Msfq6psqgJqL2ypOkxF7posIEZzDzdnZSvRk gfpG+TXL+XyW67KbJJ/pIWl6XMGm78jsl+CpEZI2XYJ4bHYAUZzhM0L9EU0OucQJF8YW eYKM5Vti/ujMs9ijUilhEx8dZWvlbWg2EvN0MsHyHoxs8gq2BXJBb6JGYjTDnBWJIC8L e5dRR44zxIXdd791HwExZRz1E0R1qZkUIT6ZdrxolXu/oxkXlQy9v/AgceDcUx9HirGa svMg== X-Forwarded-Encrypted: i=1; AJvYcCWPvDO534dWh6QglfvlRmFjZVXxfI9ClcJ3o0VWTWSHSPSAJzZNWL0ajLUDs/xHlGKcYyHX/xJaZh6c9WJmzvAOtW33J5OVu3MYEg== X-Gm-Message-State: AOJu0YwH9P/TrXw/C6yKaLVQMCyRm4Mtl1lIYkC+U1Dvp/7hi7x/9sQX rExJc/j4rdOIu5JNIO2iACtNDenwBtfXyKSAJvWR/o6RYLT9MYCg3i3aMqWIcJuWwA67qZuJXDh TNpSyj3MsqlIr77OkbgbeEVRkDa+fuF5IO0WBQ/v/DaMXNSAqLIJNodS3MRA= X-Received: by 2002:a05:651c:510:b0:2e0:22e4:a589 with SMTP id o16-20020a05651c051000b002e022e4a589mr3058993ljp.14.1714392567192; Mon, 29 Apr 2024 05:09:27 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGckFMHhqSyUYwkhkg9ISj6tpiDQV1plrjOGiJCWLdZMOccsEfHmVEEHbIwtz0kNiQUD0djTg== X-Received: by 2002:a05:651c:510:b0:2e0:22e4:a589 with SMTP id o16-20020a05651c051000b002e022e4a589mr3058963ljp.14.1714392566530; Mon, 29 Apr 2024 05:09:26 -0700 (PDT) Received: from localhost (185.223.159.143.dyn.plus.net. [143.159.223.185]) by smtp.gmail.com with ESMTPSA id k21-20020adfb355000000b0034c6b368aecsm8068266wrd.26.2024.04.29.05.09.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 29 Apr 2024 05:09:26 -0700 (PDT) From: Andrew Burgess To: Alexandra =?utf-8?B?SMOhamtvdsOh?= , gdb-patches@sourceware.org Cc: ahajkova@redhat.com Subject: Re: [PATCH v3] Add a test for the gcore script In-Reply-To: <20240423104620.1621814-1-ahajkova@khirnov.net> References: <20240423104620.1621814-1-ahajkova@khirnov.net> Date: Mon, 29 Apr 2024 13:09:25 +0100 Message-ID: <87frv4pdze.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Alexandra H=C3=A1jkov=C3=A1 writes: > From: Alexandra H=C3=A1jkov=C3=A1 > > 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, reoirganising 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 : module 'gdb' has no attribute > '_handle_missing_debuginfo' > > Tested by using make check-all-boards. > > Co-Authored-By: Jan Kratochvil > --- > v3: - do not call sh explicitly to avoid "Syntax error: "(" unexpected" > > gdb/gcore.in | 9 ++- > gdb/testsuite/gdb.base/gcorebg.c | 85 +++++++++++++++++++++++++++++ > gdb/testsuite/gdb.base/gcorebg.exp | 88 ++++++++++++++++++++++++++++++ > gdb/testsuite/lib/gdb.exp | 17 ++++++ > 4 files changed, 198 insertions(+), 1 deletion(-) > create mode 100644 gdb/testsuite/gdb.base/gcorebg.c > create mode 100644 gdb/testsuite/gdb.base/gcorebg.exp > > diff --git a/gdb/gcore.in b/gdb/gcore.in > index 982c854eb70..291b807bed9 100644 > --- a/gdb/gcore.in > +++ b/gdb/gcore.in > @@ -27,7 +27,9 @@ prefix=3Dcore > # to ensure gdb dumps all mappings (OS dependent). > dump_all_cmds=3D() > =20 > -while getopts :ao: opt; do > +DATA_DIRECTORY_OPT=3D"" All other variables are lower case, so think this should be too. > + > +while getopts :ao:d: opt; do > case "$opt" in > a) > case "$OSTYPE" in > @@ -40,8 +42,12 @@ while getopts :ao: opt; do > o) > prefix=3D$OPTARG > ;; > + d) > + DATA_DIRECTORY_OPT=3D"--data-directory $OPTARG" I've been thinking about filenames containing spaces recently. I wonder if we should create data_directory_opt as an array, like we do with dump_all_cmds? So initialise it as: data_directory_opt=3D() And here we'd do: data_directory_opt=3D("--data-directory", "$OPTARG") Then, when we use it we can do: "${data_directory_opt[@]}" which will enclose each array element in quotes. In this way, I think that if OPTARG contains a space, we should still correctly forward the string to GDB. > + ;; > *) > echo "usage: @GCORE_TRANSFORM_NAME@ [-a] [-o prefix] pid1 [= pid2...pidN]" > + echo "DATA_DIRECTORY_OPT: $DATA_DIRECTORY_OPT" I think this line was likely debug? But that points out that you should have updated the usage line. You should also update the documentation I think, there's certainly some docs for the gcore script, so they'll need updating. > exit 2 > ;; > esac > @@ -98,6 +104,7 @@ do > =09# ` =09# available but not accessible as GDB would get stopped on SIGTTIN. > =09"$binary_path/@GDB_TRANSFORM_NAME@" + $DATA_DIRECTORY_OPT \ This is where I'd propose using: "${data_directory_opt[@]}" to handle paths containing whitespace. > =09 --nx --batch --readnever -iex 'set debuginfod enabled off' \ > =09 -ex "set pagination off" -ex "set height 0" -ex "set width 0" \ > =09 "${dump_all_cmds[@]}" \ > diff --git a/gdb/testsuite/gdb.base/gcorebg.c b/gdb/testsuite/gdb.base/gc= orebg.c > new file mode 100644 > index 00000000000..a56ca743a98 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/gcorebg.c > @@ -0,0 +1,85 @@ > +/* 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 .= */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* 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 > + > +*/ Full stop at the end of each list entry, like you have for #1, and the final '*/' should be on the line for entry number 4. > + > +int > +main (int argc, char **argv) > +{ > + pid_t pid =3D 0; > + pid_t ppid; > + char buf[1024*2 + 500]; > + int gotint, res; > + int fd[2]; > + > + assert (argc =3D=3D 5); > + > + if (pipe(fd) =3D=3D -1) > + { > + perror ("pipe err\n"); > + exit (1); > + } > + pid =3D fork (); > + > + switch (pid) > + { > + case 0: > + close (fd[0]); > + if (strcmp (argv[1], "detached") =3D=3D 0) > +=09setpgrp (); > + ppid =3D getppid (); > + gotint =3D snprintf (buf, sizeof (buf), "%s -d %s -o %s %d", > +=09=09=09 argv[2], argv[3], argv[4], (int) ppid); > + assert (gotint < sizeof (buf)); > + res =3D system (buf); > + assert (res !=3D -1); > + close(fd[1]); > + break; > + > + case -1: > + close (fd[0]); > + close (fd[1]); > + perror ("fork err\n"); > + exit (1); > + break; > + > + default: > + close (fd[1]); In Pedro's review of V1 he suggested adding some comments of the flow of control. The implementation has changed since V1, but I do think it wouldn't hurt to add a comment here: /* 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 =3D read (fd[0], buf, 1); > + assert (res =3D=3D 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..6269988133c > --- /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 . > +# > +# This is a test for the gcore script (not the gcore command form > +# 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 > + > +set corefile [standard_output_file ${testfile}.core] > + > +if {[build_executable "failed to build" $testfile ${srcfile}] =3D=3D -1 = } { > + return -1 > +} > + > +# Cleanup. > + > +proc core_clean {} { > + global corefile > + > + foreach file [glob -nocomplain [join [list $corefile *] ""]] { > +=09verbose "Delete file $file" 1 > +=09remote_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 =3D $detached" { > +=09# We can't use gdb_test_multiple here because GDB is not started. > +=09set res [remote_spawn target "$binfile $detached $GCORE $GDB_DATA_DIR= ECTORY $corefile"] > +=09if { $res < 0 || $res =3D=3D "" } { > +=09 fail "Spawning gcore" > +=09 return 1 > +=09} > +=09pass "Spawned gcore" > + > +=09set saw_corefile_created false > +=09set testname "Spawned gcore finished" > +=09remote_expect target 20 { > +=09 timeout { > +=09=09fail "$testname (timeout)" > +=09=09remote_exec target "kill -9 -[exp_pid -i $res]" > +=09=09return > +=09 } > +=09 -re "Saved corefile \[^\r\n\]+\r\n" { > +=09=09set saw_corefile_created true > +=09=09exp_continue > +=09 } > +=09 eof { > +=09=09gdb_assert { $saw_corefile_created } $testname > +=09 } > +=09} > + > +=09gdb_assert {1 =3D=3D [llength [glob -nocomplain [join [list $corefile= *] ""]]]} "Core file generated by gcore" > +=09core_clean > + } > +} > + > +# First a general gcore script spawn with its controlling terminal avail= able. > + > +test_body standard > + > +# And now gcore script spawn without its controlling terminal available. > +# It is spawned through `gcorebg.c' using setpgrp (). > + > +test_body detached > diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp > index cbd37fd3094..6b1c646bd23 100644 > --- a/gdb/testsuite/lib/gdb.exp > +++ b/gdb/testsuite/lib/gdb.exp > @@ -158,6 +158,23 @@ load_lib gdb-utils.exp > load_lib memory.exp > load_lib check-test-names.exp > =20 > +# The path to the GCORE script to test. > +global GCORE > +if ![info exists GCORE] { Please add {...} around the if expression: if {![info exists GCORE]} { Thanks, Andrew > + set GCORE [findfile $base_dir/../../gdb/gcore] > +} else { > + set GCORE "" > +} > +verbose "using GCORE =3D $GCORE" 2 > + > +proc has_gcore_script {} { > + if {[info exists ::GCORE]} { > +=09return 1 > + } else { > +=09return 0 > + } > +} > + > # The path to the GDB binary to test. > global GDB > =20 > --=20 > 2.44.0