From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3802 invoked by alias); 11 Jul 2014 14:05:38 -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 3750 invoked by uid 89); 11 Jul 2014 14:05:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: na01-bn1-obe.outbound.protection.outlook.com Received: from mail-bn1lp0140.outbound.protection.outlook.com (HELO na01-bn1-obe.outbound.protection.outlook.com) (207.46.163.140) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 11 Jul 2014 14:05:29 +0000 Received: from BN1AFFO11FD007.protection.gbl (10.58.52.31) by BN1AFFO11HUB064.protection.gbl (10.58.52.215) with Microsoft SMTP Server (TLS) id 15.0.980.11; Fri, 11 Jul 2014 14:05:26 +0000 Received: from xsj-pvapsmtpgw01 (149.199.60.83) by BN1AFFO11FD007.mail.protection.outlook.com (10.58.52.67) with Microsoft SMTP Server (TLS) id 15.0.980.11 via Frontend Transport; Fri, 11 Jul 2014 14:05:26 +0000 Received: from unknown-38-66.xilinx.com ([149.199.38.66] helo=xsj-smtp1) by xsj-pvapsmtpgw01 with esmtp (Exim 4.63) (envelope-from ) id 1X5bRc-0001q8-7x; Fri, 11 Jul 2014 07:05:00 -0700 From: Ajit Kumar Agarwal To: Joel Brobecker CC: "gdb-patches@sourceware.org" , Michael Eager , Pedro Alves , Vinod Kathail , Vidhumouli Hunsigida , "Nagaraju Mekala" Subject: RE: [Patch, microblaze]: Add support of microblaze software single stepping Date: Fri, 11 Jul 2014 14:42:00 -0000 References: <20140707145634.GF6038@adacore.com> <35bccb81-cae6-4581-b5fd-16dea7171d28@BN1AFFO11FD012.protection.gbl> <20140711135114.GA4888@adacore.com> In-Reply-To: <20140711135114.GA4888@adacore.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-RCIS-Action: ALLOW Message-ID: <309ac1fe-8dab-4f96-ad26-75e9169d8537@BN1AFFO11FD007.protection.gbl> X-EOPAttributedMessage: 0 X-Forefront-Antispam-Report: CIP:149.199.60.83;CTRY:US;IPV:NLI;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(6009001)(438002)(377424004)(51704005)(189002)(199002)(377454003)(13464003)(23726002)(83322001)(85306003)(107046002)(83072002)(110136001)(44976005)(21056001)(77096002)(106116001)(53416004)(106466001)(93886003)(54356999)(76176999)(50466002)(1496007)(95666004)(74662001)(97756001)(92726001)(31966008)(92566001)(47776003)(86362001)(6806004)(81342001)(46102001)(99396002)(85852003)(50986999)(74316001)(87936001)(33646001)(20776003)(74502001)(80022001)(70736001)(31696002)(79102001)(19580395003)(77982001)(46406003)(104016003)(4396001)(81542001)(64706001)(2656002)(76482001)(19580405001)(107986001);DIR:OUT;SFP:;SCL:1;SRVR:BN1AFFO11HUB064;H:xsj-pvapsmtpgw01;FPR:;MLV:sfv;PTR:unknown-60-83.xilinx.com;MX:1;A:1;LANG:en; X-OriginatorOrg: xilinx.onmicrosoft.com X-Microsoft-Antispam: BCL:0;PCL:0;RULEID: X-Forefront-PRVS: 02698DF457 Received-SPF: Pass (: domain of xilinx.com designates 149.199.60.83 as permitted sender) receiver=; client-ip=149.199.60.83; helo=xsj-pvapsmtpgw01; Authentication-Results: spf=pass (sender IP is 149.199.60.83) smtp.mailfrom=ajit.kumar.agarwal@xilinx.com; X-SW-Source: 2014-07/txt/msg00271.txt.bz2 Hello Joel: I will incorporate the formatting changes as you have mentioned. -----Original Message----- From: Joel Brobecker [mailto:brobecker@adacore.com]=20 Sent: Friday, July 11, 2014 7:21 PM To: Ajit Kumar Agarwal Cc: gdb-patches@sourceware.org; Michael Eager; Pedro Alves; Vinod Kathail; = Vidhumouli Hunsigida; Nagaraju Mekala Subject: Re: [Patch, microblaze]: Add support of microblaze software single= stepping > Based on the feedback review comments are incorporated.=20 > ChangeLog: > 2014-07-11 Ajit Agarwal >=20 > * microblaze-tdep.c (microblaze_software_single_step): New. > (microblaze_gdbarch_init): Use of set_gdbarch_software_single_step. Thank you. Some additional trivial coments below. >>Please also explain how this patch was tested. The changes were tested with the application for barematel with remote debu= g with XMD debugger( internal to Xilinx)which connects to the target and op= ens the gdbserver connection. Single stepping command were used for next_pc= in straight line code, with imm instruction and the branch with delay Slot. > +/* This function handles software single step, branches with delay slot > + imm instruction in microblaze. */ static int=20 > +microblaze_software_single_step (struct frame_info *frame) Two comments, in this case: My first comment is that the GDB coding style r= equires a space after the comment describing the function, before the funct= ion itself starts. The second is that functions meant to be installed as gdbarch hooks should = be documented as such. The theory is that the purpose of these functions, a= s well as their API, is documented in the gdbarch code, and thus does not n= eed to be repeated here. So, the typical function description for these fun= ctions is: /* Implement the software_single_step gdbarch method. */ If you feel that the extra information you had above is useful, just add it= after the introductory comment, like so, for instance: /* Implement the software_single_step gdbarch method. This function handles software single step, branches with delay slot imm instruction in microblaze. */ > + /* Set a breakpoint at the next instruction. If the current > + instruction is an imm, set it at the inst after. If the > + instruction has a delay slot, skip the delay slot. */ This comment could also be merged with the comment above. Or, if you prefe= r, the extra info from the function documentation could also be merged with= this comment. Formatting note: 2 spaces after periods. > + pc =3D get_frame_pc (frame); > +=20 > + insn =3D microblaze_fetch_instruction (pc); > + > + minstr =3D get_insn_microblaze (insn, > + &isunsignednum, > + &insn_type, > + &delay_slots); I will leave it to you to decide, but we typically do not write function ca= lls with one argument per line, unless perhaps when the arguments are funct= ion calls, where having one per line would help the reader find which argum= ents belong to which function call. Particularly in the case below.... > + minstr =3D microblaze_decode_insn (insn, > + &rd, > + &ra, > + &rb, > + &imm); ... joining all arguments on the same line would save you a lot of screen r= eal estate. > + if (insn_type !=3D return_inst) > + breaks[0] =3D pc + delay_slots * INST_WORD_SIZE + INST_WORD_SIZE; > +=20=20=20=20 Trailing spaces here. > + bfd_boolean targetvalid; > + bfd_boolean unconditionalbranch; > +=20 And here. > + if (lrb >=3D 0 && lrb < MICROBLAZE_NUM_REGS) > + rb =3D get_frame_register_unsigned (frame, lrb); > + else > + rb =3D 0; > +=20 And here. > + address =3D microblaze_get_target_address (insn, > + immfound, > + imm, > + pc, > + ra, > + rb, > + &targetvalid, > + &unconditionalbranch); > +=20=20=20=20=20=20=20=20 > + if (!unconditionalbranch) > + breaks[1] =3D address; > + } > +=20=20=20=20 And there (3 lines have trailing spaces) Thank you, -- Joel