Re: [PATCH 1/3] gdb: Start supporting AVX instruction

[Guinevere Larsen <blarsen@redhat.com>]

On 6/5/24 13:11, Tom Tromey wrote:
>>>>>> "Guinevere" == Guinevere Larsen <blarsen@redhat.com> writes:
> Guinevere> +  /* These are used by VEX and XOP prefixes.  */
> Guinevere> +  uint8_t map_select;
> Guinevere> +  uint8_t vvvv;
> Guinevere> +  uint8_t pp;
> Guinevere> +  uint8_t l;
>
> Are these the names used by the ISA?  Since if not they are pretty ugly.
Yes, these are the ISA names for those bits. I would use more 
descriptive names instead of ISA ones, but I have no idea what they are 
supposed to mean.
>
> Guinevere> +static bool
> Guinevere> +i386_record_vex (struct i386_record_s *ir, uint8_t rex_w, uint8_t rex_r,
> Guinevere> +		 int opcode, struct gdbarch *gdbarch)
> Guinevere> +{
> Guinevere> +  switch (opcode)
> Guinevere> +    {
> Guinevere> +    default:
> Guinevere> +      gdb_printf (gdb_stderr,
> Guinevere> +		  _("Process record does not support VEX instruction 0x%02x "
> Guinevere> +		    "at address %s.\n"),
> Guinevere> +		  (unsigned int) (opcode),
> Guinevere> +		  paddress (gdbarch, ir->orig_addr));
> Guinevere> +      return -1;
>
> This should use true or false, not -1, because the function returns
> bool.
nice catch! the function should be returning int instead.
>
> I wouldn't be surprised to get some kind of dead code warning here,
> since the switch only has a single branch.
no warnings in my system... but that is possible, I guess. Do you think 
it would be better to merge this commit with the following one?

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>
> Tom
>