Re: [PATCH 2/3] gdb/record: add support to vmovd and vmovq instructions

[Guinevere Larsen <blarsen@redhat.com>]

On 6/5/24 15:24, Guinevere Larsen wrote:
> On 6/5/24 13:13, Tom Tromey wrote:
>>>>>>> "Guinevere" == Guinevere Larsen <blarsen@redhat.com> writes:
>> Guinevere> This commit adds support to the x86_64 AVX instructions 
>> vmovd and vmovq.
>> Guinevere> The programmers manuals for Intel and AMD describe these 2 
>> instructions
>> Guinevere> as being almost the same, but my local testing, using gcc 
>> 13.2 on Fedora
>> Guinevere> 39, showed several differences and inconsistencies.
>>
>> Guinevere>  static bool
>> Guinevere> -i386_record_vex (struct i386_record_s *ir, uint8_t rex_w, 
>> uint8_t rex_r,
>> Guinevere> +i386_record_vex (struct i386_record_s *ir, uint8_t vex_w, 
>> uint8_t vex_r,
>> Guinevere>           int opcode, struct gdbarch *gdbarch)
>> Guinevere>  {
>> ...
>> Guinevere> +    error ("Unrecognized VEX.PP value %d at address %s.",
>> Guinevere> +           ir->pp, paddress(gdbarch, ir->orig_addr));
>>
>> I don't know anything about record, really, but it seems weird to have a
>> function that has two error return mechanisms -- either calling error or
>> printing a message and returning some value:
>>
>> Guinevere>      default:
>> Guinevere>        gdb_printf (gdb_stderr,
>> Guinevere>            _("Process record does not support VEX 
>> instruction 0x%02x "
>>
>> What's the deal with that?
>
> Christmas vacation in between, mostly.
>
> gdb_printf is what i386_process_record already uses, so felt 
> reasonable, then I chose "error" int the other situation because it 
> felt more appropriate for me to have a strong error when we find 
> something that, to the best of my knowledge, breaks the ISA. To me it 
> marks a difference between "we're not equipped to handle" versus "up 
> to most recent ISA, this is straight up incorrect", but if you think 
> it's better to only use one error mechanism, I can switch to 
> gdb_printf everywhere.
>
Interestingly enough, I just double checked what happens, and for the 
end user, the gdb_printf option looks stronger because gdbarch will add

 > Process record: failed to record execution log.

and error will not print anything but the message given. I'll 
standardize on the gdb_printf version instead.  Thanks for the pointer!

-- 
Cheers,
Guinevere Larsen
She/Her/Hers