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

[Joel Brobecker <brobecker at adacore dot com>]

> 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...

> @@ -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;

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).

-- 
Joel