From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16529 invoked by alias); 29 Oct 2015 12:31:16 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 16486 invoked by uid 89); 29 Oct 2015 12:31:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: xyzzy.0x04.net Received: from xyzzy.0x04.net (HELO xyzzy.0x04.net) (109.74.193.254) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 29 Oct 2015 12:31:07 +0000 Received: from hogfather.0x04.net (89-65-84-110.dynamic.chello.pl [89.65.84.110]) by xyzzy.0x04.net (Postfix) with ESMTPS id E2C093FEE2; Thu, 29 Oct 2015 13:31:20 +0100 (CET) Received: from [192.168.13.108] (89-65-84-110.dynamic.chello.pl [89.65.84.110]) by hogfather.0x04.net (Postfix) with ESMTPSA id 63C8C5800AC; Thu, 29 Oct 2015 13:31:04 +0100 (CET) Subject: Re: [PATCH v2 01/13] gdb/linux-record: Add testcases for a few syscalls. To: Pedro Alves , qiyaoltc@gmail.com References: <5628E5D6.5020706@0x04.net> <1445521166-14492-1-git-send-email-koriakin@0x04.net> <5632035E.30809@redhat.com> Cc: gdb-patches@sourceware.org From: =?UTF-8?Q?Marcin_Ko=c5=9bcielnicki?= Message-ID: <56321188.1040903@0x04.net> Date: Thu, 29 Oct 2015 13:03:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <5632035E.30809@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2015-10/txt/msg00677.txt.bz2 On 29/10/15 12:30, Pedro Alves wrote: > On 10/22/2015 02:39 PM, Marcin Kościelnicki wrote: >> gdb/testsuite/ChangeLog: >> >> * gdb.reverse/linux-fstatat-reverse.c: New test. >> * gdb.reverse/linux-fstatat-reverse.exp: New file. >> * gdb.reverse/linux-getresuid-reverse.c: New test. >> * gdb.reverse/linux-getresuid-reverse.exp: New file. >> * gdb.reverse/linux-pipe-reverse.c: New test. >> * gdb.reverse/linux-pipe-reverse.exp: New file. >> * gdb.reverse/linux-readv-reverse.c: New test. >> * gdb.reverse/linux-readv-reverse.exp: New file. >> * gdb.reverse/linux-recvmsg-reverse.c: New test. >> * gdb.reverse/linux-recvmsg-reverse.exp: New file. >> * gdb.reverse/linux-time-reverse.c: New test. >> * gdb.reverse/linux-time-reverse.exp: New file. >> * gdb.reverse/linux-waitpid-reverse.c: New test. >> * gdb.reverse/linux-waitpid-reverse.exp: New file. > > Excellent, thank you! > > I ran your tests against pristine/unfixed gdb, and linux-readv-reverse.exp > passes cleanly. This is on x86_64. Is that expected? The comments in > the test seem to suggest it would fail. That's expected. The test would only fail for x32 ABI, since it had the wrong size_iov, size_pointer, size_size_t. > >> +++ b/gdb/testsuite/gdb.reverse/linux-fstatat-reverse.c >> @@ -0,0 +1,39 @@ >> +/* This testcase is part of GDB, the GNU debugger. >> + >> + Copyright 2008-2015 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 . */ >> + >> +#define _GNU_SOURCE >> +#include >> +#include >> +#include >> + >> +void marker1 () > > We follow GNU formatting in tests too, unless there's a good > reason not to. So, line break after void. Also, this is C, > so write "(void)" for parameters. Thus: > > void > marker1 (void) > { OK. > >> +{ >> +} >> + >> +void marker2 () > > Likewise. > >> +{ >> +} >> + >> +struct stat buf; >> + >> +int main() { > > int > main () > { > Not int main(void)? > >> + marker1(); >> + int fd = open("/", O_PATH); >> + fstatat(fd, ".", &buf, 0); >> + marker2(); > > Space before parens. > > Several of the test programs have non-conforming indentation too. OK. > >> + return 0; >> +} >> diff --git a/gdb/testsuite/gdb.reverse/linux-fstatat-reverse.exp b/gdb/testsuite/gdb.reverse/linux-fstatat-reverse.exp >> new file mode 100644 >> index 0000000..23652e4 >> --- /dev/null >> +++ b/gdb/testsuite/gdb.reverse/linux-fstatat-reverse.exp >> @@ -0,0 +1,63 @@ >> +# Copyright 2015 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 file is part of the gdb testsuite. >> + >> +# >> +# This test tests linux fstatat syscall for reverse execution. > > Linux, uppercase. But, > >> +# >> + >> +if ![supports_reverse] { >> + return >> +} >> + >> + >> +if ![istarget "*linux*"] then { >> + verbose "Skipping linux syscall reverse tests." >> + return >> +} > > ... I think most of these tests could/should drop this target check, > and drop the "linux-" in the filename as well. Any target/port > that supports record/reverse execution should be able to run > this, as long as it manages to compile the test program. And if the > test program doesn't compile in such a port, it'll be automatically > skipped. OK. So a testcase that doesn't compile is not a problem? Because I just noticed the time test doesn't compile on platforms that don't have a time syscall, like s390x... > >> + >> +gdb_test "break marker1" \ >> + "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \ >> + "set breakpoint at marker1" >> + >> +gdb_test "break marker2" \ >> + "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \ >> + "set breakpoint at marker2" >> + >> +gdb_continue_to_breakpoint "marker1" ".*$srcfile:.*" >> + >> +gdb_continue_to_breakpoint "marker2" ".*$srcfile:.*" > > Most (all?) these tests run to marker1 and then immediately to > marker2. Can't they just continue to marker2 directly? OK, I'll fix that. > >> +++ b/gdb/testsuite/gdb.reverse/linux-waitpid-reverse.exp > > ... > >> +# will be remembered. If not, new contents (status) will be # used, and > > Spurious # after "will be". > >> +# the test will fail. >> + >> +gdb_test "print status" ".* = -1" "check waitpid record" > > Thanks, > Pedro Alves > Thanks for the review, I'll resend a fixed version.