This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA/i386] Prec x86 MMX 3DNow! SSE SSE2 SSE3 SSSE3 SSE4 support
- From: Hui Zhu <teawater at gmail dot com>
- To: Joel Brobecker <brobecker at adacore dot com>
- Cc: Michael Snyder <msnyder at vmware dot com>, gdb-patches ml <gdb-patches at sourceware dot org>
- Date: Mon, 11 Jan 2010 10:47:36 +0800
- Subject: Re: [RFA/i386] Prec x86 MMX 3DNow! SSE SSE2 SSE3 SSSE3 SSE4 support
- References: <daef60380912070700t70e2a9ffg652549c285809b98@mail.gmail.com> <4B215151.6040608@vmware.com> <daef60380912110659y80e93fdxe3dd266a55931dc0@mail.gmail.com> <daef60380912120859r38de1672ofe60a545a6e4c21@mail.gmail.com> <4B26825B.7000209@vmware.com> <daef60380912141746i1325610fp1ede9eeacc28c209@mail.gmail.com> <4B2BDAEC.7090207@vmware.com> <daef60381001080807i402826d2h43aa14ab242ecf36@mail.gmail.com> <20100109122833.GB2007@adacore.com>
Thanks Joel,
On Sat, Jan 9, 2010 at 20:28, Joel Brobecker <brobecker@adacore.com> wrote:
>> 2010-01-09 ?Hui Zhu ?<teawater@gmail.com>
>>
>> ? ? ? * 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...
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
>> ? ? ? ?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;
>
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.
>
>> + ? ? ?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).
>
+ if (target_read_memory (ir.addr, &tmpu8, 1))
+ {
+ printf_unfiltered (_("Process record: error reading memory at "
+ "addr %s len = 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