This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/3] MIPS: Provide FPU info and decode FCSR in `info float'
- From: "Maciej W. Rozycki" <macro at codesourcery dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: Yao Qi <yao at codesourcery dot com>, <gdb-patches at sourceware dot org>
- Date: Sun, 21 Dec 2014 00:13:52 +0000
- Subject: Re: [PATCH 3/3] MIPS: Provide FPU info and decode FCSR in `info float'
- Authentication-results: sourceware.org; auth=none
- References: <1418798765-23198-1-git-send-email-yao at codesourcery dot com> <1418798765-23198-4-git-send-email-yao at codesourcery dot com> <5491609E dot 1080204 at redhat dot com>
On Wed, 17 Dec 2014, Pedro Alves wrote:
> > +static void
> > +mips_print_float_info (struct gdbarch *gdbarch, struct ui_file *file,
> > + struct frame_info *frame, const char *args)
> > +{
> > + int fcsr = mips_regnum (gdbarch)->fp_control_status;
> > + enum mips_fpu_type type = MIPS_FPU_TYPE (gdbarch);
> > + ULONGEST fcs = 0;
> > + int i;
> > +
> > + if (fcsr == -1 || !deprecated_frame_register_read (frame, fcsr, NULL))
> > + type = MIPS_FPU_NONE;
>
> "deprecated" method usage alert. It's better to use methods that return
> values and then print "<unsaved>", "<unavailable>" as appropriate,
> though in this case you may be able to just use read_frame_register_unsigned
> instead.
For the record, we use `get_frame_register_unsigned' in such cases
elsewhere, e.g. `micromips_bc1_pc' and we don't expect it to fail on the
assumption that `fp_control_status' will have been set correctly. I see
this change went in already, but I'd like to see it updated with a
follow-up change for consistency.
Specifically `get_frame_register_unsigned' is unsuitable here (as we want
to poke at the actual register file, not the frame state), but I think a
failure from `read_frame_register_unsigned' should be signalled as an
error (exception being thrown, just as from the former function) rather
than being silently treated as if there were no FPU.
Also I think the registers should be dumped earlier on, with only generic
FPU parameters preceding, as the extra decoded FCSR information is an
elaboration of raw register contents, so I find it logical (as far as the
reading flow of English text is considered) to be presented later, i.e.:
fpu type: ...
reg size: ...
[registers]
cond : ...
[...]
Or so I think. It would help reviewing it if the updated version of the
patch was presented with corresponding output produced accompanying, just
as I did with the original version. Otherwise it requires drawing the
output in one's head which is not necessarily straightforward.
Maciej