From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18600 invoked by alias); 29 Aug 2009 21:20:56 -0000 Received: (qmail 18592 invoked by uid 22791); 29 Aug 2009 21:20:56 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_28 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; Sat, 29 Aug 2009 21:20:50 +0000 Received: from mailhost3.vmware.com (mailhost3.vmware.com [10.16.27.45]) by smtp-outbound-2.vmware.com (Postfix) with ESMTP id B4F3A1B006; Sat, 29 Aug 2009 14:20:48 -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 AC29DCD90E; Sat, 29 Aug 2009 14:20:48 -0700 (PDT) Message-ID: <4A999BC3.5020606@vmware.com> Date: Sat, 29 Aug 2009 21:34: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 Subject: Re: [RFA/prec] Make i386 handle segment register better References: 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-08/txt/msg00550.txt.bz2 Hui Zhu wrote: > Hi guys, > > In prec-fix-x86-strinsn.txt patch, I add code the compare the ES and > DS to make sure if es if same with ds or not. > I think it works not bad, so I make a patch to check other segment > regiser like it. > > Please help me with it. Thanks for doing this! I think it looks good, but I have a couple of questions: > 2009-08-29 Hui Zhu > > * i386-tdep.c (i386_record_check_override): New function. > (i386_record_lea_modrm): Call i386_record_check_override. > (i386_process_record): Ditto. > > --- > i386-tdep.c | 37 ++++++++++++++++++++++++++----------- > 1 file changed, 26 insertions(+), 11 deletions(-) > > --- a/i386-tdep.c > +++ b/i386-tdep.c > @@ -3147,6 +3147,26 @@ no_rm: > return 0; > } > > +static int > +i386_record_check_override (struct i386_record_s *irp) > +{ > + if (irp->override >= 0 && irp->override != X86_RECORD_DS_REGNUM) > + { > + ULONGEST tmp, ds; > + > + regcache_raw_read_unsigned (irp->regcache, > + irp->regmap[irp->override], > + &tmp); > + regcache_raw_read_unsigned (irp->regcache, > + irp->regmap[X86_RECORD_DS_REGNUM], > + &ds); > + if (tmp != ds) > + return 1; > + } > + > + return 0; > +} > + > /* Record the value of the memory that willbe changed in current instruction > to "record_arch_list". > Return -1 if something wrong. */ > @@ -3157,7 +3177,7 @@ i386_record_lea_modrm (struct i386_recor > struct gdbarch *gdbarch = irp->gdbarch; > uint64_t addr; > > - if (irp->override >= 0) > + if (i386_record_check_override (irp)) > { > if (record_debug) > printf_unfiltered (_("Process record ignores the memory change " In this case, you "return 0", so it is true that we "ignore the memory change". In some cases below, you use an "if/else", so it is also true that we "ignore the memory change". But in the "String ops" case, there is no "else", so we really do *not* ignore the memory change. Should we be consistant, and add an "else" to the string ops case? See further comments at end. > @@ -4039,7 +4059,7 @@ reswitch: > /* mov EAX */ > case 0xa2: > case 0xa3: > - if (ir.override >= 0) > + if (i386_record_check_override (&ir)) > { > if (record_debug) > printf_unfiltered (_("Process record ignores the memory change " OK, this one is an "if/else", so you don't record the memory. > @@ -4458,13 +4478,8 @@ reswitch: > ir.regmap[X86_RECORD_REDI_REGNUM], > &tmpulongest); > > - regcache_raw_read_unsigned (ir.regcache, > - ir.regmap[X86_RECORD_ES_REGNUM], > - &es); > - regcache_raw_read_unsigned (ir.regcache, > - ir.regmap[X86_RECORD_DS_REGNUM], > - &ds); > - if (ir.aflag && (es != ds)) > + ir.override = X86_RECORD_ES_REGNUM; > + if (ir.aflag && i386_record_check_override (&ir)) > { > /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */ > if (record_debug) But in this case, there is no "else", so you still record the memory even if i386_record_check_override returns true. > @@ -5086,7 +5101,7 @@ reswitch: > opcode = opcode << 8 | ir.modrm; > goto no_support; > } > - if (ir.override >= 0) > + if (i386_record_check_override (&ir)) > { > if (record_debug) > printf_unfiltered (_("Process record ignores the memory " This is an "if/else" so you don't record the memory. > @@ -5138,7 +5153,7 @@ reswitch: > else > { > /* sidt */ > - if (ir.override >= 0) > + if (i386_record_check_override (&ir)) > { > if (record_debug) > printf_unfiltered (_("Process record ignores the memory " And this one is also an if/else. So I guess my questions are: 1) Should you use an "else" in the "String ops" case? 2) Should we go ahead and record the register changes, even though we can't record the memory change? 3) Should this be a warning, rather than just a debug message? I think yes, because if this happens, it actually means that the record log will be inaccurate. That's all for now, Michael