Re: [PATCH 4/5] arc: Add disassembler helper

[Pedro Alves <palves@redhat.com>]

On 02/14/2017 10:01 AM, Anton Kolesov wrote:

> diff --git a/gdb/arch/arc-insn.c b/gdb/arch/arc-insn.c
> new file mode 100644
> index 0000000..2dc99a7
> --- /dev/null
> +++ b/gdb/arch/arc-insn.c
> @@ -0,0 +1,275 @@
> +/* ARC disassembler helper.
> +
> +   Copyright 2017 Free Software Foundation, Inc.
> +   Contributed by Synopsys Inc.

Please see:
 https://sourceware.org/gdb/wiki/ContributionChecklist#Attribution

(multiple instances)

> +
> +/* GDB header files.  */
> +#include "defs.h"

Hmm, the original idea of the gdb/arch/ dir was to hold files that could be
used by gdbserver too.  Note how the current files all include common-defs.h 
instead of defs.h.

This file seems to depend on gdb and opcodes, so ... not sure what to
suggest here.  Do you see using this code or parts of it in
gdbserver too in the future?

> +# This tests provides certain degree of testing for arch/arc-insn.c functions,

"These tests"

> +# however it is not a comprehensive testsuite that would go through all
> +# possible ARC instructions - instead this particular test is focused on branch
> +# instructions and whether branch targets are evaluated properly.  Most of the
> +# non-branch aspects of instruction decoder are used during prologue analysis,
> +# so are indirictly tested there.
> +
> +# To maintain separation of test data and test logic, all of the information
> +# about instructions, like if it has delay slot, condition code, branch target
> +# address, is all specified in the test assembly file as a symbols, while this
> +# test case reads those symbols to learn which values are right, then compares
> +# values coming from arc-insn.c with those found in symbols.  More information
> +# about requirements to actual test cases can be found in corresponding
> +# assembly file of this test case (arc-decode-insn.S).
> +
> +if {![istarget "arc*-*-*"]} then {
> +    verbose "Skipping ARC decoder test."
> +    return
> +}
> +
> +standard_testfile .S
> +
> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable "" ] != "" } {
> +    untested arc-decode-insn.exp
> +    return -1
> +}
> +
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}

Use prepare_for_testing instead of all the above.

> +
> +if ![runto_main] {
> +    fail "Can't run to main"
> +    return 0
> +}
> +
> +# Helper function that reads properties of instruction from the ELF file via
> +# its symbols and then confirms that decoder output aligns to the expected
> +# values.
> +proc test_branch_insn { test_name } {
> +
> +    # Make messages for failed cases more clear, by using hex in them.
> +    set pc [get_hexadecimal_valueof &${test_name}_start -1]
> +
> +    # Calculate instruction length, based on ${test_name}_end symbol.
> +    set end_pc [get_hexadecimal_valueof &${test_name}_end -1]
> +    set length [expr $end_pc - $pc]
> +
> +    set target_address [get_hexadecimal_valueof &${test_name}_target -1]
> +
> +    # Figure out if there is a delay slot, using symbol
> +    # ${test_name}_has_delay_slot.  Note that it should be read via &,
> +    # otherwise it would try to print value at the address specified in
> +    # ${test_name}_has_delay_slot, while a symbol value itself is required.
> +    if { 0 == [get_integer_valueof &${test_name}_has_delay_slot 0] } {
> +	set has_delay_slot 0
> +    } else {
> +	set has_delay_slot 1
> +    }
> +
> +    set cc [get_hexadecimal_valueof &${test_name}_cc 0]
> +
> +    gdb_test_sequence "mt print arc arc-instruction $pc" "" "
> +	length_with_limm=$length
> +	cc=$cc
> +	is_control_flow=1
> +	has_delay_slot=$has_delay_slot
> +	branch_target=$target_address"

The sequence should be a list, with each element being a line.

I think you probably have duplicate test messages.  Please check
with:
https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

Thanks,
Pedro Alves