Re: [PATCH 5/6] gdb/x86: Implement ax_pseudo_register_collect hook.

Marcin Kościelnicki
2016-02-10T13:34:59Z

On 10/02/16 14:31, Pedro Alves wrote:
> Great stuff. Minor comments follow...
>
> Patch is missing intro comments to new functions:
>
> /* Implementation of foo. / See foo.h. / etc. */

OK.
>
> On 02/06/2016 08:54 PM, Marcin KoÅ›cielnicki wrote:
>
>> +static int
>> +amd64_ax_pseudo_register_collect (struct gdbarch *gdbarch,
>> + struct agent_expr *ax, int regnum)
>> +{
>> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>
> Empty line after decl here.

OK.
>
>
>> +int
>> +i386_ax_pseudo_register_collect (struct gdbarch *gdbarch,
>> + struct agent_expr *ax, int regnum)
>> +{
>> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>
> Ditto.
>
>> + if (i386_mmx_regnum_p (gdbarch, regnum))
>> + {
>> + /* MMX to FPU register mapping depends on current TOS. Let's just
>> + not care and collect everything... */
>> + int i;
>
> Ditto. (several instances more in the function)
>
>> + ax_reg_mask (ax, I387_FSTAT_REGNUM (tdep));
>> + for (i = 0; i < 8; i++)
>> + ax_reg_mask (ax, I387_ST0_REGNUM (tdep) + i);
>> + return 0;
>> + }
>
>
>> + else if (i386_byte_regnum_p (gdbarch, regnum))
>> + {
>> + /* Check byte pseudo registers last since this function will
>> + be called from amd64_ax_pseudo_register_collect, which handles
>> + byte pseudo registers differently. */
>
> I don't understand this comment. amd64_ax_pseudo_register_collect
> checks i386_byte_regnum_p before ever reaching here, afaics.

I've copied it from i386_pseudo_register_read_into_value - didn't make
that much sense to me either. Let's remove it from both places?
>
>> + int gpnum = regnum - tdep->al_regnum;
>> + ax_reg_mask (ax, gpnum % 4);
>> + return 0;
>> + }
>> + else
>> + internal_error (__FILE__, __LINE__, _("invalid regnum"));
>> + return 1;
>> +}
>
> Thanks,
> Pedro Alves
>