Re: [PATCH v2] arc: New Synopsys ARC port

[Pedro Alves <palves at redhat dot com>]

On 09/16/2016 06:11 PM, Anton Kolesov wrote:
> Changes in V2:
> 
>     * Rename arc_unwind_cache struct to arc_prologue_cache.
>     * Rename arc_frame_cache to arc_make_frame_cache.
>     * Remove arc_create_cache - inline its code into the only place where it is
>       invoked.
>     * Do not manually init arc_prologue_cache fields to zero.
>     * Use character arrays instead of char pointers for XML feature names.
>     * Fix indentation of structure definitions.
>     * Fix off-by-one errors when iterating over register numbers.
>     * Fix various errors in comments.
>     * Add "Implement the XXX gdbarch method" comments where appropriate.
>     * Reformat ?: expressions.
>     * Use gdb_byte instead of char where appropriate.
>     * Use gdb_insn_length instead of a custom function that does the same.
>       Remove related functions which are not useful anymore:
>       arc_scream_to_the_void, arc_read_memory_for_disassembler,
>       arc_initialize_disassembler.
>     * Replace invalid implicit casts to bool (for example replace "!sal.line"
>       with "sal.line == 0").
>     * Don't make returned BLINK value a constant in arc_frame_prev_register.
>     * Remove unnecessary "name" local variable from arc_tdesc_init.
>     * Remove empty gdbarch_tdep structure for ARC.
>     * Remove inappropriate _initialize_arc_tdep declaration from arc-tdep.h.
>       Add it to arc-tdep.c to avoid compiler warnings.
>     * Remove excessive requirements on register numbers in XML target
>       descriptions.
>     * Remove fictional LIMM and RESERVED regnums.
>     * Remove LP_START and LP_END from aux-minimal feature. Those registers are
>       not required, and they were there to fill space between PC and STATUS32,
>       however that is not required anymore.
>     * Rename BYTES_IN_REGISTER to ARC_REGISTER_SIZE.
>     * Add a comment to arc_delayed_print_insn to document a case that null
>       exec_bfd is handled safely in disassembler.
>     * Replace two cases of size_t usage as a loop iterator over regnums.
>       Regnums are universally an "int" in GDB.
>     * Use std::max and std::min.

Thanks much.  Looks very good now.  I'm impressed!

Once Eli has OKed the documentation, this is good to go.

Noticed what looks like a small typo:

> diff --git a/gdb/features/Makefile b/gdb/features/Makefile
> index 809c811..6b8557b 100644
> --- a/gdb/features/Makefile
> +++ b/gdb/features/Makefile
> @@ -148,6 +148,8 @@ OUTPUTS = $(patsubst %,$(outdir)/%.dat,$(WHICH))
>  # to make on the command line.
>  XMLTOC = \
>  	aarch64.xml \
> +	arc/arc.v2.xml \

The file seems to be named "arc-v2.xml" instead.

> +    <reg name="blink" bitsize="32" type="code_ptr"/>
> +
> +    <!-- Here goes extension core registers: r32 - r57 -->
> +    <!-- Here goes ACCL/ACCH registers, r58, r59 -->
> +
> +    <!-- More core registers... -->
> +    <reg name="lp_count" bitsize="32" type="uint32" regnum="60"/>
> +    <!-- r61 is reserved -->
> +    <!-- r62 is long immediate value, not a real register -->
> +    <reg name="pcl" bitsize="32" type="code_ptr" regnum="63" group=""/>
> +  </feature>
> +

I see you're only adding descriptions with the slacks in place.
That will be necessary as fallback descriptions when debugging
against older stubs that don't report target descriptions, though
new stubs that do support xml description don't really need to
leave those spaces.  Since other projects tend to copy gdb's target 
descriptions (the license is permissive  exactly to allow that), it'd 
be nice to at least add a comment to the top of the files saying 
something to the effect.

Thanks,
Pedro Alves