From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25689 invoked by alias); 26 Aug 2018 17:19:36 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 25670 invoked by uid 89); 26 Aug 2018 17:19:35 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=stub, connections, notation, nits X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 26 Aug 2018 17:19:32 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 74C461E012; Sun, 26 Aug 2018 13:19:30 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=simark.ca; s=mail; t=1535303970; bh=RKEbqXNO4sg7my0oHaotofX3BtP/5EaQT99n1S+f4Gs=; h=Subject:To:References:From:Date:In-Reply-To:From; b=URvhEtyJh6NCU6KPAjYywbv4RqV9ZhAyA3j82L3bI7tdmwfemk3AgQ5Sqt2ntlCwP D+8I6SC78r8cSQYvum/Z4h3ujcdPeDzWIpQkk8m0xPyRisqvNeVTh0a7Xn7rmu41bo VzxMw7F7rkMkAjy16FI/xi7IhnInUYNzyUuBu0J0= Subject: Re: [PATCH 3/3] GDB: New target s12z To: John Darrington , gdb-patches@sourceware.org References: <20180823173526.26144-1-john@darrington.wattle.id.au> <20180823173526.26144-3-john@darrington.wattle.id.au> From: Simon Marchi Message-ID: Date: Sun, 26 Aug 2018 17:19:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: <20180823173526.26144-3-john@darrington.wattle.id.au> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-08/txt/msg00636.txt.bz2 Hi John, I did a first pass review and noted a few minor nits. I didn't get too deep in the frame_id/unwind code because I'm not too familiar with that. You should include the new file in the ALL_TARGET_OBS makefile target, so that it's included in a --enable-targets=all build. On 2018-08-23 1:35 p.m., John Darrington wrote: > gdb/ > * configure.tgt: Add configuration for s12z. > * s12z-tdep.c: New file. > * NEWS: Mention new target. > --- > gdb/NEWS | 4 + > gdb/configure.tgt | 6 + > gdb/s12z-tdep.c | 402 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 412 insertions(+) > create mode 100644 gdb/s12z-tdep.c > > diff --git a/gdb/NEWS b/gdb/NEWS > index a7a3674375..c46056525a 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -3,6 +3,10 @@ > > *** Changes since GDB 8.2 > > +* New targets > + > + NXP S12Z s12z-*-elf > + > * GDB and GDBserver now support IPv6 connections. IPv6 addresses > can be passed using the '[ADDRESS]:PORT' notation, or the regular > 'ADDRESS:PORT' method. > diff --git a/gdb/configure.tgt b/gdb/configure.tgt > index 5e3bd5de71..72de1a1e4a 100644 > --- a/gdb/configure.tgt > +++ b/gdb/configure.tgt > @@ -643,6 +643,12 @@ spu*-*-*) > build_gdbserver=yes > ;; > > +s12z-*-*) > + # Target: Freescale S12z > + gdb_target_obs="s12z-tdep.o" > + build_gdbserver=yes I don't think you should include build_gdbserver. It is only valid if you have a gdbserver port, for when building natively on that architecture. > + ;; > + > tic6x-*-*linux) > # Target: GNU/Linux TI C6x > gdb_target_obs="tic6x-tdep.o tic6x-linux-tdep.o solib-dsbt.o \ > diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c > new file mode 100644 > index 0000000000..5169025e6b > --- /dev/null > +++ b/gdb/s12z-tdep.c > @@ -0,0 +1,402 @@ > +/* Target-dependent code for the S12Z, for the GDB. > + Copyright (C) 2018 Free Software Foundation, 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 . */ > + > +/* Much of this file is shamelessly copied from or1k-tdep.c and others */ You don't have to include this, it's perfectly encouraged to share code between ports :). > + > +#include "defs.h" > + > +#include "arch-utils.h" > +#include "block.h" > +#include "cli/cli-decode.h" > +#include "common-defs.h" > +#include "dis-asm.h" > +#include "dwarf2-frame.h" > +#include "elf-bfd.h" > +#include "floatformat.h" > +#include "frame-base.h" > +#include "frame.h" > +#include "frame-unwind.h" > +#include "gdbcmd.h" > +#include "gdbcore.h" > +#include "gdbtypes.h" > +#include "infcall.h" > +#include "inferior.h" > +#include "language.h" > +#include "objfiles.h" > +#include "observable.h" > +#include "opcode/s12z.h" > +#include "osabi.h" > +#include "regcache.h" > +#include "reggroups.h" > +#include "remote.h" > +#include "symcat.h" > +#include "symfile.h" > +#include "symtab.h" > +#include "target-descriptions.h" > +#include "target.h" > +#include "trad-frame.h" > +#include "user-regs.h" > +#include "valprint.h" > +#include "value.h" Are all these includes necessary? Can you try to reduce to what you actually use? > +#include This should not be necessary. > + > +#define N_PHYSICAL_REGISTERS (S12Z_N_REGISTERS - 2) > + > +static const int reg_perm[N_PHYSICAL_REGISTERS] = > + { > + REG_D0, > + REG_D1, > + REG_D2, > + REG_D3, > + REG_D4, > + REG_D5, > + REG_D6, > + REG_D7, > + REG_X, > + REG_Y, > + REG_S, > + REG_P, > + REG_CCW > + }; > + > +static const char * > +s12z_register_name (struct gdbarch *gdbarch, int regnum) > +{ > + return registers[reg_perm[regnum]].name; Which "registers" variable does this refer to? > +} > + > +static CORE_ADDR > +s12z_skip_prologue (struct gdbarch *gdbarch, > + CORE_ADDR pc) Remove extra spaces, and this can fit on a single line. > +{ > + CORE_ADDR start_pc = 0; > + > + if (find_pc_partial_function (pc, NULL, &start_pc, NULL)) > + { > + CORE_ADDR prologue_end = skip_prologue_using_sal (gdbarch, pc); > + > + if (0 != prologue_end) > + { > + struct symtab_and_line prologue_sal = find_pc_line (start_pc, 0); > + struct compunit_symtab *compunit > + = SYMTAB_COMPUNIT (prologue_sal.symtab); > + const char *debug_format = COMPUNIT_DEBUGFORMAT (compunit); > + > + if ((NULL != debug_format) > + && (strlen ("dwarf") <= strlen (debug_format)) > + && (0 == strncasecmp ("dwarf", debug_format, strlen ("dwarf")))) > + return (prologue_end > pc) ? prologue_end : pc; > + } > + } > + > + fprintf_unfiltered (gdb_stdlog, "%s:%d %s FAILED TO FIND END OF PROLOGUE PC = %08x\n", __FILE__, __LINE__, > + __FUNCTION__, (unsigned int) pc); Maybe use the warning() function? Also it's probably not necessary to use all caps. To print a CORE_ADDR, use: "%s", paddress (gdbarch, pc); Also, user-visible messages should use _() for i18n. > + > + return 0; > +} > + > +static CORE_ADDR > +s12z_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame) > +{ > + return frame_unwind_register_unsigned (next_frame, REG_P); > +} > + > +/* Implement the unwind_sp gdbarch method. */ This comment is good, can you put a similar for other gdbarch callbacks? > + > +static CORE_ADDR > +s12z_unwind_sp (struct gdbarch *gdbarch, struct frame_info *next_frame) > +{ > + return frame_unwind_register_unsigned (next_frame, REG_S); > +} > + > + > + Remove the extra lines. > +static struct type * > +s12z_register_type (struct gdbarch *gdbarch, int reg_nr) > +{ > + switch (registers[reg_perm[reg_nr]].bytes) > + { > + case 1: > + return builtin_type (gdbarch)->builtin_uint8; > + case 2: > + return builtin_type (gdbarch)->builtin_uint16; > + case 3: > + return builtin_type (gdbarch)->builtin_uint24; > + case 4: > + return builtin_type (gdbarch)->builtin_uint32; > + default: > + return builtin_type (gdbarch)->builtin_uint32; > + } > + return builtin_type (gdbarch)->builtin_int0; > +} > + > + > +static int > +s12z_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int num) > +{ > + switch (num) > + { > + case 15: return REG_S; > + case 7: return REG_X; > + case 8: return REG_Y; > + case 42: return REG_D0; > + case 43: return REG_D1; > + case 44: return REG_D2; > + case 45: return REG_D3; > + case 46: return REG_D4; > + case 47: return REG_D5; > + case 48: return REG_D6; > + case 49: return REG_D7; > + } > + return -1; > +} > + > + > +/* Support functions for frame handling. */ > + > +/* Initialize a prologue cache */ > + > +volatile int frame_size = 0; Does this really need to be global? And volatile? > + > +static struct trad_frame_cache * > +s12z_frame_cache (struct frame_info *this_frame, void **prologue_cache) > +{ > + struct trad_frame_cache *info; > + > + CORE_ADDR this_sp; > + CORE_ADDR this_sp_for_id; > + > + CORE_ADDR start_addr; > + CORE_ADDR end_addr; > + > + /* Nothing to do if we already have this info. */ > + if (NULL != *prologue_cache) > + return (struct trad_frame_cache *) *prologue_cache; > + > + /* Get a new prologue cache and populate it with default values. */ > + info = trad_frame_cache_zalloc (this_frame); > + *prologue_cache = info; > + > + /* Find the start address of this function (which is a normal frame, even > + if the next frame is the sentinel frame) and the end of its prologue. */ > + CORE_ADDR this_pc = get_frame_pc (this_frame); > + struct gdbarch *gdbarch = get_frame_arch (this_frame); > + find_pc_partial_function (this_pc, NULL, &start_addr, NULL); Check the return value? > + > + /* Get the stack pointer if we have one (if there's no process executing > + yet we won't have a frame. */ > + this_sp = (NULL == this_frame) ? 0 : > + get_frame_register_unsigned (this_frame, REG_S); > + > + /* Return early if GDB couldn't find the function. */ > + if (start_addr == 0) > + { > + fprintf_unfiltered (gdb_stdlog, " couldn't find function\n"); Use warning (_()) ? > + > + /* JPB: 28-Apr-11. This is a temporary patch, to get round GDB > + crashing right at the beginning. Build the frame ID as best we > + can. */ > + trad_frame_set_id (info, frame_id_build (this_sp, this_pc)); > + > + return info; > + } > + > + /* The default frame base of this frame (for ID purposes only - frame > + base is an overloaded term) is its stack pointer. For now we use the > + value of the SP register in this frame. However if the PC is in the > + prologue of this frame, before the SP has been set up, then the value > + will actually be that of the prev frame, and we'll need to adjust it > + later. */ > + trad_frame_set_this_base (info, this_sp); > + this_sp_for_id = this_sp; > + > + /* The default is to find the PC of the previous frame in the link > + register of this frame. This may be changed if we find the link > + register was saved on the stack. */ > + // trad_frame_set_reg_realreg (info, S12Z_NPC_REGNUM, S12Z_LR_REGNUM); Why is this commented? Either it should be used or it should not be there. > + > + /* We should only examine code that is in the prologue. This is all code > + up to (but not including) end_addr. We should only populate the cache > + while the address is up to (but not including) the PC or end_addr, > + whichever is first. */ > + end_addr = s12z_skip_prologue (gdbarch, start_addr); > + > + /* All the following analysis only occurs if we are in the prologue and > + have executed the code. Check we have a sane prologue size, and if > + zero we are frameless and can give up here. */ > + if (end_addr < start_addr) > + error (_("end addr %s is less than start addr %s"), > + paddress (gdbarch, end_addr), paddress (gdbarch, start_addr)); > + > + if (end_addr == start_addr) > + { > + frame_size = 0; > + } Remove braces. > + else > + { > + CORE_ADDR addr = start_addr; /* Where we have got to */ > + > + gdb_byte byte; > + > + if (0 != target_read_code (addr, &byte, 1)) > + memory_error (TARGET_XFER_E_IO, addr); I think if you call read_code, it throws memory_error for you on failure. > + > + int simm; > + if (byte == 0x1a) /* LEA S, (S, xxxx) */ > + { > + if (0 != target_read_code (addr + 1, &byte, 1)) > + memory_error (TARGET_XFER_E_IO, addr); Same. > + > + simm = (signed char) byte; > + frame_size = -simm; > + addr += 2; > + > + /* If the PC has not actually got to this point, then the frame > + base will be wrong, and we adjust it. > + > + If we are past this point, then we need to populate the stack > + accordingly. */ > + if (this_pc <= addr) > + { > + /* Only do if executing. */ > + if (0 != this_sp) > + { > + this_sp_for_id = this_sp + frame_size; > + trad_frame_set_this_base (info, this_sp_for_id); > + } > + } > + else > + { > + /* We are past this point, so the stack pointer of the prev > + frame is frame_size greater than the stack pointer of this > + frame. */ > + trad_frame_set_reg_value (info, REG_S, > + this_sp + frame_size + 3); > + } > + } > + > + /* From now on we are only populating the cache, so we stop once we > + get to either the end OR the current PC. */ > + // end_addr = (this_pc < end_addr) ? this_pc : end_addr; > + > + > + trad_frame_set_reg_addr (info, REG_P, this_sp + frame_size); > + } > + > + /* Build the frame ID */ > + trad_frame_set_id (info, frame_id_build (this_sp_for_id, start_addr)); > + > + return info; > +} > + > +/* Implement the this_id function for the stub unwinder. */ > + > +static void > +s12z_frame_this_id (struct frame_info *this_frame, > + void **prologue_cache, struct frame_id *this_id) > +{ > + struct trad_frame_cache *info = s12z_frame_cache (this_frame, > + prologue_cache); > + > + trad_frame_get_id (info, this_id); > +} > + > +/* Implement the prev_register function for the stub unwinder. */ > + > +static struct value * > +s12z_frame_prev_register (struct frame_info *this_frame, > + void **prologue_cache, int regnum) > +{ > + struct trad_frame_cache *info = s12z_frame_cache (this_frame, > + prologue_cache); > + > + return trad_frame_get_register (info, this_frame, regnum); > +} > + > +/* Data structures for the normal prologue-analysis-based unwinder. */ > + > +static const struct frame_unwind s12z_frame_unwind = { > + NORMAL_FRAME, > + default_frame_unwind_stop_reason, > + s12z_frame_this_id, > + s12z_frame_prev_register, > + NULL, > + default_frame_sniffer, > + NULL, > +}; > + > + > +constexpr gdb_byte s12z_break_insn[] = {0x00}; > + > +typedef BP_MANIPULATION (s12z_break_insn) s12z_breakpoint; > + > +struct gdbarch_tdep > +{ > +}; > + > +static struct gdbarch * > +s12z_gdbarch_init (struct gdbarch_info info, > + struct gdbarch_list *arches) The indentation here looks wrongs (should use tabs for groups of 8 columns and then complete with spaces, and there is one extra space). > +{ > + struct gdbarch_tdep *tdep = (struct gdbarch_tdep *) xmalloc (sizeof *tdep); You could use XNEW (gdbarch_tdep), which has the advantage of causing a build failure if it's unsafe to malloc a structure of this type. > + struct gdbarch *gdbarch = gdbarch_alloc (&info, tdep); > + > + /* Target data types. */ > + set_gdbarch_short_bit (gdbarch, 16); > + set_gdbarch_int_bit (gdbarch, 16); > + set_gdbarch_long_bit (gdbarch, 32); > + set_gdbarch_long_long_bit (gdbarch, 32); > + set_gdbarch_ptr_bit (gdbarch, 24); > + set_gdbarch_addr_bit (gdbarch, 24); > + set_gdbarch_char_signed (gdbarch, 0); > + > + set_gdbarch_ps_regnum (gdbarch, REG_CCW); > + set_gdbarch_pc_regnum (gdbarch, REG_P); > + set_gdbarch_sp_regnum (gdbarch, REG_S); > + > + > + set_gdbarch_breakpoint_kind_from_pc (gdbarch, > + s12z_breakpoint::kind_from_pc); > + set_gdbarch_sw_breakpoint_from_kind (gdbarch, > + s12z_breakpoint::bp_from_kind); > + > + set_gdbarch_num_regs (gdbarch, S12Z_N_REGISTERS - 2); > + set_gdbarch_register_name (gdbarch, s12z_register_name); > + set_gdbarch_skip_prologue (gdbarch, s12z_skip_prologue); > + set_gdbarch_inner_than (gdbarch, core_addr_lessthan); > + set_gdbarch_dwarf2_reg_to_regnum (gdbarch, s12z_dwarf_reg_to_regnum); > + > + set_gdbarch_register_type (gdbarch, s12z_register_type); > + > + /* Functions to access frame data. */ > + set_gdbarch_unwind_pc (gdbarch, s12z_unwind_pc); > + set_gdbarch_unwind_sp (gdbarch, s12z_unwind_sp); > + > + dwarf2_append_unwinders (gdbarch); > + frame_unwind_append_unwinder (gdbarch, &s12z_frame_unwind); > + > + return gdbarch; > +} > + > +void > +_initialize_s12z_tdep (void) > +{ > + gdbarch_register (bfd_arch_s12z, s12z_gdbarch_init, NULL); > +} > Thanks, Simon