This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] arc: New Synopsys ARC port
- From: Pedro Alves <palves at redhat dot com>
- To: Anton Kolesov <Anton dot Kolesov at synopsys dot com>, gdb-patches at sourceware dot org
- Cc: Francois dot Bedard at synopsys dot com
- Date: Wed, 31 Aug 2016 14:58:50 +0100
- Subject: Re: [PATCH 1/2] arc: New Synopsys ARC port
- Authentication-results: sourceware.org; auth=none
- References: <1472633399-32477-1-git-send-email-Anton.Kolesov@synopsys.com>
On 08/31/2016 09:49 AM, Anton Kolesov wrote:
> Since it is expected that 7.12 will be the last release that supports building
> GDB with C compiler (https://sourceware.org/ml/gdb/2016-05/msg00016.html),
> this patch contain some code that cannot be compiled with older GCC's default
> -std=gnu89 for C, in particular variables are declared at the first use,
> rather then at the top of the function.
This has not happened yet, and we still support building with a
C compiler. So this can't go in as is until that is resolved...
In general the patch looks pretty good to me, but I have a few
comments below.
>
> * GDB and GDBserver now build with a C++ compiler by default.
> diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
> new file mode 100644
> index 0000000..6e0e68d
> --- /dev/null
> +++ b/gdb/arc-tdep.c
> @@ -0,0 +1,1371 @@
> +/* Target dependent code for ARC arhitecture, for GDB.
> +
> + Copyright 2005-2016 Free Software Foundation, Inc.
Does this file really contain code that is that old?
> + Contributed by Synopsys Inc.
> +
> + This file is part of GDB.
> +
> + 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/>. */
> +
> +/* GDB header files. */
> +#include "defs.h"
> +#include "arch-utils.h"
> +#include "dwarf2-frame.h"
> +#include "frame-base.h"
> +#include "frame-unwind.h"
> +#include "gdbcore.h"
> +#include "gdbcmd.h"
> +#include "objfiles.h"
> +#include "trad-frame.h"
> +
> +/* ARC header files. */
> +#include "opcode/arc.h"
> +#include "arc-tdep.h"
> +
> +/* Default target descriptions. */
> +#include "features/arc-v2.c"
> +#include "features/arc-arcompact.c"
> +
> +/* The frame unwind cache for the ARC. Current structure is a stub, because
> + it should be filled in during the prologue analysis. */
> +
> +struct arc_unwind_cache
> + {
Formatting: { goes on column 0, and then reindent the fields as well.
> + /* The stack pointer at the time this frame was created; i.e. the caller's
> + stack pointer when this function was called. It is used to identify this
> + frame. */
> + CORE_ADDR prev_sp;
> +
> + /* Offsets for each register in the stack frame. */
> + struct trad_frame_saved_reg *saved_regs;
> + };
> +
> +/* Global debug flag. */
> +
> +int arc_debug;
> +
> +/* XML target description features. */
> +
> +static const char *const core_v2_feature_name = "org.gnu.gdb.arc.core.v2";
> +static const char *const
> + core_reduced_v2_feature_name = "org.gnu.gdb.arc.core-reduced.v2";
> +static const char *const
> + core_arcompact_feature_name = "org.gnu.gdb.arc.core.arcompact";
> +static const char *const
> + aux_minimal_feature_name = "org.gnu.gdb.arc.aux-minimal";
> +
Do these need to be pointers instead of arrays?
> +
> + In ARC PC register is a normal register so in most cases setting PC value
> + is a straightforward process: debugger just writes PC value. However it
> + gets trickier in case when current instruction is an instruction in delay
> + slot. In this case CPU will execute instruction at current PC value, then
> + will set PC to the current value of BTA register; also current instruction
> + cannot be branch/jump and some of the other instruction types. Thus if
> + debugger would try to just change PC value in this case, this instruction
> + will get executed, but then core will "jump" to the original branch target.
> +
> + Whether current instruction is a delay-slot instruction or not is indicated
> + by DE bit in STATUS32 register indicates if current instruction is a delay
> + slot instruction. This bit is writable by debug host, which allows debug
> + host to prevent core from jumping after the delay slot instruction. It
> + also works in another direction: setting this bit will make core to treat
> + any current instructions as a delay slot instruction and to set PC to the
> + current value of BTA register.
> +
> + To workaround issues with changing PC register while in delay slot
> + instruction, debugger should check for the STATUS32.DE bit and reset it if
> + it is set. No other change is required in this function. Most common
> + case, where this function might be required is calling inferior functions
> + from debugger. Generic GDB logic handles this pretty well: current values
> + of registers are stored, value of PC is changed (that is the job of this
> + function), and after inferior function is executed, GDB restores all
> + registers, include BTA and STATUS32, which also means that core is returned
> + to its original state of being halted on delay slot instructions.
> +
> + This method is useless for ARC 600, because it doesn't have externally
> + exposed BTA register. In the case of ARC 600 it is impossible to restore
> + core to it's state in all occasions thus core should never be halted (from
"to its".
> + the perspective of debugger host) in the delay slot. */
> +
> +/* Push dummy call return code sequence.
> +
> + We don't actually push any code. We just identify where a breakpoint can
> + be inserted to which we are can return and the resume address where we
> + should be called.
Something seems odd with the last sentence. "to which we are can"?
> +
> + ARC does not necessarily have an executable stack, so we can't put the
> + return breakpoint there.
What actually goes wrong if you put the breakpoint there? GDB should
manage to figure out a SIGSEGV at that address means the function
returned. x86-64 GNU/Linux has no executable stack either, for instance,
and:
(gdb) set debug infrun 1
(gdb) p return_false ()
infrun: clear_proceed_status_thread (Thread 0x7ffff7fc8700 (LWP 7333))
infrun: proceed (addr=0x4008d7, signal=GDB_SIGNAL_0)
infrun: proceed: resuming Thread 0x7ffff7fc8700 (LWP 7333)
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [Thread 0x7ffff7fc8700 (LWP 7333)] at 0x4008d7
infrun: infrun_async(1)
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun: 7333.7333.0 [Thread 0x7ffff7fc8700 (LWP 7333)],
infrun: status->kind = stopped, signal = GDB_SIGNAL_SEGV
infrun: Treating signal as SIGTRAP
infrun: TARGET_WAITKIND_STOPPED
infrun: stop_pc = 0x7fffffffd75f
infrun: BPSTAT_WHAT_STOP_SILENT
infrun: stop_waiting
infrun: clear_step_over_info
infrun: stop_all_threads
infrun: stop_all_threads, pass=0, iterations=0
infrun: Thread 0x7ffff7fc8700 (LWP 7333) not executing
infrun: stop_all_threads, pass=1, iterations=1
infrun: Thread 0x7ffff7fc8700 (LWP 7333) not executing
infrun: stop_all_threads done
$1 = 0
(gdb)
Note the GDB_SIGNAL_SEGV.
> Instead we put it at the entry point of the
> + function. This means the SP is unchanged.
> +
> + SP is a current stack pointer FUNADDR is an address of the function to be
> + called. ARGS is arguments to pass. NARGS is a number of args to pass.
> + VALUE_TYPE is a type of value returned. REAL_PC is a resume address when
> + the function is called. BP_ADDR is an address where breakpoint should be
> + set. Returns the updated stack pointer. */
> +
> +static CORE_ADDR
> +arc_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp, CORE_ADDR funaddr,
> + struct value **args, int nargs, struct type *value_type,
> + CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
> + struct regcache *regcache)
> +{
> + *real_pc = funaddr;
> + *bp_addr = entry_point_address ();
> + return sp;
> +}
> +
> +/* Determine whether a register can be read.
> +
> + All of the registers in the required set are readable. There are
> + write-only registers among the non-required, but since GDB doesn't know
> + anything about them, access is controlled by the GDB server. RESERVED and
> + LIMM are non-existent registers.
What does this mean, "non-existent registers" ? If they don't
exist, then why do they have names and are handled here?
I'm puzzled on what's the point of a register that can't be
neither read nor written?
> +
> + Returns TRUE if we _cannot_ read the register. */
> +
> +static int
> +arc_cannot_fetch_register (struct gdbarch *gdbarch, int regnum)
> +{
> + /* Assume that register is readable if it is unknown. */
> + switch (regnum)
> + {
> + case ARC_RESERVED_REGNUM:
> + case ARC_LIMM_REGNUM:
> + return TRUE;
> + default:
> + return FALSE;
> + }
> +}
> +
> +/* Determine whether a register can be written.
> +
> + It is assumed that first 64 regnums are core registers, hence standard ARC
> + rules for core registers apply and it is assumed that extension registers
> + are RW. Then PC has regnum 64, LP_START is 65, LP_END is 66, and STATUS32
> + is 67. All of those are RW in baremetal environment.
> +
> + Returns TRUE if we _cannot_ write the register. */
> +
> +static int
> +arc_cannot_store_register (struct gdbarch *gdbarch, int regnum)
> +{
> + /* Assume that register is writable if it is unknown. */
> + switch (regnum)
> + {
> + case ARC_RESERVED_REGNUM:
> + case ARC_LIMM_REGNUM:
> + case ARC_PCL_REGNUM:
> + return TRUE;
> + default:
> + return FALSE;
> + }
> +}
> +
> +/* Get the return value of a function from the registers/memory used to
> + return it, according to the convention used by the ABI - 4-bytes values are
> + in the R0,
s/in the R0/in R0/ ?
> while 8-byte values are in the R1.
ITYM "are in R0-R1". The function seems to extract it out
of both.
> +
> + TODO: This implementation ignores the case of "complex double", where
> + according to ABI, value is returned in the R0-R3 registers.
> +
> + TYPE is a returned value's type. VALBUF is a buffer for the returned
> + value. */
> +
> +static void
> +arc_extract_return_value (struct gdbarch *gdbarch, struct type *type,
> + struct regcache *regcache, gdb_byte *valbuf)
> +{
> + unsigned int len = TYPE_LENGTH (type);
> +
> + if (arc_debug)
> + debug_printf ("arc: extract_return_value\n");
> +
> + if (len <= BYTES_IN_REGISTER)
Where is BYTES_IN_REGISTER defined?
> + {
> + ULONGEST val;
> +
> + /* Get the return value from one register. */
> + regcache_cooked_read_unsigned (regcache, ARC_R0_REGNUM, &val);
> + store_unsigned_integer (valbuf, (int) len,
> + gdbarch_byte_order (gdbarch), val);
> +
> + if (arc_debug)
> + debug_printf ("arc: returning 0x%s\n", phex (val, BYTES_IN_REGISTER));
> + }
> + else if (len <= BYTES_IN_REGISTER * 2)
> + {
> + ULONGEST low, high;
> +
> + /* Get the return value from two registers. */
> + regcache_cooked_read_unsigned (regcache, ARC_R0_REGNUM, &low);
> + regcache_cooked_read_unsigned (regcache, ARC_R1_REGNUM, &high);
> +
> + store_unsigned_integer (valbuf, BYTES_IN_REGISTER,
> + gdbarch_byte_order (gdbarch), low);
> + store_unsigned_integer (valbuf + BYTES_IN_REGISTER,
> + (int) len - BYTES_IN_REGISTER,
> + gdbarch_byte_order (gdbarch), high);
> +
> + if (arc_debug)
> + debug_printf ("arc: returning 0x%s%s\n",
> + phex (high, BYTES_IN_REGISTER),
> + phex (low, BYTES_IN_REGISTER));
> + }
> + else
> + error (_("arc: extract_return_value: type length %u too large"), len);
> +}
> +
> +
> +/* Store the return value of a function into the registers/memory used to
> + return it, according to the convention used by the ABI.
> +
> + TODO: This implementation ignores the case of "complex double", where
> + according to ABI, value is returned in the R0-R3 registers.
> +
> + TYPE is a returned value's type. VALBUF is a buffer with the value to
> + return. */
> +
> +static void
> +arc_store_return_value (struct gdbarch *gdbarch, struct type *type,
> + struct regcache *regcache, const gdb_byte *valbuf)
> +{
> + unsigned int len = TYPE_LENGTH (type);
> +
> + if (arc_debug)
> + debug_printf ("arc: store_return_value\n");
> +
> + if (len <= BYTES_IN_REGISTER)
> + {
> + ULONGEST val;
> +
> + /* Put the return value into one register. */
> + val = extract_unsigned_integer (valbuf, (int) len,
> + gdbarch_byte_order (gdbarch));
> + regcache_cooked_write_unsigned (regcache, ARC_R0_REGNUM, val);
> +
> + if (arc_debug)
> + debug_printf ("arc: storing 0x%s\n", phex (val, BYTES_IN_REGISTER));
> + }
> + else if (len <= BYTES_IN_REGISTER * 2)
> + {
> + ULONGEST low, high;
> +
> + /* Put the return value into two registers. */
> + low = extract_unsigned_integer (valbuf, BYTES_IN_REGISTER,
> + gdbarch_byte_order (gdbarch));
> + high = extract_unsigned_integer (valbuf + BYTES_IN_REGISTER,
> + (int) len - BYTES_IN_REGISTER,
> + gdbarch_byte_order (gdbarch));
> +
> + regcache_cooked_write_unsigned (regcache, ARC_R0_REGNUM, low);
> + regcache_cooked_write_unsigned (regcache, ARC_R1_REGNUM, high);
> +
> + if (arc_debug)
> + debug_printf ("arc: storing 0x%s%s\n",
> + phex (high, BYTES_IN_REGISTER),
> + phex (low, BYTES_IN_REGISTER));
> + }
> + else
> + error (_("arc_store_return_value: type length too large."));
> +}
> +
> +/* Determine how a result of particular type is returned.
> +
> + Return the convention used by the ABI for returning a result of the given
> + type from a function; it may also be required to:
> +
> + 1. Set the return value (this is for the situation where the debugger user
> + has issued a "return <value>" command to request immediate return from
> + the current function with the given result; or
Parens opened but never closed.
> +
> + 2. Get the return value ((this is for the situation where the debugger
> + user has executed a "call <function>" command to execute the specified
> + function in the target program, and that function has a non-void result
> + which must be returned to the user. */
Spurious double-parens, and never closed.
These generic comments would be better placed in gdbarch.sh, and then
here just say:
/* Implement the XXX gdbarch method. */
Augmented with mention of any ARC specifics if any worth mentioning.
> +
> +static enum return_value_convention
> +arc_return_value (struct gdbarch *gdbarch, struct value *function,
> + struct type *valtype, struct regcache *regcache,
> + gdb_byte *readbuf, const gdb_byte *writebuf)
> +{
> + /* If the return type is a struct, or a union, or would occupy more than two
> + registers, the ABI uses the "struct return convention": the calling
> + function passes a hidden first parameter to the callee (in R0). That
> + parameter is the address at which the value being returned should be
> + stored. Otherwise, the result is returned in registers. */
> + int is_struct_return = (TYPE_CODE (valtype) == TYPE_CODE_STRUCT
> + || TYPE_CODE (valtype) == TYPE_CODE_UNION
> + || TYPE_LENGTH (valtype) > 2 * BYTES_IN_REGISTER);
> +
> + if (arc_debug)
> + debug_printf ("arc: return_value (readbuf = %p, writebuf = %p)\n",
> + readbuf, writebuf);
> +
> + if (writebuf != NULL)
> + {
> + /* Case 1. GDB should not ask us to set a struct return value: it
> + should know the struct return location and write the value there
> + itself. */
> + gdb_assert (!is_struct_return);
> + arc_store_return_value (gdbarch, valtype, regcache, writebuf);
> + }
> + else if (readbuf != NULL)
> + {
> + /* Case 2. GDB should not ask us to get a struct return value: it
> + should know the struct return location and read the value from there
> + itself. */
> + gdb_assert (!is_struct_return);
> + arc_extract_return_value (gdbarch, valtype, regcache, readbuf);
> + }
> +
> + return is_struct_return ? RETURN_VALUE_STRUCT_CONVENTION
> + : RETURN_VALUE_REGISTER_CONVENTION;
Needs to be wrapped in parens, so that the second line aligns properly
in emacs, per GNU convention:
return (is_struct_return ? RETURN_VALUE_STRUCT_CONVENTION
: RETURN_VALUE_REGISTER_CONVENTION);
Or perhaps better:
return (is_struct_return
? RETURN_VALUE_STRUCT_CONVENTION
: RETURN_VALUE_REGISTER_CONVENTION);
> +}
> +
> +/* Utility function to create a new frame cache structure. */
> +
> +static struct arc_unwind_cache *
> +arc_create_cache (struct frame_info *this_frame)
> +{
> + struct arc_unwind_cache *cache
> + = FRAME_OBSTACK_ZALLOC (struct arc_unwind_cache);
> +
> + /* Zero all fields. */
That's already done by ZALLOC.
> + cache->prev_sp = 0;
> +
> + /* Allocate space for saved register info. */
> + cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
> +
> + return cache;
> +}
> +
> +/* Return the base address of the frame. For ARC, the base address is the
> + frame pointer. */
> +
> +static CORE_ADDR
> +arc_frame_base_address (struct frame_info *this_frame, void **prologue_cache)
> +{
> + return (CORE_ADDR) get_frame_register_unsigned (this_frame, ARC_FP_REGNUM);
> +}
> +
> +/* Skip the prologue for the function at PC. This is done by checking from
> + the line information read from the DWARF, if possible; otherwise, we scan
> + the function prologue to find its end. */
> +
> +static CORE_ADDR
> +arc_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
> +{
> + if (arc_debug)
> + debug_printf ("arc: skip_prologue\n");
> +
> + CORE_ADDR func_addr;
> + const char *func_name;
> +
> + /* See what the symbol table says. */
> + if (find_pc_partial_function (pc, &func_name, &func_addr, NULL))
> + {
> + /* Found a function. */
> + CORE_ADDR postprologue_pc
> + = skip_prologue_using_sal (gdbarch, func_addr);
> +
> + if (postprologue_pc)
> + return max (pc, postprologue_pc);
> + }
> +
> + /* No prologue info in symbol table, have to analyze prologue. */
> +
> + /* Find an upper limit on the function prologue using the debug
> + information. If the debug information could not be used to provide that
> + bound, then pass 0 and arc_scan_prologue will estimate value itself. */
> + CORE_ADDR limit_pc = skip_prologue_using_sal (gdbarch, pc);
> + /* We don't have a proper analyze_prologue function yet, but its result
> + should be returned here. Currently GDB will just stop at the first
> + instruction of function if debug information doesn't have prologue info;
> + and if there is a debug info about prologue - this code path will not be
> + taken at all. */
> + return (limit_pc == 0 ? pc : limit_pc);
> +}
> +
> +/* arc_get_disassembler () may return different functions depending on bfd
> + type, so it is not possible to pass print_insn directly to
> + set_gdbarch_print_insn (). Instead this wrapper function is used. It also
> + may be used by other functions to get disassemble_info for address. It is
> + important to note, that those print_insn from opcodes always print
> + instruction to the stream specified in the info, so if that is not desired,
> + then print_insn in the info should be set to some function that doesn't
> + print anything, like arc_scream_into_the_void from this file. */
> +
> +static int
> +arc_delayed_print_insn (bfd_vma addr, struct disassemble_info *info)
> +{
> + int (*print_insn) (bfd_vma, struct disassemble_info *);
> + print_insn = arc_get_disassembler (exec_bfd);
Seems like this will crash if there's no exec_bfd ?
> + gdb_assert (print_insn != NULL);
> + return print_insn (addr, info);
> +}
> +
> +/** Baremetal breakpoint instructions.
"/*"
> +
> + ARC supports both big- and little-endian. However, instructions for
> + little-endian processors are encoded in the middle-endian: half-words are
> + in big-endian, while bytes inside the half-words are in little-endian; data
> + is represented in the "normal" little-endian. Big-endian processors treat
> + data and code identically.
> +
> + Assuming the number 0x01020304, it will be presented this way:
> +
> + Address : N N+1 N+2 N+3
> + little-endian : 0x04 0x03 0x02 0x01
> + big-endian : 0x01 0x02 0x03 0x04
> + ARC middle-endian : 0x02 0x01 0x04 0x03
> + */
> +
> +static const unsigned char arc_brk_s_be[] = { 0x7f, 0xff };
> +static const unsigned char arc_brk_s_le[] = { 0xff, 0x7f };
> +static const unsigned char arc_brk_be[] = { 0x25, 0x6f, 0x00, 0x3f };
> +static const unsigned char arc_brk_le[] = { 0x6f, 0x25, 0x3f, 0x00 };
Use gdb_byte.
> +
> +/* Get breakpoint which is appropriate for address at which it is to be set.
> +
> + For ARC ELF, breakpoint uses the 16-bit BRK_S instruction, which is 0x7fff
> + (little endian) or 0xff7f (big endian). We used to insert BRK_S even
> + instead of 32-bit instructions, which works mostly ok, unless breakpoint is
> + inserted into delay slot instruction. In this case if branch is taken
> + BLINK value will be set to address of instruction after delay slot, however
> + if we replaced 32-bit instruction in delay slot with 16-bit long BRK_S,
> + then BLINK value will have an invalid value - it will point to the address
> + after the BRK_S (which was there at the moment of branch execution) while
> + it should point to the address after the 32-bit long instruction. To avoid
> + such issues this function disassembles instruction at target location and
> + evaluates it value.
> +
> + ARC 600 supports only 16-bit BRK_S.
> +
> + NB: Baremetal GDB uses BRK[_S], while user-space GDB uses TRAP_S. BRK[_S]
> + is much better because it doesn't commit unlike TRAP_S, so it can be set in
> + delay slots; however it cannot be used in user-mode, hence usage of TRAP_S
> + in GDB for user-space.
> +
> + PCPTR is a pointer to the PC where we want to place a breakpoint. LENPTR
> + is a number of bytes used by the breakpoint. Returns the byte sequence of
> + a breakpoint instruction. */
> +
> +static const unsigned char *
> +arc_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
> + int *lenptr)
> +{
> + struct disassemble_info di;
> + arc_initialize_disassembler (gdbarch, &di);
> + size_t length_with_limm = arc_delayed_print_insn (*pcptr, &di);
Seems to me that if you need to do this instead of being able
to use gdb_insn_length, then that leaves gdb_insn_length calls in
common code broken. So, doesn't gdb_insn_length work here? Why not?
> +
> + /* Replace 16-bit instruction with BRK_S, replace 32-bit instructions with
> + BRK. LIMM is part of instruction length, so it can be either 4 or 8
> + bytes. */
> + if ((length_with_limm == 4 || length_with_limm == 8)
> + && !arc_mach_is_arc600 (gdbarch))
> + {
> + *lenptr = sizeof (arc_brk_le);
> + return (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> + ? arc_brk_be : arc_brk_le;
Wrap in parens, realign.
> + }
> + else
> + {
> + *lenptr = sizeof (arc_brk_s_le);
> + return (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> + ? arc_brk_s_be : arc_brk_s_le;
Wrap in parens, realign.
> + }
> +}
> +
> +/* Frame unwinder for normal frames. */
> +
> +static struct arc_unwind_cache *
> +arc_frame_cache (struct frame_info *this_frame)
> +{
> + if (arc_debug)
> + debug_printf ("arc: frame_cache\n");
> +
> + struct gdbarch *gdbarch = get_frame_arch (this_frame);
> + struct arc_unwind_cache *cache = arc_create_cache (this_frame);
> +
> + CORE_ADDR block_addr = get_frame_address_in_block (this_frame);
> + CORE_ADDR prev_pc = get_frame_pc (this_frame);
> +
> + CORE_ADDR entrypoint, prologue_end;
> + if (find_pc_partial_function (block_addr, NULL, &entrypoint, &prologue_end))
> + {
> + struct symtab_and_line sal = find_pc_line (entrypoint, 0);
> + if (!sal.line)
Not a boolean, so:
if (sal.line == 0)
> + /* No line info so use current PC. */
> + prologue_end = prev_pc;
> + else if (sal.end < prologue_end)
> + /* The next line begins after the function end. */
> + prologue_end = sal.end;
> +
> + prologue_end = min (prologue_end, prev_pc);
> + }
Indentation look odd here.
> +
> +/* Unwind a register from a normal frame. */
> +
> +static struct value *
> +arc_frame_prev_register (struct frame_info *this_frame,
> + void **this_cache, int regnum)
> +{
> + if (arc_debug)
> + debug_printf ("arc: frame_prev_register (regnum = %d)\n", regnum);
> +
> + if (*this_cache == NULL)
> + *this_cache = arc_frame_cache (this_frame);
> + struct arc_unwind_cache *cache = (struct arc_unwind_cache *) (*this_cache);
> +
> + struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +
> + /* If we are asked to unwind the PC, then we need to return BLINK instead:
> + the saved value of PC points into this frame's function's prologue, not
> + the next frame's function's resume location. `frame_unwind_got_constant`
> + is used to return non_lvalue, because it is not possible to modify PC
s/non_lvalue/not_lval
> + directly (one can modify BLINK, if they wish). If
> + trad_frame_get_prev_register (ARC_BLINK_REGNUM) would be used here, than
> + it would be possible to modify PC of frame via altering BLINK of
> + next_frame, or at least I believe that should be possible technically,
> + however sematically could be confusing, so better to avoid this, like
> + other arches do. */
What other archs do this for this purpose?
> + if (regnum == gdbarch_pc_regnum (gdbarch))
> + {
> + CORE_ADDR blink;
> + blink = frame_unwind_register_unsigned (this_frame, ARC_BLINK_REGNUM);
> + return frame_unwind_got_constant (this_frame, regnum, blink);
> + }
> +
> +
> +/* Identify some properties of particular registers for the DWARF unwinder.
> + */
Formatting.
> +/* Initialize target description for the ARC.
> +
> + Returns TRUE if input tdesc was valid and in this case it will assign TDESC
> + and TDESC_DATA output parameters. */
> +
> +static int
> +arc_tdesc_init (struct gdbarch_info info, const struct target_desc **tdesc,
> + struct tdesc_arch_data **tdesc_data)
> +{
> + if (arc_debug)
> + debug_printf ("arc: Arhitecture initialization.\n");
Typo.
> +
> + const struct target_desc *tdesc_loc = info.target_desc;
> +
> + /* Depending on whether this is ARCompact or ARCv2 we will assigned
> + different default registers sets (which will differ in exactly two core
> + registers). GDB will also refuse to accept register feature from invalid
> + ISA - v2 features can be used only with v2 ARChitecture. registers of
> + invalid architecture. We read bfd_arch_info, which looks like to be a
> + safe bet here, as it looks like it is always initialized even when we
> + don't pass any elf file to GDB at all (it uses default arch in this
> + case). Also GDB will call this function multiple times, and if XML
> + target description file contains architecture specifications, then GDB
> + will set this architecture to info.bfd_arch_info, overriding value from
> + ELF file if they are different. That means that, where matters, this
> + value is always our best guess on what CPU we are debugging. It has been
> + noted that that architecture specified in tdesc file has higher
> + precedence over ELF and even "set architecture" - that is, using "set
> + architecture" command will have no effect when tdesc has "arch" tag. */
> + /* Cannot use arc_mach_is_arcv2 (), because gdbarch is not created yet. */
Typo "we will assigned". Uppercase "registers of invalid architecture" sentence
seems stray.
> + const int is_arcv2 = (info.bfd_arch_info->mach == bfd_mach_arc_arcv2);
> + int is_reduced_rf;
> + const char *const *core_regs;
> + const char *core_feature_name;
> +
> + /* If target doesn't provide a description - use default one. */
> + if (!tdesc_has_registers (tdesc_loc))
> + {
> + if (is_arcv2)
> + {
> + tdesc_loc = tdesc_arc_v2;
> + if (arc_debug)
> + debug_printf ("arc: Using default register set for ARC v2.\n");
> + }
> + else
> + {
> + tdesc_loc = tdesc_arc_arcompact;
> + if (arc_debug)
> + debug_printf ("arc: Using default register set for ARCompact.\n");
> + }
> + }
> + else
> + {
> + if (arc_debug)
> + debug_printf ("arc: Using provided register set.\n");
> + }
> + gdb_assert (tdesc_loc);
No implicit boolean coercion, write:
gdb_assert (tdesc_loc != NULL);
throughout.
> +
> + /** Now we can search for base registers. Core registers can be either full
> + or reduced. Summary:
Use "/*".
> +
> + - core.v2 + aux-minimal
> + - core-reduced.v2 + aux-minimal
> + - core.arcompact + aux-minimal
> +
> + NB: It is intirely feaseble to have ARCompact with reduced
"feasible"
> + core regs, but we ignore that because GCC doesn't support that and at
> + the same time this architecture version is obsolete. */
What is obsolete? arcompact, or arcompact with reduced core regs?
Please clarify the comment.
> + const struct tdesc_feature *feature
> + = tdesc_find_feature (tdesc_loc, core_v2_feature_name);
> + if (feature)
feature != NULL. Here and multiple other places.
> + {
> +
> + struct tdesc_arch_data *tdesc_data_loc = tdesc_data_alloc ();
> +
> + gdb_assert (feature);
> + int valid_p = 1;
> +
> + for (size_t i = ARC_FIRST_CORE_REGNUM; i < ARC_LAST_CORE_REGNUM; i++)
> + {
> + /* R61 and R62 do never exist in real hardware, however have to check for
> + them to preserve compatibility with old gdbserver which reserve a slot
> + for them in g/G-packets. */
> +
> + /* If rf16, then skip extra registers. */
> + if (is_reduced_rf && ((i >= ARC_R4_REGNUM && i <= ARC_R9_REGNUM)
> + || (i >= ARC_R16_REGNUM && i <= ARC_R25_REGNUM)))
> + continue;
> +
> + /* This subtraction is superflouos, because FIRST_CORE_REGNUM is 0, but
"superfluous"
> + OTOH it adds a level of abstraction between arrays and regnums. Not
> + sure if that is a good idea. */
> + const char *name = core_regs[i - ARC_FIRST_CORE_REGNUM];
It's common to just assume first register is 0. But just pick one or the
other way, and drop the comment, which ends up just being a distraction.
> +
> + valid_p = tdesc_numbered_register (feature, tdesc_data_loc, i, name);
> +
> + /* - Ignore errors in extension registers - they are optional.
> + - Ignore missing ILINK because it doesn't make sense for Linux.
> + - Ignore missing ILINK2 when architecture is ARCompact, because it
> + doesn't make sense for Linux targets.
> +
> + In theory those optional registers should be in separate features,
> + but that's just crazy - features would be tiny and numerous and would
> + be make for complex maintanence, especially since regnums of
> + different features would interleve. */
> + if (!valid_p && (i <= ARC_SP_REGNUM || i == ARC_BLINK_REGNUM
> + || i == ARC_LP_COUNT_REGNUM || i == ARC_PCL_REGNUM
> + || (i == ARC_R30_REGNUM && is_arcv2)))
> + {
> + arc_print (_("Error: Cannot find required register `%s' "
> + "in feature `%s'.\n"), name, core_feature_name);
> + tdesc_data_cleanup (tdesc_data_loc);
> + return FALSE;
> + }
> + }
> +
> + /* Mandatory AUX registeres are intentionally few and are common between
> + ARCompact and ARC v2, so same code can be used for both. */
> + feature = tdesc_find_feature (tdesc_loc, aux_minimal_feature_name);
> + if (!feature)
feature == NULL
> + {
> + arc_print (_("Error: Cannot find required feature `%s' in supplied "
> + "target description.\n"), aux_minimal_feature_name);
> + tdesc_data_cleanup (tdesc_data_loc);
> + return FALSE;
> + }
> +
> + for (size_t i = ARC_FIRST_AUX_REGNUM; i < ARC_LAST_AUX_REGNUM; i++)
> + {
> + const char *name = aux_minimal_register_names[i - ARC_FIRST_AUX_REGNUM];
> + valid_p = tdesc_numbered_register (feature, tdesc_data_loc, i, name);
> + /* LP registers are optional. */
> + if (!valid_p && (i != ARC_LP_START_REGNUM) && (i != ARC_LP_END_REGNUM))
Drop unnecessary parens.
> + {
> + arc_print (_("Error: Cannot find required register `%s' "
> + "in feature `%s'.\n"),
> + name, tdesc_feature_name (feature));
> + tdesc_data_cleanup (tdesc_data_loc);
> + return FALSE;
> + }
> + }
> +
> + *tdesc = tdesc_loc;
> + *tdesc_data = tdesc_data_loc;
> +
> + return TRUE;
> +}
> +
> +static struct gdbarch *
> +arc_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> +{
> + const struct target_desc *tdesc;
> + struct tdesc_arch_data *tdesc_data;
> + if (!arc_tdesc_init (info, &tdesc, &tdesc_data))
> + return NULL;
> +
> + /* Allocate the ARC-private target-dependent information structure, and the
> + GDB target-independent information structure. */
> + struct gdbarch_tdep *tdep = XCNEW (struct gdbarch_tdep);
> + struct gdbarch *gdbarch = gdbarch_alloc (&info, tdep);
> +
> + /* Data types. */
> + set_gdbarch_short_bit (gdbarch, 16);
> + set_gdbarch_int_bit (gdbarch, 32);
> + set_gdbarch_long_bit (gdbarch, 32);
> + set_gdbarch_long_long_bit (gdbarch, 64);
> + set_gdbarch_long_long_align_bit (gdbarch, 32);
> + set_gdbarch_float_bit (gdbarch, 32);
> + set_gdbarch_float_format (gdbarch, floatformats_ieee_single);
> + set_gdbarch_double_bit (gdbarch, 64);
> + set_gdbarch_double_format (gdbarch, floatformats_ieee_double);
> + set_gdbarch_ptr_bit (gdbarch, 32);
> + set_gdbarch_addr_bit (gdbarch, 32);
> + set_gdbarch_char_signed (gdbarch, 0);
> +
> + set_gdbarch_write_pc (gdbarch, arc_write_pc);
> +
> + set_gdbarch_virtual_frame_pointer (gdbarch, arc_virtual_frame_pointer);
> +
> + /* tdesc_use_registers expects gdbarch_num_regs to return number of registers
> + parsed by gdbarch_init, and then it will add all of the remaining
> + registers and will increase number of registers. */
> + set_gdbarch_num_regs (gdbarch, ARC_LAST_REGNUM + 1);
> + set_gdbarch_num_pseudo_regs (gdbarch, 0);
> + set_gdbarch_sp_regnum (gdbarch, ARC_SP_REGNUM);
> + set_gdbarch_pc_regnum (gdbarch, ARC_PC_REGNUM);
> + set_gdbarch_ps_regnum (gdbarch, ARC_STATUS32_REGNUM);
> + set_gdbarch_fp0_regnum (gdbarch, -1); /* No FPU registers. */
> +
> + set_gdbarch_dummy_id (gdbarch, arc_dummy_id);
> + set_gdbarch_push_dummy_call (gdbarch, arc_push_dummy_call);
> + set_gdbarch_push_dummy_code (gdbarch, arc_push_dummy_code);
> +
> + set_gdbarch_cannot_fetch_register (gdbarch, arc_cannot_fetch_register);
> + set_gdbarch_cannot_store_register (gdbarch, arc_cannot_store_register);
> +
> + set_gdbarch_believe_pcc_promotion (gdbarch, 1);
> +
> + set_gdbarch_return_value (gdbarch, arc_return_value);
> +
> + set_gdbarch_skip_prologue (gdbarch, arc_skip_prologue);
> + set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
> +
> + set_gdbarch_breakpoint_from_pc (gdbarch, arc_breakpoint_from_pc);
> +
> + /* On ARC 600 BRK_S instruction advances PC, unlike other ARC cores. */
> + if (!arc_mach_is_arc600 (gdbarch))
> + set_gdbarch_decr_pc_after_break (gdbarch, 0);
> + else
> + set_gdbarch_decr_pc_after_break (gdbarch, 2);
> +
> + set_gdbarch_unwind_pc (gdbarch, arc_unwind_pc);
> + set_gdbarch_unwind_sp (gdbarch, arc_unwind_sp);
> +
> + set_gdbarch_frame_align (gdbarch, arc_frame_align);
> +
> + set_gdbarch_print_insn (gdbarch, arc_delayed_print_insn);
> +
> + set_gdbarch_cannot_step_breakpoint (gdbarch, 1);
> +
> + /* "nonsteppable" wachpoint means that watchpoint triggers before
"watchpoint"
> + instruction is committed, therefore it is required to remove watchpoint
> + to step though instruction that triggers it. ARC watchpoints trigger
> + only after instruction is committed, thus there is no need to remove
> + them. In fact on ARC watchpoint for memory writes may trigger with more
> + significant delay, like one or two instructions, depending on type of
> + memory where write is performed (CCM or external) and next instruction
> + after the memory write. That value used to be set to "1", but that is
> + not correct for ARC - that was causing GDB to step one additional
> + instruction after watchpoint is triggered and that is not needed. */
I'd drop the last sentence talking about what the value used to be set to.
> + set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 0);
> +
> + /* This doesn't include possible long-immediate value. */
> + set_gdbarch_max_insn_length (gdbarch, 4);
> +
> + /* Frame unwinders and sniffers. */
> + dwarf2_frame_set_init_reg (gdbarch, arc_dwarf2_frame_init_reg);
> + dwarf2_append_unwinders (gdbarch);
> + frame_unwind_append_unwinder (gdbarch, &arc_frame_unwind);
> + frame_base_set_default (gdbarch, &arc_normal_base);
> +
> + /* Setup stuff specific to a particular environment (baremetal or Linux).
> + It can override functions set earlier. */
> + gdbarch_init_osabi (info, gdbarch);
> +
> + tdesc_use_registers (gdbarch, tdesc, tdesc_data);
> +
> + return gdbarch;
> +}
> +
> +
> +static void
> +arc_dump_tdep (struct gdbarch *gdbarch, struct ui_file *file)
> +{
> + /* Empty for now. */
> +}
> +
> +/* Dummy printf-like function. Used by arc_initialize_disassembler, see
> + explanation of its purpose there. */
> +
> +static int
> +arc_scream_into_the_void (void *file, const char *f, ...)
> +{
> + return 0;
> +}
> +
> +/* Wrapper for the target_read_memory function. */
> +
> +static int
> +arc_read_memory_for_disassembler (bfd_vma memaddr, bfd_byte *myaddr,
> + unsigned int length,
> + struct disassemble_info *info)
> +{
> + return target_read_memory ((CORE_ADDR) memaddr, (gdb_byte *) myaddr,
> + (int) length);
> +}
> +
> +
> +void
> +arc_initialize_disassembler (struct gdbarch *gdbarch,
> + struct disassemble_info *info)
This isn't used outside this .c file in this patch, so make it
static. But see comment above about gdb_insn_length.
> +{
> + /* opcodes disassembler will always print instruction to the stream, which
> + is not desired when instruction is disassembled for analysis. Stream
> + cannot be set to NULL, because it is used unconditionally in the opcodes,
> + so the only way is to provide dummy printf-like function which will not
> + actually print anything anywhere. */
> + init_disassemble_info (info, gdb_stderr, arc_scream_into_the_void);
> + info->arch = gdbarch_bfd_arch_info (gdbarch)->arch;
> + info->mach = gdbarch_bfd_arch_info (gdbarch)->mach;
> + info->endian = gdbarch_byte_order (gdbarch);
> + info->read_memory_func = arc_read_memory_for_disassembler;
> +}
> +
> new file mode 100644
> index 0000000..9e47167
> --- /dev/null
> +++ b/gdb/arc-tdep.h
> @@ -0,0 +1,133 @@
> +/* Target dependent code for ARC arhitecture, for GDB.
> +
> + Copyright 2005-2016 Free Software Foundation, Inc.
Same comment as before.
> + Contributed by Synopsys Inc.
> +
> + This file is part of GDB.
> +
> + 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/>. */
> +
> +#ifndef ARC_TDEP_H
> +#define ARC_TDEP_H
> +
> +/* Need disassemble_info. */
> +#include "dis-asm.h"
> +
> +/* Certain limitations are imposed on GDB register numbers to simplify logic
> + in the GDB. Those limitations apply to both ARCompact and ARC v2.
> +
> + 1. ARC can have up to 64 core registers, and each one of them has same GDB
> + regnum as an architectural number. So R0 always has regnum 0, LP_COUNT
> + always has regnum 60.
> +
> + 2. PC always has regnum 64. That register is mandatory.
> +
> + 3. LP_START and LP_END has regnums 65 and 66 respectively, those registers
> + are optional, and those register numbers shall not be occupied by other
> + registers when LP_START and LP_END are not present.
> +
> + 4. STATUS32 always has regnum 67. */
> +
> +enum arc_regnum
> + {
> + /* Core registers. */
> + ARC_R0_REGNUM = 0,
> + ARC_FIRST_CORE_REGNUM = ARC_R0_REGNUM,
> + ARC_R1_REGNUM = 1,
> + ARC_R4_REGNUM = 4,
> + ARC_R7_REGNUM = 7,
> + ARC_R9_REGNUM = 9,
> + ARC_R13_REGNUM = 13,
> + ARC_R16_REGNUM = 16,
> + ARC_R25_REGNUM = 25,
> + /* Global data pointer. */
> + ARC_GP_REGNUM,
> + /* Frame pointer. */
> + ARC_FP_REGNUM,
> + /* Stack pointer. */
> + ARC_SP_REGNUM,
> + /* Return address from interrupt. */
> + ARC_ILINK_REGNUM,
> + ARC_R30_REGNUM,
> + /* Return address from function. */
> + ARC_BLINK_REGNUM,
> + /* Zero-delay loop counter. */
> + ARC_LP_COUNT_REGNUM = 60,
> + /* Reserved. */
> + ARC_RESERVED_REGNUM,
> + /* Long immediate value (not a physical register. */
> + ARC_LIMM_REGNUM,
> + /* Program counter, aligned to 4-bytes, read-only. */
> + ARC_PCL_REGNUM,
> + ARC_LAST_CORE_REGNUM = ARC_PCL_REGNUM,
> + /* AUX registers. */
> + /* Actual program counter. */
> + ARC_PC_REGNUM,
> + ARC_FIRST_AUX_REGNUM = ARC_PC_REGNUM,
> + /* Zero-delay loop start instruction. */
> + ARC_LP_START_REGNUM,
> + /* Zero-delay loop next-after-last instruction. */
> + ARC_LP_END_REGNUM,
> + /* Status register. */
> + ARC_STATUS32_REGNUM,
> + ARC_LAST_REGNUM = ARC_STATUS32_REGNUM,
> + ARC_LAST_AUX_REGNUM = ARC_STATUS32_REGNUM,
> +
> + /* Additional ABI constants. */
> + ARC_FIRST_ARG_REGNUM = ARC_R0_REGNUM,
> + ARC_LAST_ARG_REGNUM = ARC_R7_REGNUM,
> + ARC_FIRST_CALLEE_SAVED_REGNUM = ARC_R13_REGNUM,
> + ARC_LAST_CALLEE_SAVED_REGNUM = ARC_R25_REGNUM,
> + };
> +
> +#define BYTES_IN_REGISTER 4
> +
> +#define arc_print(fmt, args...) fprintf_unfiltered (gdb_stdlog, fmt, ##args)
> +
> +extern int arc_debug;
> +
> +/* Target-dependent information. */
> +
> +struct gdbarch_tdep
> + {
> + };
Formatting is off. { should on column 0.
Empty structs with no fields is not valid C89 either.
How about just simply dropping these placeholders, and add
them back when they're actually needed?
> +
> +
> +void _initialize_arc_tdep (void);
The _initialize functions are special. Move this to just above
the definition, and add a -Wmissing-prototypes comment. You'll
find numerous examples in the tree.
> @node ARM
> @subsection ARM
>
> @@ -40907,6 +40932,7 @@ registers using the capitalization used in the description.
>
> @menu
> * AArch64 Features::
> +* ARC Features::
> * ARM Features::
> * i386 Features::
> * MicroBlaze Features::
> @@ -40932,6 +40958,69 @@ The @samp{org.gnu.gdb.aarch64.fpu} feature is optional. If present,
> it should contain registers @samp{v0} through @samp{v31}, @samp{fpsr},
> and @samp{fpcr}.
>
> +@node ARC Features
> +@subsection ARC Features
> +@cindex target descriptions, ARC Features
> +
> +ARC processors are highly configurable, so even core registers and their amount
Registers can be counted, so s/amount/number/
> +are not completely predetermined. In addition flags and PC registers which are
s/are/is/.
> +important to GDB are not "core" registers in ARC. To simplify internal GDB
> +design some requirements are applied to ARC target descriptions:
> +
> +@itemize @bullet
> +@item
> +One of the core registers features must be present. At all occasions GDB
> +register number must be equal to architectural number of that register, so for
> +example regnum of @samp{blink} is always 31.
> +
> +@item @samp{org.gnu.gdb.arc.aux-minimal} is mandatory and registers in it has
> +fixed GDB register numbers.
> +
> +@item All other features are optional and cannot have registers with register
> +number less then 68.
GDB should be mapping the tdesc numbers to internal numbers, so I don't
understand why do the register numbers matter in the target description at all?
It should only matter for the fallback descriptions built in gdb, in
order to match the layout of the g/G packets of remote servers that
don't send in tdescs over the wire, I believe?
Thanks,
Pedro Alves