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

Pedro Alves
2015-10-29T11:30:44Z

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.

> +++ 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)
{

> +{
> +}
> +
> +void marker2 ()

Likewise.

> +{
> +}
> +
> +struct stat buf;
> +
> +int main() {

int
main ()
{

> + marker1();
> + int fd = open("/", O_PATH);
> + fstatat(fd, ".", &buf, 0);
> + marker2();

Space before parens.

Several of the test programs have non-conforming indentation too.

> + 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.

> +
> +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?

> +++ 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