Re: [PATCH 3/3] Add a test case for the jit-reader interface.

[Pedro Alves <palves@redhat.com>]

I've looked at the patch and noticed a few minor issues.

On 10/08/2012 12:47 PM, Sanjoy Das wrote:

> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/jit-reader.exp
> @@ -0,0 +1,72 @@
> +# Copyright 2011-2012 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/>.
> +
> +standard_testfile jithost.c
> +
> +if ![istarget x86_64-*-linux-gnu*] then {

We should move aways from strict istarget checks on x86_64/i686,
due to -m32 or -mx32, or even i686 with -m64.  One possibility is:

if { (![istarget x86_64-*-*] && ![istarget i?86-*-*]) || ![is_lp64_target] } {

But I'm not sure whether the test should run on x32.

> +    untested jit-reader.exp
> +    return -1;
> +}
> +
> +if {[skip_shlib_tests]} {
> +    untested jit-reader.exp

These should be silent.  IOW, remove the untested calls.

> +    return -1
> +}
> +
> +if {[get_compiler_info]} {
> +    warning "Could not get compiler info"
> +    untested jit-reader.exp

From <http://sourceware.org/gdb/wiki/GDBTestcaseCookbook>:

  In untested calls, please spell out the reason the test ends up untested, instead of
  just writing the test name, as with the latter we just end up with the test name
  duplicated in the gdb.sum output. For example:"

> +    return 1
> +}
> +
> +set jit_host_src ${srcfile}
> +set jit_host_bin ${binfile}
> +
> +set include_dir ${objdir}/../../
> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${jit_host_src}" "${jit_host_bin}" \
> +       executable  [list debug incdir=${include_dir}]] != "" } {
> +    untested jit-reader.exp
> +    return -1
> +}
> +
> +set jit_reader jitreader
> +set jit_reader_src ${jit_reader}.c
> +set jit_reader_bin [standard_output_file ${jit_reader}.so]
> +
> +if { [gdb_compile_shlib "${srcdir}/${subdir}/${jit_reader_src}" "${jit_reader_bin}" \
> +	  [list debug incdir=${include_dir}]] != "" } {
> +    untested jit-reader.exp
> +    return -1
> +}

Add a call to gdb_load_shlibs, so the .so is uploaded to remote targets.
(unless the test really only should be run on native targets.)

> +
> +proc jit_reader_test {} {
> +    global jit_host_bin
> +    global jit_reader_bin
> +    global verbose
> +
> +    clean_restart $jit_host_bin
> +
> +    if {$verbose > 0} {
> +	gdb_run_cmd "set debug jit 1"
> +    }
> +
> +    gdb_test_no_output "jit-reader-load ${jit_reader_bin}"
> +    gdb_run_cmd "run"
> +
> +    gdb_test "bt" "jit_function_00.*"
> +}
> +
> +jit_reader_test
> diff --git a/gdb/testsuite/gdb.base/jithost.c b/gdb/testsuite/gdb.base/jithost.c
> new file mode 100644
> index 0000000..967245c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/jithost.c

The test's sources miss copyright headers.

> diff --git a/gdb/testsuite/gdb.base/jitreader.c b/gdb/testsuite/gdb.base/jitreader.c
> new file mode 100644
> index 0000000..2438b0d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/jitreader.c
> @@ -0,0 +1,136 @@
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include "gdb/jit-reader.h"

Is this sure to pick up the build dir's jit-reader.h, or could it trip
on the system's installed copy from the system's gdb version?

I notice that the test's formatting is a bit inconsistent.  It's almost GNU
formatting, but at places it misses the space before parens.  It's not a
strict requirement for tests to follow the GNU conventions, but I think consistency
would be nice.  Issues I noticed (several instances of each):

> +struct jithost_abi {

{ does on next line, indented two spaces.

> +enum gdb_status
> +unwind_frame(struct gdb_reader_funcs* self, struct gdb_unwind_callbacks* cbs)

Missing space before parens.

> +extern struct gdb_reader_funcs* gdb_init_reader(void) {

{ does on next line.

"gdb_reader_funcs *gdb_init_reader"

-- 
Pedro Alves