From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21830 invoked by alias); 22 Oct 2011 14:48:29 -0000 Received: (qmail 21816 invoked by uid 22791); 22 Oct 2011 14:48:27 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-wy0-f169.google.com (HELO mail-wy0-f169.google.com) (74.125.82.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 22 Oct 2011 14:48:13 +0000 Received: by wyg34 with SMTP id 34so5768774wyg.0 for ; Sat, 22 Oct 2011 07:48:12 -0700 (PDT) MIME-Version: 1.0 Received: by 10.227.195.71 with SMTP id eb7mr462194wbb.51.1319294892036; Sat, 22 Oct 2011 07:48:12 -0700 (PDT) Received: by 10.180.105.69 with HTTP; Sat, 22 Oct 2011 07:48:11 -0700 (PDT) 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: Sat, 22 Oct 2011 15:42:00 -0000 Message-ID: Subject: Re: [PATCH] arm reversible : From: oza Pawandeep To: =?ISO-8859-1?Q?Petr_Hluz=EDn?= Cc: paawan oza , Tom Tromey , "gdb-patches@sourceware.org" , chandra krishnappa Content-Type: text/plain; charset=windows-1252 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-10/txt/msg00613.txt.bz2 Hi Petr, Thanks for your comments again. I have tried to take care of all your comments except 1) there is contradiction between yours and Tom's comment about gdb_assert_not_reached. I have changed back and forth earlier. currently it return -1, indirectly gdb throws __error which results into record stop completely. 2) the local variable usage as you sueggsted definately improves cache line usage and performance I want to leave it to regression probably in phase - 3; because other changes from Yao and you have taken good amount of time. please excuse me If you have to repeat he comments: as this patch has grown too much that I miss something some or the other th= ing the next patch update include the implementation of arm_extension_space ins= n. Regards, Oza. On Mon, Oct 17, 2011 at 2:32 AM, Petr Hluz=EDn wrot= e: > On 15 October 2011 05:45, paawan oza wrote: >> please find the patch below. > > I did not verify the style guidelines (spaces etc.), I am not familiar en= ough. > However some places have a comma ',' at the beginning of new line - I > think the "operator at newline" guideline does not apply to commas. > > I did not check ARM semantics. It looks plausible, though. > > The current patch (2011-10-15) is definitely an improvement to 2011-05-12. > Only the assertion thing got worse: > > In arm_handle_ld_st_imm_offset_insn() >>+ =A0 switch (arm_insn_r->opcode) > In arm_handle_ld_st_reg_offset_insn(): >>+ =A0 switch (arm_insn_r->opcode) >>+ =A0 switch (offset_12) >>+ =A0 switch (arm_insn_r->opcode) > In arm_handle_ld_st_multiple_insn() >>+ =A0 switch (addr_mode) > > These switches seem to have `return -1;' in cases which are not > reachable, therefore shoudl have gdb_assert_not_reached(). > The guideline - which I think Tom was reffering to - is that > impossible states and coding bugs in gdb should trigger assertions > however user's input (no matter how malformed) should trigger warning > or error messages. > (This would mean to turn the lines with `default:' to the previous > revision. I understand this sucks.) > > Some situations are difficult to decide whether they are trigger-able > by user input or not. > If my code is not coded or intended to handle such situations I prefer > to kill the process (or whatever are assertions configured to do) and > get feedback from user. > I am not familiar with GDB customs, though. Tom? > > Oza> + =A0 =A0 =A0gdb_assert_not_reached ("no decoding pattern found"); >> > Tom> It seems wrong to use an assert in this code. =A0At least, it is not > Tom> obvious to me that this represents a logic error in gdb as opposed t= o a > Tom> merely unrecognized instruction. =A0An unrecognized instruction can = occur > Tom> for many reasons, e.g., a bad jump. > > The switch variable `arm_insn_r->opcode' cannot be initialized to any > value different from 0..15 because of the expression: > arm_insn_r->opcode =3D bits (arm_insn_r->arm_insn, 21, 24). > The switch variable `offset_12' cannot be initialized to any value > different from 0..3 because of the expression: offset_12 =3D bits > (arm_insn_r->arm_insn, 5, 6). > The switch variable `addr_mode' cannot be initialized to any value > different from 0..3 because of the expression: addr_mode =3D bits > (arm_insn_r->arm_insn, 23, 24). > It would be bit easier to see that if the variables were local (as I sugg= ested). > Other values are not possible to create, even with corrupted input or > unrecognized instructions. > > Paawan: If Tom and I give you contradicting suggestions then you > should complain. > > > Issues remaining from my previous reviews: > > In arm_handle_coproc_data_proc_insn() > + =A0 if (15 =3D=3D arm_insn_r->opcode) > ... > + =A0 =A0 =A0 =A0/* Handle arm syscall insn. =A0*/ > + =A0 =A0 =A0 =A0if (tdep->arm_swi_record !=3D NULL) > + =A0 =A0 =A0 =A0 =A0{ > + =A0 =A0 =A0 =A0 =A0 =A0tdep->arm_swi_record(reg_cache); > + =A0 =A0 =A0 =A0 =A0} > ... > + =A0 return -1; > > The function still returns -1 even when the instruction was recognized > (15) and OS syscall support function is not NULL. > Yes, there is no way to set it to non-NULL value yet but when it is > implemented then you would have to do this change anyway: > - =A0 tdep->arm_swi_record(reg_cache); > + =A0 return tdep->arm_swi_record(reg_cache); > > I guess the function should use `arm_insn_r' argument to record the > changes - which is missing. > In thumb_handle_swi_insn() the situation is the opposite: it returns 0 > no matter what the arm_swi_record() returns. > > > In arm_handle_data_proc_misc_ld_str_insn() >>> + =A0struct >>> + =A0 =A0{ >>> + =A0 =A0 =A0ULONGEST unsigned_regval; >>> + =A0 =A0} u_buf[2]; >>> >>> You can get the same result (and simpler code) with >>> ULONGEST u_buf[2]; >>> or maybe also better name with >>> ULONGEST u_regvals[2]; >>> >>> The same applies to other functions. >> >> Oza: It is correct, it was mis-evolved as inistially it was union with 2= members >> and I fixed Tom=92s review comments for endianness. I will change this, = but pelase >> do not mind if it is not there in the immediate versions of patch, event= ually >> after testing it will be changed. > > This is still true. > > >> Oza: please report as I don=92t have any automated tool which reports th= em, if it >> is not effort for you please report all if possible. Gcc also give some = warning >> about unused one, but not sure about gdb warning suppression. > > Unused local variables, as requested: > arm_handle_ld_st_imm_offset_insn: reg_src2, immed_high, immed_low > arm_handle_ld_st_reg_offset_insn: immed_high, immed_low > arm_handle_ld_st_multiple_insn: shift_imm, reg_src2 > arm_handle_coproc_data_proc_insn - all of them: shift_imm, reg_src1, > reg_src2, addr_mode, start_address > thumb_handle_ld_st_imm_offset_insn: reg_val1 > thumb_handle_ld_st_stack_insn: reg_val1 > thumb_handle_misc_insn: reg_val1, reg_src1 - write-only in case `(2 =3D=3D > opcode)', immed_8, immed_5 > thumb_handle_swi_insn: reg_val1 > thumb_handle_branch_insn: reg_val1, reg_src1, opcode, immed_5 > > Typos: > preccedded -> precceded (3x), Accorindly -> Accordingly (2x), > addresing -> addressing, optmization -> optimization, Wihtout > optmization =A0-> Without optimization, > > Otherwise the patch looks good. > > -- > Petr Hluzin >