[RFA/i386] Prec x86 MMX 3DNow! SSE SSE2 SSE3 SSSE3 SSE4 support
Hui Zhu
teawater@gmail.com
Mon Jan 11 02:48:00 GMT 2010
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
More information about the Gdb-patches
mailing list