From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2551 invoked by alias); 13 Jan 2013 06:48:32 -0000 Received: (qmail 2540 invoked by uid 22791); 13 Jan 2013 06:48:30 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO,TW_CP 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, 13 Jan 2013 06:48:25 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 1F13A2E449; Sun, 13 Jan 2013 01:48:25 -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 u0ByMy7aPt-q; Sun, 13 Jan 2013 01:48:25 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 7F98E2E274; Sun, 13 Jan 2013 01:48:24 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 0E7D0C162D; Sun, 13 Jan 2013 10:48:17 +0400 (RET) Date: Sun, 13 Jan 2013 06:48:00 -0000 From: Joel Brobecker To: Marcus Shawcroft Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCH 1/5] aarch64-tdep basic port. Message-ID: <20130113064816.GA7262@adacore.com> References: <50AD0303.5030100@arm.com> <50BCD288.4030109@arm.com> <20121223063407.GK5370@adacore.com> <50EAD9FF.4040303@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50EAD9FF.4040303@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: 2013-01/txt/msg00254.txt.bz2 > 2013-01-07 Jim MacArthur > Marcus Shawcroft > Nigel Stephens > Yufeng Zhang > > * Makefile.in: Add AArch64. > * aarch64-tdep.c: New file. > * aarch64-tdep.h: New file. > * 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). A few more minor comments, and I expect the next version of this patch to be good to go. > +/* Decode an opcode if it represents an immediate ADD or SUB instruction. > + > + ADDR specifies the address of the opcode. > + INSN specifies the opcode to test. > + RD receives the 'rd' field from the decoded instruction. > + RN receives the 'rn' field from the decoded instruction. > + > + Return 1 if the opcodes matches and is decoded, otherwise 0. > + */ This is a bit of a nit-pick, but since there are going to be other modificasions, the closing "*/" is typically placed at the end of the comment, rather than on the next line (unless there is not enough room to do so). Can you adjust all function descriptions accordingly? > + /* See if we can determine the end of the prologue via the symbol > + table. If so, then return either PC, or the PC after the > + prologue, whichever is greater. */ > + if (find_pc_partial_function (pc, NULL, &func_addr, NULL)) > + { > + CORE_ADDR post_prologue_pc = skip_prologue_using_sal (gdbarch, func_addr); This line is too long. Can you reformat it as follow? CORE_ADDR post_prologue_pc = skip_prologue_using_sal (gdbarch, func_addr); > + if (sal.line == 0) > + /* No line info so use the current PC. */ > + prologue_end = prev_pc; > + else if (sal.end < prologue_end) > + /* The next line begins after the function end. */ > + prologue_end = sal.end; Because you moved the block inside the if/else section, it visually looks as if there was 2 statements. We've decided for the GDB project that we would use curly braces in this case, even if they are actually superfluous. Thus: if (sal.line == 0) { /* No line info so use the current PC. */ prologue_end = prev_pc; } else if (sal.end < prologue_end) { /* The next line begins after the function end. */ prologue_end = sal.end; } > + member0_type = check_typedef (TYPE_FIELD_TYPE (ty, 0)); > + if (TYPE_CODE (member0_type) == TYPE_CODE_FLT) > + { > + int i; > + for (i = 0; i < TYPE_NFIELDS (ty); i++) Missing empty line after variable declarations... > + /* 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. > + 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); Missing empty line after variable declaration... > + /* Now we have tuned the configuration, set a few final things, > + based on what the OS ABI has told us. */ > + > + if (tdep->jb_pc >= 0) > + set_gdbarch_get_longjmp_target (gdbarch, aarch64_get_longjmp_target); Have you answered the comment I made in my first review? I can't find anything... | 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. > + /* Debug this file's internals. */ > + add_setshow_zinteger_cmd ("aarch64", class_maintenance, &aarch64_debug, _("\ This is something that I missed in the first review, because relatively recent, but we've decided that it was friendlier to use a boolean, instead of a zinteger. If you eventually need to use more than 2 states, it's pretty easy to upgrade. > diff --git a/gdb/features/aarch64-core.xml b/gdb/features/aarch64-core.xml > new file mode 100644 > index 0000000..e1e9dc3 > --- /dev/null > +++ b/gdb/features/aarch64-core.xml > @@ -0,0 +1,46 @@ > + > +