From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26854 invoked by alias); 24 May 2011 06:44:57 -0000 Received: (qmail 26845 invoked by uid 22791); 24 May 2011 06:44:56 -0000 X-SWARE-Spam-Status: No, hits=1.0 required=5.0 tests=BAYES_05,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RFC_ABUSE_POST,TW_EG,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from nm2.bullet.mail.sp2.yahoo.com (HELO nm2.bullet.mail.sp2.yahoo.com) (98.139.91.72) by sourceware.org (qpsmtpd/0.43rc1) with SMTP; Tue, 24 May 2011 06:44:39 +0000 Received: from [98.139.91.69] by nm2.bullet.mail.sp2.yahoo.com with NNFMP; 24 May 2011 06:44:38 -0000 Received: from [98.139.91.36] by tm9.bullet.mail.sp2.yahoo.com with NNFMP; 24 May 2011 06:44:38 -0000 Received: from [127.0.0.1] by omp1036.mail.sp2.yahoo.com with NNFMP; 24 May 2011 06:44:38 -0000 Received: (qmail 16597 invoked by uid 60001); 24 May 2011 06:44:38 -0000 Message-ID: <343375.12529.qm@web112504.mail.gq1.yahoo.com> Received: from [115.99.18.98] by web112504.mail.gq1.yahoo.com via HTTP; Mon, 23 May 2011 23:44:38 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@web112503.mail.gq1.yahoo.com> <35206.88376.qm@web112501.mail.gq1.yahoo.com> Date: Tue, 24 May 2011 06:44:00 -0000 From: paawan oza Subject: Re: [PATCH] arm reversible : To: =?utf-8?B?UGV0ciBIbHV6w61u?= Cc: gdb-patches@sourceware.org In-Reply-To: MIME-Version: 1.0 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-05/txt/msg00547.txt.bz2 Hi Peter, thank you again for your comments. please find my comments inlined below. patch is coming in next mails. Fri, May 13, 2011 2:50:26 AMRe: [PATCH] arm reversible : From: Petr Hluz=C3=ADn=20=20=20 To: paawan oza=20=20=20 Cc: gdb-patches@sourceware.org=20=20=20 ________________________________ =20 Hi On 7 May 2011 15:49, paawan oza wrote: > Hi Petr and Tom, > > I have implemented most of your comments.. > The comments which I could not put it in are below with explanation. > > 1) ARM_RECORD_ARCH_LIST_ADD_REG is used only at 2 places, but probably I= =20 wanted > to > > give clear interface where it just takes only one argument, register numb= er. > I should have done it for memory, but have not done yet. > > sure, it hides local variable usage and it doesnt look good, but my inten= sion > was to give clear interface > > to function process_record modifiers. > > I am still thinking over it. (The third revision (2011-05-12) of the patch no longer uses the macro. Goo= d.) Clear interface... yes, the macro hides one parameter, but I do not see it as an advantage. In fact the hiding looks more like an disadvantage - it is harder to reason what the code does. (This would apply to its memory variant if you did one.) Why do you think there is an use for an interface? Are there going to be more register-memory-recording implementations behind the interface? If not then the interface would be only a scaffolding prepared for an event which will not happen. Sure, the need to add another implementation of change-recording may arise one day but it may not and if it does then altering the code is matter of doing "find all references" in IDE. In theory any part of a program can have an alternate implementation. =20 Oza: this is taken care. > 2) > + uint32_t opcode; /* insn opcode. */ > > I understand that above is one of the fields of arm_insn_record, but why = I am > usiong it as global field, > because the decoding bits by this field kind of global and common to most= of=20 >the > insns. > > probably I would like to just extract it once and use anywhere, but > unfortunately I have extracted in every function > and seems local, will work on that though. FYI: The struct field is always initialized by "bits (arm_insn_r->arm_insn, 21, 24)" - except in arm_handle_coproc_data_proc_insn(). If it is not a bug then the initializations cannot be merged to one place. =20 (For me: the 3rd revision of the patch still uses the opcode and decode fie= lds.) =20 Oza: yes, I agree, but still I am considering it as global though I have lo= cal=20 usage because it makes an ARM_RECORD, and later in future we can pass whole= =20 structure info to any other function, where opcode might be useful. Current= ly it=20 seems local and may be it will remain local, but probably it was just the d= esign=20 in that way. > 3) > + start_address =3D start_address + 4 > - start_address =3D start_address - 4 > in two of the four cases. > > If you have a look at the code, start_address initialization is the key f= actor > for addressing mode. > not how start_address later gets incremented.. so even in decrement mode, > increment is necessary but > initialization is different. Oh, I now see. You are right. I was confused because I would wrote that differently, but your version is good. =20 Oza: thanks. > 4) > In arm_handle_coproc_data_proc_insn(): >> + tdep->arm_swi_record(reg_cache); > > When this line is executed this function still returns -1 (i.e. failure). > I guess the two if's are intended to return success then. > > oza: the coprocessor insns are not implemented and syscall record too. so= in > both cases we return > -1. But when it does become implemented the branch will incorrectly return a failing value. I assume the recording of the instruction OS-dependent, right? And OS-kind will be run-time configurable? If yes, then I suggest documenting the variability, e.g.: -/* Parse swi insn args, sycall record. */ +/* Parse swi insn args, OS-dependent. */ =20 Oza: if (15 =3D=3D arm_insn_r->opcode) is only for systemcalls, nothing els= e. So anyother coprocessor branch will not fall into if. Once they are impleme= nted=20 no more need to return -1. I can document it if you like. And swi args and parsing is doen in another file which is not yet taken, so= all=20 os depedent stuff will be in linux file. If you see this file is arm-tdep (target depedent, not OS depedent). > 5) > The return value is never tested. What is its meaning? > > The parsed args seem to be stored as a side-effect. Where are they stored? > (Answers should go to comments.) > (Since this is an indirect call, the documentation should be more > verbose than usual cases.) > > Oza: swi basically equivalent to sysenter an int80 insn on x86, so system= call=20 >is > implemented that way. > I am going to work on that implementation in phase 3. > phase 2 does not support it. OK. I guess it will use `arm_insn_r' to record the changes - which would have to be added as an argument. FYI. Oza : ok I guess here I have nothing to change. =20 =20 Moving on. Comments on the version 3 (2011-05-12) of the patch. http://sources.redhat.com/ml/gdb/2011-05/msg00028.html Typos: recors, mendatory, preccedded (-> preceded), accorindly, addresing - addressing, wihtout optmization (Some typos were fixed in v3 before I could reply to v2. Good.) A function arm_handle_data_pro_mic_load_str_insn() in old patch is named as arm_handle_data_pro_mic_load_str_insn() in the last patch. The previous name contained more familiar word/contractions ("misc", "proc") Was that accidental? =20 Oza: corrected back, I was trying to take care of 80 line limit, taken care= and=20 changed to more meaning ful. +#define GET_REG_VAL(REGCACHE,NO,VAL) \ + regcache_raw_read_unsigned (REGCACHE, NO, VAL); + +#define GET_REG_VAL_SIGNED(REGCACHE,NO,VAL) \ + regcache_raw_read_unsigned (REGCACHE, NO, VAL); What do you gain by hiding calls by the macros? E.g. do you expect the the function to be changed? My concern is the slight difficulty reading the code: a reader is more likely to be familiar with the regcache_raw_read..() functions than the macros. Oza: I understand your point. There are many versions of regcache_raw_read= =E2=80=A6.so=20 just was trying to apply clear interface to the next developers, probably t= hey=20 can go for any read API technically, but this is just to restrict them usin= g=20 only single API for record code. In arm_handle_data_pro_mic_load_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. =20 Oza: It is correct, it was mis-evolved as inistially it was union with 2 me= mbers=20 and I fixed Tom=E2=80=99s review comments for endianness. I will change thi= s, but pelase=20 do not mind if it is not there in the immediate versions of patch, eventual= ly=20 after testing it will be changed. in handle_extension_space(): + printf_unfiltered (_("Process record does not support instruction 0x%0= x " + "at address %s.\n"), + arm_insn_r->arm_insn, + paddress (arm_insn_r->gdbarch, arm_insn_r->this_addr= )); The `arm_insn_r->arm_insn' is a block of 4 bytes (ARM), not a classical num= ber. Therefore I feel its more appropriate to include leading zeroes, i.e. use "0x%08x" format. Which formating prefer other fellows? (Does have '0' in "%0x" have any effect?) (In multiple functions.) Oza: well, this is directly taken from x86 code directly as it seems standa= rd=20 way of throwing this type of error. In arm_handle_data_pro_mic_load_str_insn() >+ /* following is always true, in case none of the above conditions meet,= it=20 >will */ >+ else if (arm_insn_r->opcode <=3D 15) You can add "else assert(false)" branch and get rid of the comment. The assert statement has exact meaning, the meaning is faster to read and machines check it. (Even some tools may understand it.) A comment may go wrong over time as a code is changed (and may be wrong even now). Oza: done The same goes for arm_handle_data_proc_imm_insn(). =20 =20 Oza: done. >+#define NO_OF_TYPE_OF_ARM_INSNS 8 >+#define NO_OF_TYPE_OF_THUMB_INSNS 8 These are used only for arm_handle_insn[] and thumb_handle_insn[] array initializations. So far they have the meaning of "the size of the arrays" - which can be easily deduced from the arrays itself. Plus they are fixed by the instruction set architecture - not configurable, unlikely to change, perhaps not to be used on more places - therefore the usual guideline of not embedding magic constants does not apply here. It is a minor point. =20 How do other people feel about it (an identifier which does not have to exi= st)? Oza, do you plan to use them at more places? =20 Oza: point taken and removed, I suppose not going to use it. > > > + struct regcache *regcache; > > > > Many functions use unnecesary cast to init local var, e.g. `(struct > > regcache*) thumb_insn_r->regcache'. > > Most functions do not actually use the var. This is still true. =20 Oza: done and corrected. Thanks. In arm_handle_ld_st_imm_offset_insn() >+ uint32_t reg_src1 =3D 0 , reg_src2=3D 0, reg_dest =3D 0; >+ uint32_t immed_high =3D 0, immed_low =3D 0, offset_12 =3D 0, tgt_mem_ad= dr =3D 0; >+ uint32_t record_buf[8], record_buf_mem[8]; Vars reg_src2, immed_high, immed_low are unused. There are also some unused variables with whole-function scope in other functions. Should I report them? =20 Oza: please report as I don=E2=80=99t have any automated tool which reports= them, if it=20 is not effort for you please report all if possible. Gcc also give some war= ning=20 about unused one, but not sure about gdb warning suppression. > > The ? and : should be at the begging. > > GNU style guideline: "When you split an expression into multiple > > lines, split it before an operator, not after one." > > Also I find it more readable. > >+ ret =3D (0x0F !=3D arm_record->cond) \ >+ ? \ >+ arm_handle_insn[insn_id] (arm_record) \ >+ : \ >+ handle_extension_space(arm_record); Erm, this is even weirder. It is supposed to look like: ret =3D (0x0F !=3D arm_record->cond) ? arm_handle_insn[insn_id] (arm_record) : handle_extension_space(arm_record); Also the patch seems to have extra slashes ('\') at the end. They are unnecessary. Maybe even incorrect. (Did an email client do that?) Oza: corrected. In thumb_handle_ld_st_reg_offset_insn (): Consider applying this change: - } - - opcode1 =3D bits (thumb_insn_r->arm_insn, 10, 12); - if (opcode1) + } else if(bits (thumb_insn_r->arm_insn, 10, 12)) This would allow you to get rid of the `goto' statements. =20 Oza: good one, thanks, done. In decode_insn() > + struct > + { > + ULONGEST unsigned_regval; > + gdb_byte buf[insn_size]; > + } u_buf; The `unsigned_regval' field is not referenced. Oza: corrected. --=20 Petr Hluzin