Re: [PATCH v2 01/13] gdb/linux-record: Add testcases for a few syscalls.

["Marcin Kościelnicki" <koriakin@0x04.net>]

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 <http://www.gnu.org/licenses/>.  */
>> +
>> +#define _GNU_SOURCE
>> +#include <unistd.h>
>> +#include <fcntl.h>
>> +#include <sys/stat.h>
>> +
>> +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 <http://www.gnu.org/licenses/>.
>> +
>> +# 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.