From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1227 invoked by alias); 8 Nov 2011 06:02:13 -0000 Received: (qmail 1219 invoked by uid 22791); 8 Nov 2011 06:02:10 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,TW_SB X-Spam-Check-By: sourceware.org Received: from mail-ww0-f43.google.com (HELO mail-ww0-f43.google.com) (74.125.82.43) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 08 Nov 2011 06:01:52 +0000 Received: by wwf4 with SMTP id 4so152148wwf.12 for ; Mon, 07 Nov 2011 22:01:50 -0800 (PST) MIME-Version: 1.0 Received: by 10.180.82.68 with SMTP id g4mr11021054wiy.11.1320732110637; Mon, 07 Nov 2011 22:01:50 -0800 (PST) Received: by 10.180.102.133 with HTTP; Mon, 7 Nov 2011 22:01:50 -0800 (PST) In-Reply-To: References: <998639.46560.qm@web112516.mail.gq1.yahoo.com> <321260.58442.qm@web112504.mail.gq1.yahoo.com> <1316327455.23344.YahooMailNeo@web112509.mail.gq1.yahoo.com> <1316404058.27177.YahooMailNeo@web112502.mail.gq1.yahoo.com> <1318650316.91503.YahooMailNeo@web112508.mail.gq1.yahoo.com> Date: Tue, 08 Nov 2011 06:02:00 -0000 Message-ID: Subject: Re: [PATCH] arm reversible : From: oza Pawandeep To: Tom Tromey Cc: =?ISO-8859-1?Q?Petr_Hluz=EDn?= , paawan oza , "gdb-patches@sourceware.org" , chandra krishnappa Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes 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-11/txt/msg00169.txt.bz2 Hi All, sorry for delay in reply. was on vacation. I have already addressed the comments from both Petr and Yao in the next pa= tch. the comments which I have not fixed please find the explanation below. Petr >> Is it supposed to return a boolean-like value? If yes then it would be more descriptive to use INSN_IS_RECORDED. However I do not know if GDB uses this convention. Tom? Also there are extra parentheses. There should be parentheses around ARM_RECORD argument. Consider adding `=3D=3D NULL' to the two tests to make it clear the fields are pointers, not integers or booleans, and that the || is supposed to be logical, not bit-wise. (I have heard few people have tools good enough to tell them the types.) Oza >> I have changed the condition itself now. you were right; after changing now it makes sense. Petr >> Why don't you use `record_buf_mem[0]' and `record_buf_mem[1]' syntax? Oza >> because calling function takes care of REG_ALLOC routine calls. I did not want ALLOC calls to be scattered into any other function other than main decoding functions. Petr >> This condition is never true because `opcode1 =3D bits (arm_insn_r->arm_insn, 4, 7)' is value in range 0..15 therefore `opcode1 & 0xE0' is always zero. I think. Oza >> you are right, I fixed it now. Oza >> typo mistakes are corrected. Petr >> Operators should go to beginning of a new line, not at the end of previous line. Oza >> corrected. Petr >> In function arm_record_extension_space: The function stores some values in local variable `uint32_t record_buf[8]' but these values are never read. I suspect you forgot to add calls to: + REG_ALLOC (arm_insn_r->arm_regs, arm_insn_r->reg_rec_count, record_buf); + MEM_ALLOC (arm_insn_r->arm_mems, arm_insn_r->mem_rec_count, record_buf_m= em); Oza >> yes, thanks for this comment, I added them now. Petr >> n function arm_record_extension_space: The calls to INSN_RECORDED(arm_insn_r) will always return false, because the fields referenced by the macro are always 0 at entry to arm_record_extension_space() and the function does not modify them. The missing calls to REG_ALLOC() & MEM_ALLOC() would modify them in the end but it would be late anyway. I guess you will have to either add a local flag variable or alter control flow - either way the INSN_RECORDED seems like going to be unused. (Function arm_record_strx() works differently and is not affected.) In function arm_record_extension_space: Consider adding `gdb_assert (!INSN_RECORDED(arm_insn_r))' at the beginning of the function. That way it will be easy to understand which part of code does change the state. In function decode_insn: Consider adding `gdb_assert (!INSN_RECORDED(arm_insn_r))' just before call to arm_record_extension_space. That way it will be easy to understand which part of code does change the state. Or use return value from arm_record_extension_space() - this would make the understanding even faster. In function decode_insn: Consider adding these assertions at the exit: + gdb_assert (arm_record->mem_rec_count =3D=3D 0 || arm_record->arm_mems != =3D NULL); + gdb_assert (arm_record->reg_rec_count =3D=3D 0 || arm_record->arm_regs != =3D NULL); This would catch the missing REG_ALLOC calls in arm_record_extension_space(= ). Oza >> the whole logic is falling in place now, everything is taken care with the change of the MACRO IS_INSN_RECORDED. Yao >> macro THUMB2_INSN_SIZE_BYTES is never used. Please remove it. Oza >> it is intropduced again as required in the logic which you have prop= osed. Yao >> This function is used to match opcode for instructions. Why don't you use bit operation (AND and OR) and logic operation to match instruction? Bit operation and logic operation are widely used in gdb. It is efficient and easy to read. I suggest to replace sbo_sbz by bit/logical operation when matching instruction. Oza >> I am not sure what you meant by re-writting sbo_sbz function. as there is already '&' and '!' and '>>' which all are bit operators. Yao >> 15 is a magic number here and somewhere else. Please use ARM_PC_REGNUM. Oza >> corrected. Yao >> Comment here is confusing. STRH (register) and STRH (immediate) are handled here, but comment doesn't reflect this. Oza >> done Yao >> The name of function is incorrect. Better to name it "thumb_record_ldm_stm_swi", or something similar. Oza >> done Yao >> Again, don't have to store register PS for BL. Oza >> done Yao >> This kind of design leaves thumb2-32 bit support difficult in the future. We can let the caller, arm_process_record, to differentiate arm/thumb 16bit/thumb 32bit, and call different decode routines respectively, like this, Oza >> proposed logic incorporated in caller. Regards, Oza. On Mon, Nov 7, 2011 at 9:08 PM, Tom Tromey wrote: >>>>>> "Petr" =3D=3D Petr Hluz=EDn writes: > > Petr> How strong is the "can't happen" requirement? Is programmer required > Petr> to prove the assertion cannot be triggered? The difficulty in gener= al > Petr> case would be equivalent to proving a code is bug-free - i.e. it is > Petr> impossible. At my job we try to verify an assumption until we have a > Petr> sort-of-proof or the search becomes difficult and no clue/indication > Petr> to the contrary has been found yet. > Petr> I am asking because GDB code contains surprisingly few assertions. > > I don't think there is a hard-and-fast rule in gdb. =A0I am not completely > sure though. > > I think in gdb it is best to error instead of assert if there is any > doubt. =A0That's because I think when people turn to gdb they are already > a bit frustrated, and then if gdb itself fails, this is extremely > irritating. =A0That's certainly been the case for me in the past. > > I realize you can sort of recover from internal_error (and hence > assertions). =A0But my impression is that internal_error is treated like > "not an exception-thrower" in gdb, leading to cleanup problems and the > like. > > Petr> Anyway, the patch had used assertions correctly because expression > Petr> `bits (X, 21, 24)' yields value in range 0..15 for any value of X -= no > Petr> matter how bogus X value. Yes, all 2^32 values map to 0..15. The > Petr> assertions satisfy the guideline. (This is not even the hard-to-pro= ve > Petr> case I was speculating above.) > > Yeah, my review was wrong here. > > Tom >