From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31359 invoked by alias); 17 Jul 2009 23:58:45 -0000 Received: (qmail 31344 invoked by uid 22791); 17 Jul 2009 23:58:45 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_22,J_CHICKENPOX_25 X-Spam-Check-By: sourceware.org Received: from smtp-outbound-2.vmware.com (HELO smtp-outbound-2.vmware.com) (65.115.85.73) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 17 Jul 2009 23:58:36 +0000 Received: from mailhost3.vmware.com (mailhost3.vmware.com [10.16.27.45]) by smtp-outbound-2.vmware.com (Postfix) with ESMTP id 96C2E1410E; Fri, 17 Jul 2009 16:58:32 -0700 (PDT) Received: from [10.20.94.141] (msnyder-server.eng.vmware.com [10.20.94.141]) by mailhost3.vmware.com (Postfix) with ESMTP id 8E22FCD980; Fri, 17 Jul 2009 16:58:32 -0700 (PDT) Message-ID: <4A610EE8.1090904@vmware.com> Date: Sat, 18 Jul 2009 01:20:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20080411) MIME-Version: 1.0 To: Hui Zhu CC: gdb-patches ml , mark.kettenis@xs4all.nl Subject: Re: [RFA/RFC Prec] Add Linux AMD64 process record support second version, (instruction set support) 1/3 References: <4A5A810B.7080603@vmware.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: 2009-07/txt/msg00441.txt.bz2 > Thanks Michael, > > I updated all the AMD64 patches. Please help me review it. > > Thanks, > Hui > > 2009-07-17 Hui Zhu > > Add AMD64 process record instruction set support. > > * i386-tdep.h (gdbarch_tdep): Add record_regmap for registers > because the AMD64's registers order in GDB is not same with > I386 instructions. > Add i386_syscall_record to be the syscall function handle > interface. > (record_i386_regnum): Number for record_regmap. > * i386-tdep.c (OT_QUAD): For 64 bits. > (i386_record_s): Add rex_x, rex_b, rip_offset and > popl_esp_hack for AMD64 instruction set. And regmap for > record_regmap. > (i386_record_lea_modrm_addr): Support AMD64 instruction set > 64 bits lea. > (i386_record_lea_modrm): Ditto. > (i386_record_push): New function. Record the execution log > of push. > (I386_RECORD_ARCH_LIST_ADD_REG): New macro to record the > register. > (i386_process_record): Support AMD64 instruction set. > amd64-tdep.c (amd64_record_regmap): For record_regmap. Should be a "*" at the beginning of the line above. > @@ -2864,6 +2865,7 @@ enum > > struct i386_record_s > { > + struct gdbarch *gdbarch; > struct regcache *regcache; > CORE_ADDR addr; > int aflag; > @@ -2872,6 +2874,11 @@ struct i386_record_s > uint8_t modrm; > uint8_t mod, reg, rm; > int ot; > + uint8_t rex_x; > + uint8_t rex_b; > + int rip_offset; > + int popl_esp_hack; popl_esp_hack is used but never initialized. Do you still need it? Is it for some future use? > @@ -4611,14 +4633,13 @@ reswitch: > case 0x0f9d: > case 0x0f9e: > case 0x0f9f: > + I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM); > ir.ot = OT_BYTE; > if (i386_record_modrm (&ir)) > return -1; > if (ir.mod == 3) > - { > - if (record_arch_list_add_reg (ir.regcache, ir.rm & 0x3)) > - return -1; > - } > + I386_RECORD_ARCH_LIST_ADD_REG (ir.rex_b ? (ir.rm | ir.rex_b): > + (ir.rm & 0x3)); Space before colon operator (':'), and then the 2nd line isn't indented right. I know it looks prettier the way it is, but it should be over to the left. ;-) > @@ -5115,21 +5225,21 @@ reswitch: > > /* arpl */ > case 0x63: > - ir.ot = ir.dflag ? OT_LONG : OT_WORD; > if (i386_record_modrm (&ir)) > return -1; > - if (ir.mod != 3) > - { > - if (i386_record_lea_modrm (&ir)) > - return -1; > - } > + if (ir.mod == 3 || ir.regmap[X86_RECORD_R8_REGNUM]) > + { > + I386_RECORD_ARCH_LIST_ADD_REG (ir.regmap[X86_RECORD_R8_REGNUM]? > + (ir.reg | rex_r) : ir.rm); Space before question-mark, and then the indentation of the second line. > @@ -5232,9 +5341,8 @@ reswitch: > break; > } > > -/* In the future, Maybe still need to deal with need_dasm */ > - if (record_arch_list_add_reg (ir.regcache, I386_EIP_REGNUM)) > - return -1; > + /* In the future, Maybe still need to deal with need_dasm */ "maybe" lower case. Period and two spaces at end of sentence. Everything else in this patch looks great! Mark Kettenis should approve it, though. Michael