From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31556 invoked by alias); 28 Apr 2011 18:26:21 -0000 Received: (qmail 31537 invoked by uid 22791); 28 Apr 2011 18:26:19 -0000 X-SWARE-Spam-Status: No, hits=-0.1 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RFC_ABUSE_POST,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from nm28-vm0.bullet.mail.sp2.yahoo.com (HELO nm28-vm0.bullet.mail.sp2.yahoo.com) (98.139.91.234) by sourceware.org (qpsmtpd/0.43rc1) with SMTP; Thu, 28 Apr 2011 18:26:05 +0000 Received: from [98.139.91.65] by nm28.bullet.mail.sp2.yahoo.com with NNFMP; 28 Apr 2011 18:26:04 -0000 Received: from [98.139.91.18] by tm5.bullet.mail.sp2.yahoo.com with NNFMP; 28 Apr 2011 18:26:04 -0000 Received: from [127.0.0.1] by omp1018.mail.sp2.yahoo.com with NNFMP; 28 Apr 2011 18:26:04 -0000 Received: (qmail 52882 invoked by uid 60001); 28 Apr 2011 18:26:04 -0000 Message-ID: <136943.43839.qm@web112518.mail.gq1.yahoo.com> Received: from [123.238.92.202] by web112518.mail.gq1.yahoo.com via HTTP; Thu, 28 Apr 2011 11:26:03 PDT References: <341905.10459.qm@web112513.mail.gq1.yahoo.com> <208397.95006.qm@web112517.mail.gq1.yahoo.com> <4DA27006.1080607@codesourcery.com> <763549.92092.qm@web112506.mail.gq1.yahoo.com> <335149.24692.qm@web112515.mail.gq1.yahoo.com> <592215.58786.qm@web112508.mail.gq1.yahoo.com> <172713.29831.qm__351.089161313389$1303740245$gmane$org@web112503.mail.gq1.yahoo.com> Date: Thu, 28 Apr 2011 18:26:00 -0000 From: paawan oza Subject: Re: [PATCH] arm reversible : To: Tom Tromey Cc: =?iso-8859-1?Q?Petr_Hluz=EDn?= , gdb@sourceware.org, gdb-patches@sourceware.org In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable 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: 2011-04/txt/msg00547.txt.bz2 Hi Tom, I have implemented all the comments, except one thing I am not sure. Tom's comments Oza> + union Oza> + { Oza> + uint32_t s_word; Oza> + gdb_byte buf[4]; Oza> + } u_buf[2]; There are lots of these little unions in the code. =46rom what I can tell, this is mostly to read target memory into the array, then convert to a scalar like: Oza> + GET_REG_VAL (reg_cache, reg_src1, &u_buf[0].buf[0]); [...] Oza> + arm_insn_r->arm_mems[1].addr =3D u_buf[0].s_word; It seems to me that extract_typed_address or something similar would be better. I'm not very familiar with the target record stuff; but if it is possible to cross-debug and use it, then this code does not seem to account for host/target endian differences. Oza's query; 1) are suggesting to use something else than unions ? whatever you have=20 described about union is right. 2) why could I use extract_typed_address instead of direct assignment ? 3) in my understanding cross debug would not affect endianess because all = the=20 record saving done on target memory and played back in target memory. they never get fetched to host machine I think. please clarify how would it affect host/target endianness issue ? I am confused. Regards, Oza. ----- Original Message ---- From: Tom Tromey To: paawan oza Cc: Petr Hluz=EDn ; gdb@sourceware.org;=20 gdb-patches@sourceware.org Sent: Tue, April 26, 2011 1:27:17 AM Subject: Re: [PATCH] arm reversible : >>>>> "Oza" =3D=3D paawan oza writes: Oza> PS: I have added REG_ALLOC and MEM_ALLOC macors to accumulate Oza> xmallocs, but I am not sure whether to go for that macros or leave Oza> the implementation of reg/mem allcoation scattered across code Oza> calling xmalloc. I saw one use of REG_ALLOC and none of MEM_ALLOC. If you intend to just have a single use it is probably better to just drop the macros. But, maybe you intend to use these universally through the code. That would be fine. Oza> + /* Enable process record */ Period plus two spaces at the end of a comment. Oza> + set_gdbarch_process_record(gdbarch, arm_process_record); Space before an open paren. There are a lot of small formatting problems in this patch. Oza> #include "features/arm-with-m.c" Oza> + Oza> static int arm_debug; Adding a blank line for no reason. Oza> +/* arm-reversible process reacord data structures. */ Surely it should be "ARM". Oza> +struct arm_mem_r Oza> +{ Oza> + uint32_t len; Oza> + CORE_ADDR addr; Oza> +}; Wrong formatting. The struct needs documentation and so do its fields. Oza> + Oza> +static int Oza> +SBO_SBZ (uint32_t insn, uint32_t bit_num, uint32_t len, uint32_t sbo) A new function needs a documentation comment. Uppercase function names need special pleading. Oza> + uint32_t ONES =3D bits (insn, bit_num - 1, (bit_num -1) + (len - 1)= ); Likewise locals. Oza> +static int=20 Oza> +arm_handle_data_proc_misc_load_str_insn \ Oza> +(insn_decode_record *arm_insn_r) Bad formatting. Oza> + union Oza> + { Oza> + uint32_t s_word; Oza> + gdb_byte buf[4]; Oza> + } u_buf[2]; There are lots of these little unions in the code. =46rom what I can tell, this is mostly to read target memory into the array, then convert to a scalar like: Oza> + GET_REG_VAL (reg_cache, reg_src1, &u_buf[0].buf[0]); [...] Oza> + arm_insn_r->arm_mems[1].addr =3D u_buf[0].s_word; It seems to me that extract_typed_address or something similar would be better. I'm not very familiar with the target record stuff; but if it is possible to cross-debug and use it, then this code does not seem to account for host/target endian differences. Oza> + /* SPSR is going to be changed. */ Oza> + /* Oza: FIX ME ? how to read SPSR value? */ There are a few comments in this style. First, don't put your name into comments. Second, I think instead of new "FIXME" comments, new code should either just work, or call error and have an explanatory comment. I can't really comment on the details of the implementation. I don't know much about either ARM or process record. It seems basically reasonable; I prefer a somewhat more symbolic style, but I understand that the x86 process record code is also written with many manifest constants. Tom