From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30185 invoked by alias); 16 Oct 2011 21:03:29 -0000 Received: (qmail 30173 invoked by uid 22791); 16 Oct 2011 21:03:28 -0000 X-SWARE-Spam-Status: No, hits=-1.0 required=5.0 tests=AWL,BAYES_50,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-bw0-f41.google.com (HELO mail-bw0-f41.google.com) (209.85.214.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 16 Oct 2011 21:03:13 +0000 Received: by bkbzu5 with SMTP id zu5so5556504bkb.0 for ; Sun, 16 Oct 2011 14:03:12 -0700 (PDT) Received: by 10.223.63.206 with SMTP id c14mr7332924fai.7.1318798992114; Sun, 16 Oct 2011 14:03:12 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.89.11 with HTTP; Sun, 16 Oct 2011 14:02:51 -0700 (PDT) In-Reply-To: <1318650316.91503.YahooMailNeo@web112508.mail.gq1.yahoo.com> 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> From: =?UTF-8?B?UGV0ciBIbHV6w61u?= Date: Sun, 16 Oct 2011 23:32:00 -0000 Message-ID: Subject: Re: [PATCH] arm reversible : To: paawan oza Cc: Tom Tromey , "gdb-patches@sourceware.org" , chandra krishnappa Content-Type: text/plain; charset=UTF-8 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-10/txt/msg00450.txt.bz2 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 enou= gh. 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() >+ switch (arm_insn_r->opcode) In arm_handle_ld_st_reg_offset_insn(): >+ switch (arm_insn_r->opcode) >+ switch (offset_12) >+ switch (arm_insn_r->opcode) In arm_handle_ld_st_multiple_insn() >+ 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> + gdb_assert_not_reached ("no decoding pattern found"); > Tom> It seems wrong to use an assert in this code. At least, it is not Tom> obvious to me that this represents a logic error in gdb as opposed to a Tom> merely unrecognized instruction. An 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 sugges= ted). 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() + if (15 =3D=3D arm_insn_r->opcode) ... + /* Handle arm syscall insn. */ + if (tdep->arm_swi_record !=3D NULL) + { + tdep->arm_swi_record(reg_cache); + } ... + 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: - tdep->arm_swi_record(reg_cache); + 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() >> + struct >> + { >> + ULONGEST unsigned_regval; >> + } 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=E2=80=99s review comments for endianness. I will change t= his, but pelase > do not mind if it is not there in the immediate versions of patch, eventu= ally > after testing it will be changed. This is still true. > Oza: please report as I don=E2=80=99t have any automated tool which repor= ts them, if it > is not effort for you please report all if possible. Gcc also give some w= arning > 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 -> Without optimization, Otherwise the patch looks good. --=20 Petr Hluzin