From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20255 invoked by alias); 23 Dec 2012 06:34:28 -0000 Received: (qmail 19954 invoked by uid 22791); 23 Dec 2012 06:34:25 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO,TW_CP,TW_YM X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 23 Dec 2012 06:34:16 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 757471C7E68; Sun, 23 Dec 2012 01:34:15 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id s-ffrvoPsNKX; Sun, 23 Dec 2012 01:34:15 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id A83B01C7DDE; Sun, 23 Dec 2012 01:34:14 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 54C32C255C; Sun, 23 Dec 2012 10:34:07 +0400 (RET) Date: Sun, 23 Dec 2012 06:34:00 -0000 From: Joel Brobecker To: Marcus Shawcroft Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCH 1/5] AArch64 GDB and GDBSERVER Port V2 Message-ID: <20121223063407.GK5370@adacore.com> References: <50AD0303.5030100@arm.com> <50BCD288.4030109@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50BCD288.4030109@arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) 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 X-SW-Source: 2012-12/txt/msg00785.txt.bz2 Marcus, I sincerely apologize for the delay in getting this reviewed. Several of us seem to have had a busy end of year, and it is not easy to find a slot of quiet time long enough to review such large patches. > 2012-11-21 Jim MacArthur > Marcus Shawcroft > Nigel Stephens > Yufeng Zhang > > * Makefile.in: Add AArch64. > * aarch64-tdep.c: New file. > * aarch64-tdep.h: New file. > * configure.host: Add AArch64. > * configure.tgt: Add AArch64. > * features/Makefile: Add AArch64. > * features/aarch64-core.xml: New file. > * features/aarch64-fpu.xml: New file. > * features/aarch64-without-fpu.c: New file (generated). > * features/aarch64-without-fpu.xml: New file. > * features/aarch64.c: New file (generated). > * features/aarch64.xml: New file. > * regformats/aarch64-without-fpu.dat: New file (generated). > * regformats/aarch64.dat: New file (generated). See my comments below. Nice code overall, just many many little nits that should be very easy to fix. A few important questions at the end, as well. A request: Please fix the subject of each of your patches so that it actually tells us what the objective of the patch is. I kept getting confused while trying to refer to patch numbers while writing this review... I will take a look at the rest of the GDB patches knowing that a second review will be needed anyways, because some of the fixes requested here will mean a change in those other patches. > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index 3dd7b85..b1bed86 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -517,6 +517,7 @@ TARGET_OBS = @TARGET_OBS@ > # All target-dependent objects files that require 64-bit CORE_ADDR > # (used with --enable-targets=all --enable-64-bit-bfd). > ALL_64_TARGET_OBS = \ > + aarch64-linux-tdep.o aarch64-newlib-tdep.o aarch64-tdep.o \ This change should only add aarch64-tdep.o to the list. > ALLDEPFILES = \ > + aarch64-linux-nat.c \ > + aarch64-linux-tdep.c aarch64-newlib-tdep.c aarch64-tdep.c \ Same here. > +static int32_t > +extract_signed_bitfield (uint32_t insn, unsigned width, unsigned offset) Can you add a comment documenting the function, please? There are several other functions that need the same treatment. Please make sure to document the various arguments. > +static int > +decode_add_sub_imm (CORE_ADDR addr, uint32_t insn, unsigned *rd, unsigned *rn, > + int32_t * imm) > +{ > + if ((insn & 0x9f000000) == 0x91000000) Can you add a small comment explaining what instructions this check matches? > + if (decode_masked_match (insn, 0x9f000000, 0x90000000)) Same here; although perhaps the function description might be sufficient to make this check obvious. > +static CORE_ADDR > +aarch64_analyze_prologue (struct gdbarch *gdbarch, [...] > + pv_t regs[32]; I tend to try to avoid litteral constants like these. Can we create a named constant, or at worst a macro, which names this value? It makes it more obvious what it is about; and also avoids repeating this constant later, for instance: > + for (i = 0; i < 32; i++) > + regs[i] = pv_register (i, 0); > + /* If recording this store would invalidate the store area > + (perhaps because rn is not known) then we should abandon > + further prologue analysis. */ > + if (pv_area_store_would_trash (stack, > + pv_add_constant (regs[rn], imm)) || > + pv_area_store_would_trash (stack, > + pv_add_constant (regs[rn], imm + 8))) The binary opeator "||" should be at the start of the next line, rather than at the end. Alternatively, you can write this as two conditions. For instance if (pv_area_store_would_trash (stack, pv_add_constant (regs[rn], imm))) break; if (pv_area_store_would_trash (stack, pv_add_constant (regs[rn], imm + 8))) break; > + for (i = 0; i < 32; i++) > + { > + CORE_ADDR offset; > + if (pv_area_find_reg (stack, gdbarch, i, &offset)) Can you add an empty line after the variable declaration? > + if (find_pc_partial_function (pc, NULL, &func_addr, NULL)) > + { > + CORE_ADDR post_prologue_pc = > + skip_prologue_using_sal (gdbarch, func_addr); Same here... > +/* Called by aarch64_make_prologue_cache only. */ > + > +static void > +aarch64_scan_prologue (struct frame_info *this_frame, > + struct aarch64_prologue_cache *cache) > +{ Can you document what the function does, rather than who calls it? > + if (sal.line == 0) /* no line info, use current PC */ /* No line info, use the current PC. */ > + prologue_end = prev_pc; > + else if (sal.end < prologue_end) /* next line begins after fn end */ Same as above (sentences start with capital letters and end with a period). > + if (prev_regnum == AARCH64_PC_REGNUM) > + { > + CORE_ADDR lr; > + lr = frame_unwind_register_unsigned (this_frame, AARCH64_LR_REGNUM); Missing empty line after variable declaration. > + /* SP is generally not saved to the stack, but this frame is > + identified by the next frame's stack pointer at the time of the > + call. The value was already reconstructed into PREV_SP. */ > + /* > + * +----------+ ^ > + * | saved lr | | > + * +->| saved fp |--+ > + * | | | > + * | | | <- Previous SP > + * | +----------+ > + * | | saved lr | > + * +--| saved fp |<- FP > + * | | > + * | |<- SP > + * +----------+ > + */ Can you remove the leading '*' in your comment above? > +/* AArch64 default frame base information. */ > +struct frame_base aarch64_normal_base = { Curly brace on the next line... > +static CORE_ADDR > +aarch64_unwind_pc (struct gdbarch *gdbarch, struct frame_info *this_frame) > +{ > + CORE_ADDR pc = frame_unwind_register_unsigned (this_frame, AARCH64_PC_REGNUM); This line is too long. > +/* When arguments must be pushed onto the stack, they go on in reverse > + order. The code below implements a FILO (stack) to do this. */ > + > +struct stack_item > +{ > + int len; > + struct stack_item *prev; > + void *data; > +}; > + > +static struct stack_item * > +push_stack_item (struct stack_item *prev, const bfd_byte *contents, int len) [etc] Why not use a VEC, here? Also, at a minimum, the various fields in your struct stack_item need to be documented individually. > + if (TYPE_NFIELDS (ty) > 0 && TYPE_NFIELDS (ty) <= 4) > + { > + struct type *member0_type; > + member0_type = check_typedef (TYPE_FIELD_TYPE (ty, 0)); Empty line after variable declaration... > + if (TYPE_CODE (member0_type) == TYPE_CODE_FLT) > + { > + int i; > + for (i = 0; i < TYPE_NFIELDS (ty); i++) Same here... > + { > + struct type *member1_type; > + member1_type = check_typedef (TYPE_FIELD_TYPE (ty, i)); And there... > +/* AArch64 function call information structure. */ > +struct aarch64_call_info > +{ > + unsigned argnum; > + unsigned ncrn; > + unsigned nvrn; > + unsigned nsaa; > + struct stack_item *si; > +}; Can you document each field individually, please? > + if (aarch64_debug) > + fprintf_unfiltered (gdb_stdlog, "arg %d in %s = 0x%s\n", ^^^^^^^^^^ > + info->argnum, > + gdbarch_register_name (gdbarch, regnum), > + phex (regval, X_REGISTER_SIZE)); > + fprintf_unfiltered (gdb_stdlog, "arg %d in %s\n", ^^^^^^^^^^ > + info->argnum, > + gdbarch_register_name (gdbarch, regnum)); > + if (aarch64_debug) > + fprintf_unfiltered (gdb_stdlog, "arg %d len=%d @ sp + %d\n", ^^^^^^^^^^ > + info->argnum, len, info->nsaa); Note: I am confused now as to whether those debug traces should be printed on stdlog or stderr. I thought it was stderr, but I see stdlog used everywhere for this purpose. Your code uses either, but I am thinking now that it should be gdb_stdlog. Can you adjust your code to consistently use the same output? > + /* Push stack alignment padding. */ > + int pad = align - (info->nsaa & (align - 1)); > + info->si = push_stack_item (info->si, buf, pad); Empty line after variable declaration... > + gdb_assert (TYPE_CODE (func_type) == TYPE_CODE_FUNC || > + TYPE_CODE (func_type) == TYPE_CODE_METHOD); "||" at start of next line. > + /* If we were given an initial argument for the return slot because > + lang_struct_return was true. Lose it. */ ^^^ I'd use a comma, here. > + { > + const bfd_byte *buf = value_contents (arg); > + struct type *target_type = > + check_typedef (TYPE_TARGET_TYPE (arg_type)); > + pass_in_v (gdbarch, regcache, &info, buf); Empty line after variable declaration... > + int elements = TYPE_NFIELDS (arg_type); > + /* Homogeneous Aggregates */ Same here... > + if (info.nvrn + elements < 8) > + { > + int i; > + for (i = 0; i < elements; i++) And here... > + { > + /* We know that we have sufficient registers > + available therefore this will never fallback > + to the stack. */ > + struct value *field = > + value_primitive_field (arg, 0, i, arg_type); > + struct type *field_type = > + check_typedef (value_type (field)); > + pass_in_v_or_stack (gdbarch, regcache, &info, field_type, > + value_contents_writeable (field)); And here... > +static int > +aarch64_gdb_print_insn (bfd_vma memaddr, disassemble_info * info) ^^^ No space between '*' and 'info'. > +static const char aarch64_default_breakpoint[] = {0x00,0x00,0x20,0xd4}; Missing spaces after comas. > + bfd_byte buf[V_REGISTER_SIZE]; > + int len = TYPE_LENGTH (type); > + regcache_cooked_read (regs, AARCH64_V0_REGNUM, buf); Empty line after variable declarations... > + else if (TYPE_CODE (type) == TYPE_CODE_COMPLEX) > + { > + int regno = AARCH64_V0_REGNUM; > + bfd_byte buf[V_REGISTER_SIZE]; > + struct type *target_type = check_typedef (TYPE_TARGET_TYPE (type)); > + int len = TYPE_LENGTH (target_type); > + regcache_cooked_read (regs, regno, buf); Same. > + if (TYPE_CODE (type) == TYPE_CODE_FLT) > + { > + bfd_byte buf[V_REGISTER_SIZE]; > + int len = TYPE_LENGTH (type); > + memcpy (buf, valbuf, len > V_REGISTER_SIZE ? V_REGISTER_SIZE : len); Same. > +/* Implement the "pseudo_register_reggroup_p" tdesc_arch_data method. */ > + > +static int > +aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum, > + struct reggroup *group) > +{ > + regnum -= gdbarch_num_regs (gdbarch); > + > + if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32) > + return group == all_reggroup || group == vector_reggroup; > + else if (regnum >= AARCH64_D0_REGNUM && regnum < AARCH64_D0_REGNUM + 32) > + return group == all_reggroup || group == vector_reggroup > + || group == float_reggroup; > + else if (regnum >= AARCH64_S0_REGNUM && regnum < AARCH64_S0_REGNUM + 32) > + return group == all_reggroup || group == vector_reggroup > + || group == float_reggroup; > + else if (regnum >= AARCH64_H0_REGNUM && regnum < AARCH64_H0_REGNUM + 32) > + return group == all_reggroup || group == vector_reggroup; > + else if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32) > + return group == all_reggroup || group == vector_reggroup; For multi-line return expressions, the GNU Coding Standards require that the expression be enclosed between parentheses. The parentheses are strictly unnecessary, but help code formatters. Thus, for instance: return (group == all_reggroup || group == vector_reggroup || group == float_reggroup); > + gdb_assert (0); Use gdb_assert_not_reached. > + /* Ensure the register buffer is zero, we want gdb writes of the > + various 'scalar' pseudo registers to behavior like architectural ^^^^^^^^^^^ to behave? > + writes, register width bytes are written the remainder are set to writes: Register width bytes are written, and the remainder [...] > + zero. */ > + memset (reg_buf, 0, sizeof (reg_buf)); > + gdb_assert (0); Use gdb_assert_not_reached > + if (tdep->jb_pc >= 0) > + set_gdbarch_get_longjmp_target (gdbarch, aarch64_get_longjmp_target); This code looks useless AFAICT, as tdep->jb_pc is set to -1 earlier in this function, and I don't think anything changes its value before this code. > +void > +_initialize_aarch64_tdep (void) > +{ > + struct cmd_list_element *new_set, *new_show; > + const char *setname; > + const char *setdesc; Can you delete those unused variables? > +/* Register numbers of various important registers. */ > +enum gdb_regnum > +{ > + AARCH64_X0_REGNUM, /* First integer register */ I would prefer it if you renamed this enum into something less generic. For instance: enum aarch64_regnum. > +/* Size of integer registers. */ > +#define X_REGISTER_SIZE 8 > +#define B_REGISTER_SIZE 1 > +#define H_REGISTER_SIZE 2 > +#define S_REGISTER_SIZE 4 > +#define D_REGISTER_SIZE 8 > +#define V_REGISTER_SIZE 16 > +#define Q_REGISTER_SIZE 16 Alignment issue if first macro. > +/* Target-dependent structure in gdbarch. */ > +struct gdbarch_tdep > +{ > + /* Lowest address at which instructions will appear. */ > + CORE_ADDR lowest_pc; Why put this in the tdep vector if it is a constant? I checked the other patches, and they do not reference this field at all. > + /* Breakpoint pattern for an AArch64 insn. */ > + const char *aarch64_breakpoint; > + /* And its size. */ > + int aarch64_breakpoint_size; Same here... > + /* Cached core file helpers. */ > + struct regset *gregset; > + struct regset *fpregset; These are unused in this patch. It's a bit of a pain, but can you please remove them from this patch, and add it to the patch that really uses them? > diff --git a/gdb/configure.host b/gdb/configure.host > index 7dc35e1..c5a7a3e 100644 This diff should be part of the patch that introduces native support to GDB (ie patches #3 and #4). > diff --git a/gdb/configure.tgt b/gdb/configure.tgt > index 36d4304..63fd4b0 100644 > --- a/gdb/configure.tgt > +++ b/gdb/configure.tgt > @@ -31,6 +31,18 @@ esac > # map target info into gdb names. > > case "${targ}" in > +aarch64*-*-elf) > + # Target: AArch64 embedded system > + gdb_target_obs="aarch64-tdep.o aarch64-newlib-tdep.o" > + ;; > + > +aarch64*-*-linux*) > + # Target: AArch64 linux > + gdb_target_obs="aarch64-tdep.o aarch64-linux-tdep.o \ > + glibc-tdep.o linux-tdep.o solib-svr4.o \ > + symfile-mem.o" > + build_gdbserver=yes > + ;; Similarly, the part that handles aarch64*-*-linux* should be moved to patch #2 where it introduces aarch64-linux target support. I am also confused as to the relationship between this code and the "newlib" one. From what I can tell, there is no way to configure GDB without the newlib code. However, that code seems to be unused in practice (because no sniffer seems to be returning GDB_OSABI_NEWLIB). Either: (1) The newlib code is an integral part of the bare-metal aarch64 code, in which case I think it should be part of this patch. But then how is it activated (I may be missing something); (2) The newlib code is not necessary, in which case aarch64-newlib-tdep.o needs to be removed from gdb_target_obs. -- Joel