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

[Guinevere Larsen <blarsen@redhat.com>]

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.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>
> Tom
>