From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11268 invoked by alias); 9 Jan 2010 12:28:56 -0000 Received: (qmail 11252 invoked by uid 22791); 9 Jan 2010 12:28:55 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 09 Jan 2010 12:28:51 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 8FDCF2BAB5C; Sat, 9 Jan 2010 07:28:49 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id DIOfkPZo5-Rs; Sat, 9 Jan 2010 07:28:49 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id E1F662BAB59; Sat, 9 Jan 2010 07:28:48 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 4C055F595E; Sat, 9 Jan 2010 16:28:33 +0400 (RET) Date: Sat, 09 Jan 2010 12:28:00 -0000 From: Joel Brobecker To: Hui Zhu Cc: Michael Snyder , gdb-patches ml Subject: Re: [RFA/i386] Prec x86 MMX 3DNow! SSE SSE2 SSE3 SSSE3 SSE4 support Message-ID: <20100109122833.GB2007@adacore.com> References: <4B215151.6040608@vmware.com> <4B26825B.7000209@vmware.com> <4B2BDAEC.7090207@vmware.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) 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/msg00211.txt.bz2 > 2010-01-09 Hui Zhu > > * i386-tdep.c (OT_DQUAD): New enum. > (i386_process_record): Add code for MMX, 3DNow!, SSE, SSE2, > 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). Please 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... > @@ -5122,7 +5123,7 @@ i386_process_record (struct gdbarch *gdb > break; > > case 0x62: /* bound */ > - printf_unfiltered (_("Process record doesn't support " > + printf_unfiltered (_("Process record does not support " > "instruction bound.\n")); > ir.addr -= 1; > goto 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: Instruction 'bound' is not supported by Process Record. or: Instruction not supported by Process Record: bound. Instruction not supported by Process Record ("bound") etc... Also, the design you are using to bail out, is using labels and gotos unnecessarily. This, in turn, leads to the following oddity in the case above: (gdb) cont Process record does not support instruction bound. Process record does not support instruction 0x62 at address 0xdeadbeef. error: Process record: failed to record execution log. The first two error messages are almost identical and the second one should simply go. I 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: error: Process record: Failed to record execution log: Process record does not support instruction bound at address 0xfeedface. 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: 1. Define a new function: i386_insn_record_not_supported (struct gdbarch *gdbarch, const char *insn_name, CORE_ADDR insn_addr); Change the code above to: i386_insn_record_not_supported (gdbarch, "bound", ir.addr); return -1; 2. Define a new function: i386_opcode_record_not_supported (strct gdbarch* gdbarch, uint32_t opcode, CORE_ADDR insn_addr); Change the goto no_support into: i386_opcode_record_not_supported (gdbarch, opcode, ir.addr); 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. > + case 0x0f0f: /* 3DNow! data */ > + if (i386_record_modrm (&ir)) > + return -1; 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. > + switch (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). -- Joel