From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20153 invoked by alias); 4 Dec 2011 13:29:53 -0000 Received: (qmail 20138 invoked by uid 22791); 4 Dec 2011 13:29:52 -0000 X-SWARE-Spam-Status: No, hits=-2.5 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; Sun, 04 Dec 2011 13:29:38 +0000 Received: by wgbds11 with SMTP id ds11so4419234wgb.12 for ; Sun, 04 Dec 2011 05:29:37 -0800 (PST) Received: by 10.227.206.4 with SMTP id fs4mr8974613wbb.21.1323005377178; Sun, 04 Dec 2011 05:29:37 -0800 (PST) MIME-Version: 1.0 Received: by 10.223.97.14 with HTTP; Sun, 4 Dec 2011 05:29:16 -0800 (PST) In-Reply-To: References: <998639.46560.qm@web112516.mail.gq1.yahoo.com> <4EDAD0EF.20405@codesourcery.com> From: =?UTF-8?B?UGV0ciBIbHV6w61u?= Date: Sun, 04 Dec 2011 13:29:00 -0000 Message-ID: Subject: Re: [PATCH] arm reversible : To: oza Pawandeep Cc: Yao Qi , Tom Tromey , paawan oza , "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-12/txt/msg00097.txt.bz2 2011/12/4 oza Pawandeep : > Hi, > > Updated patch contains all the 3 review comments fixed (Tom, Petr and Yao= ). > Change log is elaborated; I am looking forwarding to adding more > details if need. seeking for your feedback on the same. > > diff -urN arm_orig/ChangeLog arm_new/ChangeLog > --- arm_orig/ChangeLog =C2=A02011-12-03 18:05:04.000000000 +0530 > +++ arm_new/ChangeLog =C2=A0 2011-12-04 16:45:00.000000000 +0530 > @@ -1,3 +1,65 @@ > +2011-12-03 =C2=A0Oza Pawandeep =C2=A0 > + > + =C2=A0 =C2=A0 =C2=A0 * arm-linux-tdep.c: arm_linux_init_abi modified to= include > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0arm reversible debugging feature. > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0registered arm_process_record to gdb_= arch > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0syscall pointer initialization. > + > + =C2=A0 =C2=A0 =C2=A0 * arm-tdep.c: arm-reversible-debugging implementat= ion. > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 newly added functions are as follows. > + > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> arm_process_record: handles basic i= nitialization and record > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 summarisation, decodes basic = insn ids, on which it hands over > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 controls to decode_insn. > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> deallocate_reg_mem : clean up funct= ion > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> decode_insn: Decodes arm/thumb insn= and calls appropriate > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 decoding routine to record th= e change. > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> thumb_record_branch: branch insn re= ording (thumb) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> thumb_record_ldm_stm_swi: load, sto= re and sycall insn > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 recoding =C2=A0(thumb) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> thumb_record_misc: misc insn record= ing =C2=A0(thumb) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> thumb_record_ld_st_stack: store and= stack insn recording =C2=A0(thumb) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> thumb_record_ld_st_imm_offset: load= , store with immediate offset > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 insn recording =C2=A0(thumb) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> thumb_record_ld_st_reg_offset: load= , store with register offset > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 recording =C2=A0(thumb) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> thumb_record_add_sub_cmp_mov: addit= ion, subtractation, compare > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 and move insn recording =C2= =A0(thumb) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> thumb_record_shift_add_sub: shift, = add and sub insn recording > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (thumb) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> arm_record_coproc_data_proc: coproc= essor and data processing > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 recording (partially implemen= ted) (arm) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> arm_record_coproc: coprocessor insn= recording > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (partially implemented) (arm) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> arm_record_b_bl: branch insn record= ing (arm) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> arm_record_ld_st_multiple: load and= store multiple insn recording > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (arm) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> arm_record_ld_st_reg_offset: load a= nd store reg offset recording > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (arm) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> arm_record_ld_st_imm_offset: load a= nd store immediate offset > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 recording (arm) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> arm_record_data_proc_imm: data proc= essing insn recording =C2=A0(arm) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> arm_record_data_proc_misc_ld_str: d= ata processing, misc, load and > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 store insn recording =C2=A0(a= rm) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> arm_record_extension_space:arm exte= nsion space insn recording > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (arm) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> arm_record_strx: str(X) type insn r= ecording =C2=A0(arm) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> sbo_sbz: checks for mendatory sbo a= nd sbz fields in insn, > + > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0added new data structures: > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> insn_decode_record_t: local record = structure which contains insn's > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0record, which includes both reg and m= emory. > + > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0REG_ALLOC and MEM_ALLOC macros takes = care of actual memory allocation > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0in local record which is finally proc= essed by arm_rocess_record. > + > + =C2=A0 =C2=A0 =C2=A0 * arm-tdep.h: arm-reversible data structures > + > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 > modified gdbarch_tdep: added member (func= tion pointer) arm_swi_record > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0which is supposed to be record= ing system calls > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 > arm_process_record externed. > + > + WOW, thats a lot of text. Other people's changelog entries are more terse. For example the GNU guidelines say "For example, =E2=80=9CNew functi= on=E2=80=9D is enough for the change log when you add a function, because there should be a comment before the function definition to explain what it does." In function decode_insn(): > + > + =C2=A0if (extract_arm_insn (arm_record, insn_size)) > + =C2=A0 =C2=A0{ > + =C2=A0 =C2=A0 =C2=A0if (record_debug) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0{ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf_unfiltered (_("Process record:= error reading memory at " > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"addr %s len =3D %d.\n"), > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0paddress (arm_record->gdbarch, arm_re= cord->this_addr), insn_size); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + =C2=A0 =C2=A0} If the extract_arm_insn() call fails then the function returns only if `record_debug' is true. This is strange. Enabling debugging traces usually expected to only affect printing, here it affects behavior of the decode_insn() function. Plus the rest of the function seems to misbehave if it is passed failed result in `arm_record'. I guess the `return -1;' statement should be moved outside of body of condition `if (record_debug)'. Unortunatelly I did not notice that earlier. In arm_process_record(): > + > + =C2=A0if (extract_arm_insn (&arm_record, 2)) > + =C2=A0 =C2=A0{ > + =C2=A0 =C2=A0 =C2=A0if (record_debug) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0{ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf_unfiltered (_("Process record:= error reading memory at " > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 "addr %s len =3D %d.\n"), > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 paddress (arm_record.gdbarch, > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 arm_record.this_addr), 2); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + =C2=A0 =C2=A0} The same applies here. Except the one last thing the patch looks good to me now. My previous suggestions have been resolved. As always, I did not check whitespace, ARM semantics and Changelog entries. --=20 Petr Hluzin