From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 558 invoked by alias); 11 Jan 2010 02:48:02 -0000 Received: (qmail 496 invoked by uid 22791); 11 Jan 2010 02:48:01 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,SARE_MSGID_LONG40,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail-px0-f175.google.com (HELO mail-px0-f175.google.com) (209.85.216.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 11 Jan 2010 02:47:57 +0000 Received: by pxi5 with SMTP id 5so15571095pxi.12 for ; Sun, 10 Jan 2010 18:47:56 -0800 (PST) MIME-Version: 1.0 Received: by 10.142.9.34 with SMTP id 34mr2260395wfi.114.1263178076091; Sun, 10 Jan 2010 18:47:56 -0800 (PST) In-Reply-To: <20100109122833.GB2007@adacore.com> References: <4B215151.6040608@vmware.com> <4B26825B.7000209@vmware.com> <4B2BDAEC.7090207@vmware.com> <20100109122833.GB2007@adacore.com> From: Hui Zhu Date: Mon, 11 Jan 2010 02:48:00 -0000 Message-ID: Subject: Re: [RFA/i386] Prec x86 MMX 3DNow! SSE SSE2 SSE3 SSSE3 SSE4 support To: Joel Brobecker Cc: Michael Snyder , gdb-patches ml 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: 2010-01/txt/msg00239.txt.bz2 Thanks Joel, On Sat, Jan 9, 2010 at 20:28, Joel Brobecker wrote: >> 2010-01-09 =A0Hui Zhu =A0 >> >> =A0 =A0 =A0 * i386-tdep.c (OT_DQUAD): New enum. >> =A0 =A0 =A0 (i386_process_record): Add code for MMX, 3DNow!, SSE, SSE2, >> =A0 =A0 =A0 SSE3, SSSE3 and SSE4. > > My two cents below. Although I have not seen anything incorrect with this > patch, I am reluctant to approve it outright. MarkK has been maintaining > the x86 target for a while, so it'd be nice if he could take a look > (unless he doesn't want to). =A0Please ping me in a week if no one else > has looked at this patch. > > Please also start adding testcases if you can. This is a lot of code, > with lots of hard-coded numbers and very little comments. Perhaps > the code is self-explanatory for an x86 specialist, but I'm not, so... Now, the current testcases will use some sse insn. I think they are test for it. For the sse insn special test, when I write the code for it, I just read the spec and write the prec code. I don't good at write sse asm. Sorry for it. I need a people help me with it. > >> @@ -5122,7 +5123,7 @@ i386_process_record (struct gdbarch *gdb >> =A0 =A0 =A0 =A0break; >> >> =A0 =A0 =A0case 0x62: =A0 =A0/* bound */ >> - =A0 =A0 =A0printf_unfiltered (_("Process record doesn't support " >> + =A0 =A0 =A0printf_unfiltered (_("Process record does not support " >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"instruction bound.\n= ")); >> =A0 =A0 =A0 =A0ir.addr -=3D 1; >> =A0 =A0 =A0 =A0goto no_support; > > Generally speaking, I find the error message phrased in a way that > they can be confusing sometimes, such as in the example above. > I suggest instead something such as: > > =A0 Instruction 'bound' is not supported by Process Record. > > or: > > =A0 Instruction not supported by Process Record: bound. > =A0 Instruction not supported by Process Record ("bound") > > etc... > > Also, the design you are using to bail out, is using labels and > gotos unnecessarily. =A0This, in turn, leads to the following oddity > in the case above: > > =A0 (gdb) cont > =A0 Process record does not support instruction bound. > =A0 Process record does not support instruction 0x62 at address 0xdeadbee= f. > =A0 error: Process record: failed to record execution log. > > The first two error messages are almost identical and the second one > should simply go. =A0I also think that the reason for failing to record > the execution log should be placed *after* the error message, but > that may be a personal preference. For instance: > > =A0 =A0error: Process record: Failed to record execution log: > =A0 =A0Process record does not support instruction bound at address 0xfee= dface. > > Again, let's not address this issue as part of this patch, but once > this patch is in, can you please start preparing another one which > cleans this up by getting rid of these labels: > > =A0 =A01. Define a new function: > =A0 =A0 =A0 =A0 i386_insn_record_not_supported (struct gdbarch *gdbarch, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 const char *insn_name, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 CORE_ADDR insn_addr); > > =A0 =A0 =A0 Change the code above to: > =A0 =A0 =A0 =A0 i386_insn_record_not_supported (gdbarch, "bound", ir.addr= ); > =A0 =A0 =A0 =A0 return -1; > > =A0 =A02. Define a new function: > =A0 =A0 =A0 =A0 i386_opcode_record_not_supported (strct gdbarch* gdbarch, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 uint32_t opcode, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 CORE_ADDR insn_addr); > > =A0 =A0 =A0 Change the goto no_support into: > =A0 =A0 =A0 =A0 i386_opcode_record_not_supported (gdbarch, opcode, ir.add= r); > =A0 =A0 =A0 =A0 return -1; > > I would even consider the idea of changing the semantics of your > function, and raise an error whose message carries the reason for > the error, instead of doing the printing yourself before returning -1. > >> + =A0 =A0case 0x0f0f: =A0 =A0/* 3DNow! data */ >> + =A0 =A0 =A0if (i386_record_modrm (&ir)) >> + =A0 =A0 return -1; > I am not sure it better can be the part of this patch. It doesn't have relationship with sse insn support, right? Looks you have a lot of idea on it. I suggest you can work on it. :) > This is the type of code that could definitely use some comments. > Why are you return -1 (error condition, right?) in this case? Because > of this immediate return, there won't even been any feedback to the user > as to why the recording failed. > >> + =A0 =A0 =A0switch (tmpu8) > > This is also for another followup patch, but if you could use more > meaningful variable names, that would help improve the code quality > (IMO). > + if (target_read_memory (ir.addr, &tmpu8, 1)) + { + printf_unfiltered (_("Process record: error reading memory at " + "addr %s len =3D 1.\n"), + paddress (gdbarch, ir.addr)); + return -1; + } + ir.addr++; + switch (tmpu8) Im not sure it clear or not. I think add more vals will make the code not clear. Best regards, Hui