Re: [RFA/i386] Prec x86 MMX 3DNow! SSE SSE2 SSE3 SSSE3 SSE4 support

[Hui Zhu <teawater@gmail.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