From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21532 invoked by alias); 11 Jul 2014 19:34:48 -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 21468 invoked by uid 89); 11 Jul 2014 19:34:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: na01-bl2-obe.outbound.protection.outlook.com Received: from mail-bl2lp0212.outbound.protection.outlook.com (HELO na01-bl2-obe.outbound.protection.outlook.com) (207.46.163.212) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 11 Jul 2014 19:34:38 +0000 Received: from BY2FFO11FD002.protection.gbl (10.1.14.33) by BY2FFO11HUB056.protection.gbl (10.1.15.231) with Microsoft SMTP Server (TLS) id 15.0.980.11; Fri, 11 Jul 2014 19:34:22 +0000 Received: from xsj-pvapsmtpgw01 (149.199.60.83) by BY2FFO11FD002.mail.protection.outlook.com (10.1.14.124) with Microsoft SMTP Server (TLS) id 15.0.980.11 via Frontend Transport; Fri, 11 Jul 2014 19:34:21 +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 1X5gZw-0001Id-2u; Fri, 11 Jul 2014 12:33:56 -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 19:44: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: multipart/mixed; boundary="_002_37378DC5BCD0EE48BA4B082E0B55DFAA40B91915XAPPVEXMBX01xln_" MIME-Version: 1.0 X-RCIS-Action: ALLOW Message-ID: 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)(199002)(189002)(51704005)(377424004)(377454003)(13464003)(64706001)(79102001)(77096002)(80022001)(81542001)(85306003)(44976005)(74316001)(31966008)(74662001)(4396001)(74502001)(53416004)(85852003)(20776003)(512954002)(92566001)(31696002)(21056001)(95666004)(83072002)(76176999)(83322001)(33646001)(568964001)(6806004)(87936001)(106466001)(92726001)(104016003)(70736001)(86362001)(77982001)(76482001)(54356999)(107046002)(99936001)(19580405001)(50986999)(71186001)(2656002)(46102001)(93886003)(106116001)(84326002)(19580395003)(81342001)(1496007)(110136001)(99396002)(107986001);DIR:OUT;SFP:;SCL:1;SRVR:BY2FFO11HUB056;H:xsj-pvapsmtpgw01;FPR:;MLV:sfv;PTR:unknown-60-83.xilinx.com;A:1;MX: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/msg00292.txt.bz2 --_002_37378DC5BCD0EE48BA4B082E0B55DFAA40B91915XAPPVEXMBX01xln_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Content-length: 5132 Hello Joel: Please find the patch with the updated review comments. Please let me know = if the changes looks okay. [Patch, microblaze]: Add support of microblaze software single stepping. This patch adds the support of microblaze software single stepping. It handles the cases of branch and return with delay slot and imm instruction in microblaze. ChangeLog: 2014-07-12 Ajit Agarwal * microblaze-tdep.c (microblaze_software_single_step): New. (microblaze_gdbarch_init): Use of set_gdbarch_software_single_step. Signed-off-by:Ajit Agarwal ajitkum@xilinx.com >>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. Thanks & Regards Ajit -----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. > +/* 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 --_002_37378DC5BCD0EE48BA4B082E0B55DFAA40B91915XAPPVEXMBX01xln_ Content-Type: application/octet-stream; name="0001-Patch-microblaze-Add-support-of-microblaze-software-.patch" Content-Description: 0001-Patch-microblaze-Add-support-of-microblaze-software-.patch Content-Disposition: attachment; filename="0001-Patch-microblaze-Add-support-of-microblaze-software-.patch"; size=4306; creation-date="Thu, 19 Jun 2014 18:16:06 GMT"; modification-date="Fri, 11 Jul 2014 19:27:49 GMT" Content-Transfer-Encoding: base64 Content-length: 5840 RnJvbSAyMWMwYzk1ODFmYWJhNzJiOTgwNjFjNDZhZDA2ZTU4YTc4ZGQ2YjJk IE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBBaml0IEt1bWFyIEFn YXJ3YWwgPGFqaXRrdW1AeGhkc3BkZ251Lihub25lKT4KRGF0ZTogU2F0LCAx MiBKdWwgMjAxNCAwMDo1MzozNSArMDUzMApTdWJqZWN0OiBbUEFUQ0hdIFtQ YXRjaCwgbWljcm9ibGF6ZV06IEFkZCBzdXBwb3J0IG9mIG1pY3JvYmxhemUg c29mdHdhcmUgc2luZ2xlIHN0ZXBwaW5nLgoKVGhpcyBwYXRjaCBhZGRzIHRo ZSBzdXBwb3J0IG9mIG1pY3JvYmxhemUgc29mdHdhcmUgc2luZ2xlIHN0ZXBw aW5nLiBJdApoYW5kbGVzIHRoZSBjYXNlcyBvZiBicmFuY2ggYW5kIHJldHVy biB3aXRoIGRlbGF5IHNsb3QgYW5kIGltbSBpbnN0cnVjdGlvbgppbiBtaWNy b2JsYXplLgoKQ2hhbmdlTG9nOgoyMDE0LTA3LTEyICBBaml0IEFnYXJ3YWwg IDxhaml0a3VtQHhpbGlueC5jb20+CgoJKiBtaWNyb2JsYXplLXRkZXAuYyAo bWljcm9ibGF6ZV9zb2Z0d2FyZV9zaW5nbGVfc3RlcCk6IE5ldy4KCShtaWNy b2JsYXplX2dkYmFyY2hfaW5pdCk6IFVzZSBvZiBzZXRfZ2RiYXJjaF9zb2Z0 d2FyZV9zaW5nbGVfc3RlcC4KClNpZ25lZC1vZmYtYnk6QWppdCBBZ2Fyd2Fs IGFqaXRrdW1AeGlsaW54LmNvbQotLS0KIGdkYi9taWNyb2JsYXplLXRkZXAu YyB8ICAgODcgKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr KysrKysrKysrKysrKwogMSBmaWxlcyBjaGFuZ2VkLCA4NyBpbnNlcnRpb25z KCspLCAwIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL2dkYi9taWNyb2Js YXplLXRkZXAuYyBiL2dkYi9taWNyb2JsYXplLXRkZXAuYwppbmRleCAxNGMx YjUyLi5hMWYyOWQ2IDEwMDY0NAotLS0gYS9nZGIvbWljcm9ibGF6ZS10ZGVw LmMKKysrIGIvZ2RiL21pY3JvYmxhemUtdGRlcC5jCkBAIC02MjgsNiArNjI4 LDkxIEBAIG1pY3JvYmxhemVfc3RhYnNfYXJndW1lbnRfaGFzX2FkZHIgKHN0 cnVjdCBnZGJhcmNoICpnZGJhcmNoLCBzdHJ1Y3QgdHlwZSAqdHlwZSkKICAg cmV0dXJuIChUWVBFX0xFTkdUSCAodHlwZSkgPT0gMTYpOwogfQogCisvKiBJ bXBsZW1lbnQgdGhlIHNvZnR3YXJlX3NpbmdsZV9zdGVwIGdkYmFyY2ggbWV0 aG9kLgorICAgVGhpcyBmdW5jdGlvbiBoYW5kbGVzIHNvZnR3YXJlIHNpbmds ZSBzdGVwLCBicmFuY2hlcyB3aXRoIGRlbGF5IHNsb3QKKyAgIGltbSBpbnN0 cnVjdGlvbiBpbiBtaWNyb2JsYXplLiAgU2V0IGEgYnJlYWtwb2ludCBhdCB0 aGUgbmV4dCBpbnN0cnVjdGlvbi4KKyAgIElmIHRoZSBjdXJyZW50IGluc3Ry dWN0aW9uIGlzIGFuIGltbSwgc2V0IGl0IGF0IHRoZSBpbnN0IGFmdGVyLiAg SWYgdGhlCisgICBpbnN0cnVjdGlvbiBoYXMgYSBkZWxheSBzbG90LCBza2lw IHRoZSBkZWxheSBzbG90LiAgKi8KKworc3RhdGljIGludAorbWljcm9ibGF6 ZV9zb2Z0d2FyZV9zaW5nbGVfc3RlcCAoc3RydWN0IGZyYW1lX2luZm8gKmZy YW1lKQoreworICBzdHJ1Y3QgZ2RiYXJjaCAqYXJjaCA9IGdldF9mcmFtZV9h cmNoIChmcmFtZSk7CisgIHN0cnVjdCBhZGRyZXNzX3NwYWNlICphc3BhY2Ug PSBnZXRfZnJhbWVfYWRkcmVzc19zcGFjZSAoZnJhbWUpOworICBzdHJ1Y3Qg Z2RiYXJjaF90ZGVwICp0ZGVwID0gZ2RiYXJjaF90ZGVwIChhcmNoKTsKKyAg ZW51bSBiZmRfZW5kaWFuIGJ5dGVfb3JkZXIgPSBnZGJhcmNoX2J5dGVfb3Jk ZXIgKGFyY2gpOworICBpbnQgcmV0ID0gMDsKKyAgaW50IGlpOworICBDT1JF X0FERFIgcGM7CisgIGxvbmcgaW5zbjsKKyAgZW51bSBtaWNyb2JsYXplX2lu c3RyIG1pbnN0cjsKKyAgYmZkX2Jvb2xlYW4gaXN1bnNpZ25lZG51bTsKKyAg ZW51bSBtaWNyb2JsYXplX2luc3RyX3R5cGUgaW5zbl90eXBlOworICBzaG9y dCBkZWxheV9zbG90czsKKyAgaW50IGltbTsKKyAgYmZkX2Jvb2xlYW4gaW1t Zm91bmQgPSBGQUxTRTsKKyAgQ09SRV9BRERSIGJyZWFrc1syXSA9IHstMSwt MX07CisgIENPUkVfQUREUiBhZGRyZXNzOworICBpbnQgdGFyZ2V0dmFsaWQ7 CisKKyAgcGMgPSBnZXRfZnJhbWVfcGMgKGZyYW1lKTsKKyAKKyAgaW5zbiA9 IG1pY3JvYmxhemVfZmV0Y2hfaW5zdHJ1Y3Rpb24gKHBjKTsKKworICBtaW5z dHIgPSBnZXRfaW5zbl9taWNyb2JsYXplIChpbnNuLCAmaXN1bnNpZ25lZG51 bSwgJmluc25fdHlwZSwgJmRlbGF5X3Nsb3RzKTsKKworICBpZiAoaW5zbl90 eXBlID09IGltbWVkaWF0ZV9pbnN0KQorICAgIHsKKyAgICAgIGludCByZCwg cmEsIHJiOworCisgICAgICBpbW1mb3VuZCA9IFRSVUU7CisgICAgICBtaW5z dHIgPSBtaWNyb2JsYXplX2RlY29kZV9pbnNuIChpbnNuLCAmcmQsICZyYSwg JnJiLCAmaW1tKTsKKyAgICAgIHBjID0gcGMgKyBJTlNUX1dPUkRfU0laRTsK KyAgICAgIGluc24gPSBtaWNyb2JsYXplX2ZldGNoX2luc3RydWN0aW9uIChw Yyk7CisgICAgICBtaW5zdHIgPSBnZXRfaW5zbl9taWNyb2JsYXplIChpbnNu LCAmaXN1bnNpZ25lZG51bSwgJmluc25fdHlwZSwgJmRlbGF5X3Nsb3RzKTsK KyAgICB9CisgCisgIGlmIChpbnNuX3R5cGUgIT0gcmV0dXJuX2luc3QpCisg ICAgYnJlYWtzWzBdID0gcGMgKyBkZWxheV9zbG90cyAqIElOU1RfV09SRF9T SVpFICsgSU5TVF9XT1JEX1NJWkU7CisgIC8qIE5vdyBjaGVjayBmb3IgYnJh bmNoIG9yIHJldHVybiBpbnN0cnVjdGlvbnMuICAqLworICBpZiAoaW5zbl90 eXBlID09IGJyYW5jaF9pbnN0IHx8IGluc25fdHlwZSA9PSByZXR1cm5faW5z dCkKKyAgICB7CisgICAgICBpbnQgbGltbTsKKyAgICAgIGludCBscmQsIGxy YSwgbHJiOworICAgICAgaW50IHJhLCByYjsKKyAgICAgIGJmZF9ib29sZWFu IHRhcmdldHZhbGlkOworICAgICAgYmZkX2Jvb2xlYW4gdW5jb25kaXRpb25h bGJyYW5jaDsKKyAgICAgIG1pY3JvYmxhemVfZGVjb2RlX2luc24gKGluc24s ICZscmQsICZscmEsICZscmIsICZsaW1tKTsKKyAgICAgIGlmIChscmEgPj0g MCAmJiBscmEgPCBNSUNST0JMQVpFX05VTV9SRUdTKQorICAgICAgICByYSA9 IGdldF9mcmFtZV9yZWdpc3Rlcl91bnNpZ25lZCAoZnJhbWUsIGxyYSk7Cisg ICAgICBlbHNlCisgICAgICAgIHJhID0gMDsKKyAgICAgIGlmIChscmIgPj0g MCAmJiBscmIgPCBNSUNST0JMQVpFX05VTV9SRUdTKQorICAgICAgICByYiA9 IGdldF9mcmFtZV9yZWdpc3Rlcl91bnNpZ25lZCAoZnJhbWUsIGxyYik7Cisg ICAgICBlbHNlCisgICAgICAgIHJiID0gMDsKKyAgICAgIGFkZHJlc3MgPSBt aWNyb2JsYXplX2dldF90YXJnZXRfYWRkcmVzcyAoaW5zbiwgaW1tZm91bmQs IGltbSwKKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgcGMsIHJhLCByYiwKKyAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgJnRhcmdldHZhbGlkLAorICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAmdW5j b25kaXRpb25hbGJyYW5jaCk7CisgICAgICBpZiAoIXVuY29uZGl0aW9uYWxi cmFuY2gpCisgICAgICAgIGJyZWFrc1sxXSA9IGFkZHJlc3M7CisgICAgfQor ICAvKiBJbnNlcnQgdGhlIGJyZWFrcG9pbnRzLiAgKi8KKyAgaWYgKGJyZWFr c1swXSAhPSAtMSkKKyAgICB7CisgICAgICBpbnNlcnRfc2luZ2xlX3N0ZXBf YnJlYWtwb2ludCAoYXJjaCwgYXNwYWNlLCBicmVha3NbMF0pOworICAgICAg cmV0ID0gMTsKKyAgICB9CisgIGlmIChicmVha3NbMV0gIT0gLTEpCisgICAg eworICAgICAgaW5zZXJ0X3NpbmdsZV9zdGVwX2JyZWFrcG9pbnQgKGFyY2gs IGFzcGFjZSwgYnJlYWtzWzFdKTsKKyAgICAgIHJldCA9IDE7CisgICAgfQor CisgIHJldHVybiByZXQ7Cit9CisgCiBzdGF0aWMgdm9pZAogbWljcm9ibGF6 ZV93cml0ZV9wYyAoc3RydWN0IHJlZ2NhY2hlICpyZWdjYWNoZSwgQ09SRV9B RERSIHBjKQogewpAQCAtNzA4LDYgKzc5Myw4IEBAIG1pY3JvYmxhemVfZ2Ri YXJjaF9pbml0IChzdHJ1Y3QgZ2RiYXJjaF9pbmZvIGluZm8sIHN0cnVjdCBn ZGJhcmNoX2xpc3QgKmFyY2hlcykKIAogICBzZXRfZ2RiYXJjaF9icmVha3Bv aW50X2Zyb21fcGMgKGdkYmFyY2gsIG1pY3JvYmxhemVfYnJlYWtwb2ludF9m cm9tX3BjKTsKIAorICBzZXRfZ2RiYXJjaF9zb2Z0d2FyZV9zaW5nbGVfc3Rl cCAoZ2RiYXJjaCwgbWljcm9ibGF6ZV9zb2Z0d2FyZV9zaW5nbGVfc3RlcCk7 CisKICAgc2V0X2dkYmFyY2hfZnJhbWVfYXJnc19za2lwIChnZGJhcmNoLCA4 KTsKIAogICBzZXRfZ2RiYXJjaF9wcmludF9pbnNuIChnZGJhcmNoLCBwcmlu dF9pbnNuX21pY3JvYmxhemUpOwotLSAKMS43LjEKCg== --_002_37378DC5BCD0EE48BA4B082E0B55DFAA40B91915XAPPVEXMBX01xln_--