This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]